-
Notifications
You must be signed in to change notification settings - Fork 494
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
CosmosClientSideRequestStatistics merged into DiagnosticContext #1189
Conversation
Microsoft.Azure.Cosmos/src/Diagnostics/AddressResolutionStatistics.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosClientSideRequestStatistics.cs
Show resolved
Hide resolved
@@ -93,6 +93,21 @@ internal override void AddDiagnosticsInternal(PointOperationStatistics pointOper | |||
this.ContextList.Add(pointOperationStatistics); | |||
} | |||
|
|||
internal override void AddDiagnosticsInternal(StoreResponseStatistics storeResponseStatistics) |
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.
You could probably write a visitor for this, so you will know to update this when a new type gets added.
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.
The problem is abstract classes. It can't be both a CosmosDiagnosticsInternal
and a CosmosDiagnosticsInternalVisitor
. Also it does not need to support all vistors.
Microsoft.Azure.Cosmos/src/Diagnostics/StoreResponseStatistics.cs
Outdated
Show resolved
Hide resolved
…c context on tostring to avoid missing information.
…void breaking changes.
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.
892d0bc
… requests better.
…diagnostic context.
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosClientSideRequestStatistics.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosClientSideRequestStatistics.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosClientSideRequestStatistics.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosClientSideRequestStatistics.cs
Outdated
Show resolved
Hide resolved
…iagnosticContext to get the full information.
…nostics/fix_client_stats
Pull Request Template
Description
AddressResolutionStatistics and StoreResponseStatistics are now merged into the diagnostic context list. This allows the results to be shown in order with the other context information rather than a single aggregated result. This allows to see which retry has which AddressResolutionStatistics and StoreResponseStatistics.
CosmosClientSideRequestStatistics is added to the context a single time to avoid duplicating the same data for multiple retries.
Type of change
Please delete options that are not relevant.
Example:
// Success scenario
Request To large:
Failed with 429s: