-
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
Adding diagnostic context #1062
Conversation
…ime. Fixed tests.
Can you also please include CP sample as well? |
* Wired batch and bulk with diagnostic context. * fixed comments * Fixed bulk test
Lets please create a new policy every time. Refers to: Microsoft.Azure.Cosmos/src/DocumentClient.cs:699 in ccdaf97. [](commit_id = ccdaf97, deletion_comment = False) |
Microsoft.Azure.Cosmos/src/Query/Core/Metrics/SchedulingTimeSpan.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticScope.cs
Outdated
Show resolved
Hide resolved
|
||
if (this.ElapsedTime.HasValue) | ||
{ | ||
jsonWriter.WriteValue(this.ElapsedTime.Value); |
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.
Elapsed is set only on Dispose(), which Write is consumed before. How will it be set?
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.
There is the possibility of async background tasks that may still be running or possibility of a scope not getting disposed of correctly in an exception scenario. Either way it's better to gracefully handle the scenario than throw an exception.
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContext.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsContext.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticSummary.cs
Outdated
Show resolved
Hide resolved
|
||
internal override void WriteJsonObject(JsonWriter jsonWriter) | ||
{ | ||
foreach (CosmosDiagnosticWriter writer in this.contextList) |
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.
For queries this might get really high. What's the V2 story?
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.
If this becomes to large HA will need to provide a way to summarize the content. The current design to v2 has 2 different properties diagnostic string and query metrics. Query metrics does have some ability to be summarized, but diagnostic string does not.
Pull Request Template
Description
This PR adds a diagnostics core object. It has an inner class to create a scope which is labeled. Each scope track the start time and the duration. This allows each layer of the SDK to tracked. The outer diagnostics core also has a basic overview if there is retries.
Key design decisions:
Additional diagnostics being added:
Type of change
Please delete options that are not relevant.
Performance results:
// Original no diagnostics
BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18363
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET Core SDK=3.0.100
[Host] : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
MediumRun : .NET Core 3.0.0 (CoreCLR 4.700.19.46205, CoreFX 4.700.19.46214), X64 RyuJIT
Job=MediumRun IterationCount=15 LaunchCount=2
WarmupCount=10
// Original with Diagnostics
Job=MediumRun IterationCount=15 LaunchCount=2
WarmupCount=10
// With changes no diganostics
Job=MediumRun IterationCount=15 LaunchCount=2
WarmupCount=10
// With changes and diagnostics
Job=MediumRun IterationCount=15 LaunchCount=2
WarmupCount=10