Diagnostic Visitors #1156
Diagnostic Visitors #1156
Conversation
bchong95
requested review from
j82w,
khdang,
kirankumarkolli and
kirillg
as code owners
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsSerializerVisitor.cs
Outdated
Show resolved
Hide resolved
| @@ -123,6 +124,36 @@ sealed class BackendMetrics | |||
| /// </summary> | |||
| public TimeSpan VMExecutionTime { get; } | |||
|
|
|||
| public override string ToString() | |||
| { | |||
| return $"totalExecutionTimeInMs={this.TotalTime.TotalMilliseconds};" + | |||
There was a problem hiding this comment.
Use StringBuilder this is going to cause to much perf overhead. #Resolved
| } | ||
|
|
||
| internal void AddContextWriter(CosmosDiagnosticWriter diagnosticWriter) | ||
| internal void AddContextWriter(CosmosDiagnosticsInternal diagnosticWriter) |
There was a problem hiding this comment.
AddDiagnosticsInternal ? #Resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsSerializerVisitor.cs
Show resolved
Hide resolved
| this.jsonWriter = new JsonTextWriter(textWriter ?? throw new ArgumentNullException(nameof(textWriter))); | ||
| } | ||
|
|
||
| public override void Visit(PointOperationStatistics pointOperationStatistics) |
There was a problem hiding this comment.
Did this make any changes to the output string? If so can you provide an example in the description. #Resolved
Microsoft.Azure.Cosmos/src/Diagnostics/CosmosDiagnosticsInternalVisitor.cs
Show resolved
Hide resolved
| /// <summary> | ||
| /// Extracts the aggregated <see cref="BackendMetrics"/> from a <see cref="CosmosDiagnostics"/>. | ||
| /// </summary> | ||
| internal sealed class BackendMetricsExtractor : CosmosDiagnosticsInternalVisitor<(bool, BackendMetrics)> |
There was a problem hiding this comment.
(bool, BackendMetrics [](start = 85, length = 21)
Nullable #Resolved
There was a problem hiding this comment.
| /// <summary> | ||
| /// Extracts the aggregated <see cref="BackendMetrics"/> from a <see cref="CosmosDiagnostics"/>. | ||
| /// </summary> | ||
| internal sealed class BackendMetricsExtractor : CosmosDiagnosticsInternalVisitor<(bool, BackendMetrics)> |
There was a problem hiding this comment.
bool [](start = 86, length = 4)
enum for the failure reason #Resolved
There was a problem hiding this comment.
| if (!this.isDisposed) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think we should not have a TryGet method here; rather, we should return whatever value the stopwatch has #Resolved
There was a problem hiding this comment.
It's better to have a TryGet. It's misleading if it returns the current elapsed time if the operation was never disposed of or still hasn't completed if there is a background task then it causes it to be ambiguous. #Resolved
| { | ||
| CosmosDiagnosticsSerializerVisitor cosmosDiagnosticsSerializerVisitor = new CosmosDiagnosticsSerializerVisitor(textWriter); | ||
| this.Accept(cosmosDiagnosticsSerializerVisitor); | ||
| } |
There was a problem hiding this comment.
consider moving this to the base class #Resolved
There was a problem hiding this comment.
The base class doesn't have the visitor methods that are needed for this
In reply to: 369911329 [](ancestors = 369911329)
| @@ -12,6 +12,7 @@ namespace Microsoft.Azure.Cosmos.Tests | |||
| using System.Net.Http; | |||
| using System.Threading; | |||
| using System.Threading.Tasks; | |||
| using Microsoft.Azure.Cosmos.Diagnostics; | |||
There was a problem hiding this comment.
undo?
Similar places #Resolved
There was a problem hiding this comment.
The namespace was changed intentionally to match the file structure
In reply to: 369949865 [](ancestors = 369949865)
Diagnostic Visitors
Description
This PR adds visitor to CosmosDiagnostics. This ensures that for the abstract type CosmosDiagnostics the compute gateway can handle every derived type (or else there will be a compile time error). BackendMetricsExtractor is an example of this. This essentially turns a soft contract into a hard contract. I added
CosmosDiagnosticsInternalto extend CosmosDiagnostics, but for internal use only.