-
Notifications
You must be signed in to change notification settings - Fork 497
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
[Internal] Diagnostics: Adds Merge API that combines several CosmosTraceDiagnostics Instances #4175
Conversation
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosTraceDiagnostics.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosItemTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
|
|
this.traceDiagnostics = traceDiagnostics; | ||
this.isMergedDiagnostics = true; | ||
|
||
TraceSummary traceSummary = new TraceSummary(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the summary for every merge?
ex:
ReadItem -> Inside RequestInvoker, will it include a summary?
Queries -> Which might go multiple NW interactions, will every interaction have its own summary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summar of offline sync-up: For initial version having the simple version with hint/context to dis-ambiguate if it's a hedging or not is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope: Lets just have the hint about retrycontext
If the hedging is from ReqeustInvoker, then clientconfig will not be present there right? Diagnostics might get repeated but that's something we can follow-up later. |
Follow-up from offline sync-up: Start with how the hedged diagnostics look like. Mostly only system info might be repeated and for initial preview we can live with it. |
Pull Request Template
Description
This PR adds a constructor for
CosmosTraceDiagnostics
that takes in a list ofCosmosTraceDiagnostics
and merges them into one. This feature will be used for when parallel request hedging is added to the SDK as a way to have the full context for a request that sends out multiple parallel requests.The Merged Diagnostics will be broken down into the following:
Other things that can be added to data
Example of Merged Diagnostics in Current form as a proof of concept (here there are two merged read requests to the emulator):
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #4179