add isSynthetic to StandardMetrics #44091
Conversation
TimothyMothra
requested review from
cijothomas,
reyang,
rajkumar-rangaraj and
vishweshbankwar
as code owners
|
API change check API changes are not detected in this pull request. |
| @@ -27,5 +29,7 @@ internal static class StandardMetricConstants | |||
| internal const string CloudRoleNameKey = "cloud/roleName"; | |||
| internal const string CloudRoleInstanceKey = "cloud/roleInstance"; | |||
| internal const string MetricIdKey = "_MS.MetricId"; | |||
|
|
|||
| internal static ReadOnlySpan<string> SyntheticUserAgentValues => new string[] { "AlwaysOn", "AppInsights", "search", "spider", "crawl", "Bot", "Monitor", "Gomez", "Pingdom" }; | |||
There was a problem hiding this comment.
it'd be nice to add a comment about how this list was come up with. Particularly "AppInsights".
There was a problem hiding this comment.
hmmm. Let me follow up with Jackson on this.
This is the list that was in our spec, but it differs from our legacy sdk.
https://github.com/microsoft/ApplicationInsights-dotnet/blob/99118c924d3880f3853c0b1c9ebe67289073dbfa/examples/.NET%20Config%20File/2.10.0_ApplicationInsights.config#L12
There was a problem hiding this comment.
I don't think this should block the PR. Any changes should come through the spec and we can update this list as needed
There was a problem hiding this comment.
That is just example of the extended list, not the default followed in ApplicationInsights. The actual list is "search|spider|crawl|Bot|Monitor|AlwaysOn".
Also, the above is not covering ApplicationInsights WebTests.
They are covered by separate headers (different from user-agent) https://github.com/microsoft/ApplicationInsights-dotnet/blob/main/NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/TelemetryInitializers/SyntheticTelemetryInitializer.cs#L42-L43
Do we know if ApplicationInsights WebTests send a useragent from the above list? if not, please make sure the spec is fixed.
There was a problem hiding this comment.
Ohhhhh, we have different Synthetic TelemetryInitializers in the legacy sdk. I missed this.
|
|
||
| WaitForActivityExport(traceTelemetryItems); | ||
|
|
||
| standardMetricCustomProcessor._meterProvider?.ForceFlush(); |
There was a problem hiding this comment.
mm.. why would this flush be needed? The flush on provider should call flush on all processors already.
There was a problem hiding this comment.
I'm hesitant to make changes here because the tests are working. :)
| .AddProcessor(new BatchActivityExportProcessor(new AzureMonitorTraceExporter(new AzureMonitorExporterOptions(), new MockTransmitter(traceTelemetryItems)))) | ||
| .Build(); | ||
|
|
||
| using (var activity = activitySource.StartActivity("Test", ActivityKind.Consumer)) |
There was a problem hiding this comment.
curious why Consumer kind is used? Standard metrics are calculated for Server/Client as well, and the name of the metric also differs, right?
There was a problem hiding this comment.
The spec says IsSynthetic is only for requests.
I don't think there was any specific reason ActivityKind.Consumer was used here instead of ActivityKind.Server.
We could add both to the test cases, but adding an additional dimension would double the existing cases.
There was a problem hiding this comment.
Is there a test confirming that we only add IsSynthetic dimension for Request Telemetry only? The standard metrics likely have very rigid backend restrictions, potentially dropping entire metric, if additional dimensions are sent!
Also, as per the doc here : https://learn.microsoft.com/en-us/azure/azure-monitor/reference/supported-metrics/microsoft-insights-components-metrics the synthetic flag is applicable for Dependency, TraceTelemetry as well.
There was a problem hiding this comment.
Okay, we need to block this PR until I can get some clarifications.
| @@ -79,6 +86,7 @@ private void ReportRequestDurationMetric(Activity activity) | |||
| tags.Add(new KeyValuePair<string, object?>(StandardMetricConstants.CloudRoleInstanceKey, StandardMetricResource?.RoleInstance)); | |||
| tags.Add(new KeyValuePair<string, object?>(StandardMetricConstants.CloudRoleNameKey, StandardMetricResource?.RoleName)); | |||
| tags.Add(new KeyValuePair<string, object?>(StandardMetricConstants.RequestSuccessKey, RequestData.IsSuccess(activity, statusCodeAttributeValue, OperationType.Http))); | |||
| tags.Add(new KeyValuePair<string, object?>(StandardMetricConstants.IsSyntheticKey, IsSyntheticRequest(userAgent) ? "True" : "False")); | |||
There was a problem hiding this comment.
not part of this PR, but just noticed that most of the other tags for the metric is fixed. So instead of passing them with each Record call, they should be modelled as resource/meter tags, and let exporter extract and puts them in proper place. This is significant perf overhead otherwise.
Take a look at the benchmarks to see how the perf is impacted with each additional dimension!
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/Benchmarks/Metrics/HistogramBenchmarks.cs#L22-L25
(Not a blocker for this PR)
|
Converting to draft so this doesn't merge. Need to get clarification on the requirements. |
This PR adds isSynthetic to StandardMetrics, as per the spec.
Notes
This is almost identical to the implementation in Application Insights SDK.
https://github.com/microsoft/ApplicationInsights-dotnet/blob/99118c924d3880f3853c0b1c9ebe67289073dbfa/WEB/Src/Web/Web/SyntheticUserAgentTelemetryInitializer.cs#L69-L82