Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed the timers to track proper DB execution duration. #2536

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

lingxiao-microsoft
Copy link
Contributor

@lingxiao-microsoft lingxiao-microsoft commented Jan 23, 2025

Why make this change?

Previously we created a PR to add timers to track the duration of DB execution. However, there are some issues within that results in incorrect time duration logged.

Please exclude the whitespaces diff to have a better reviewing experience
image

  1. Previously we added timer in ExecuteQueryAsync and ExecuteQueryAgainstDbAsync
  2. However, within function ExecuteQueryAsync, it will invoke ExecuteQueryAgainstDbAsync .
  3. Which will result in logging the time duration twice.
  4. See repro below
    image

We can see the stored value is always twice as the stop watch
image
image

What is this change?

  • Fixing the timer to log properly
    • Removed the timer from upper function ExecuteQueryAsync and ExecuteQuery
    • Put the timer inside ExecuteQueryAgainstDbAsync and ExecuteQueryAgainstDb
    • Added try-finally block to wrap the functions.

How was this tested?

  • Integration Tests
  • Unit Tests

Sample Request(s)

  • Example REST and/or GraphQL request to demonstrate modifications
    image
    • Putting a breakpoint at the ExecuteQueryAsync level
    • Time durations are matched now
      image

@JerryNixon
Copy link
Contributor

@sezal98

@lingxiao-microsoft lingxiao-microsoft changed the title [DRAFT] Fixed the timers to track proper DB execution duration. Fixed the timers to track proper DB execution duration. Jan 24, 2025
@lingxiao-microsoft lingxiao-microsoft marked this pull request as ready for review January 24, 2025 18:18
Copy link
Contributor

@rohkhann rohkhann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@lingxiao-microsoft
Copy link
Contributor Author

/azp run

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Left a few comments.

@lingxiao-microsoft
Copy link
Contributor Author

/azp run

@lingxiao-microsoft lingxiao-microsoft enabled auto-merge (squash) January 29, 2025 17:59
@lingxiao-microsoft
Copy link
Contributor Author

/azp run

@lingxiao-microsoft lingxiao-microsoft merged commit 9fd99aa into main Jan 29, 2025
7 checks passed
@lingxiao-microsoft lingxiao-microsoft deleted the dev/lingxiao/graphql-fix-timer branch January 29, 2025 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants