-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Add standard metrics custom processor and request duration metric #33955
Add standard metrics custom processor and request duration metric #33955
Conversation
API change check API changes are not detected in this pull request. |
...tor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/StandardMetricsExtractionProcessor.cs
Show resolved
Hide resolved
sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/AzureMonitorExporterTraceExtensions.cs
Outdated
Show resolved
Hide resolved
sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/MetricDataPoint.cs
Show resolved
Hide resolved
...tor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/StandardMetricsExtractionProcessor.cs
Outdated
Show resolved
Hide resolved
...tor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/StandardMetricsExtractionProcessor.cs
Show resolved
Hide resolved
...enTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.Tests/StandardMetricTests.cs
Outdated
Show resolved
Hide resolved
...enTelemetry.Exporter/tests/Azure.Monitor.OpenTelemetry.Exporter.Tests/StandardMetricTests.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
TagList tags = default; | ||
tags.Add(new KeyValuePair<string, object>(StandardMetricConstants.RequestResultCodeKey, statusCodeAttributeValue)); |
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.
handle the case where statusCodeAttributeValue
is null
tags.Add(new KeyValuePair<string, object>(StandardMetricConstants.IsAutoCollectedKey, "True")); | ||
tags.Add(new KeyValuePair<string, object>(StandardMetricConstants.IsSyntheticKey, "False")); | ||
tags.Add(new KeyValuePair<string, object>(StandardMetricConstants.CloudRoleInstanceKey, StandardMetricResource.RoleInstance)); | ||
tags.Add(new KeyValuePair<string, object>(StandardMetricConstants.CloudRoleNameKey, StandardMetricResource.RoleName)); |
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.
this should be in resource.
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 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.
Need to look in to this. Resource attributes are not converted to telemetry custom dimensions right now. And standard metric requires these to be at the dimension level.
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 resource attributes are not converted, then its already a bug in MetricExporter, that must be separately addressed. Atleast the rolename/roleinstance part from resource should be set as dimensions.
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.
Atleast the rolename/roleinstance part from resource should be set as dimensions.
This gets converted to cloud role and role instance on telemetryItem level not as dimensions.
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.
Atleast the rolename/roleinstance part from resource should be set as dimensions.
This gets converted to cloud role and role instance on telemetryItem level not as dimensions.
yes got it. that is incorrect and should be fixed.
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.
Should we duplicate again in telemetry properties?
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.
yes
This PR adds a custom span processor for extracting standard metrics. It also adds
request/duration
metric.TODOs:
From end user perspective this will be enabled by default on configuring metric exporter.
Adding the complete design plan for extracting standard metrics below
Proposal: Standard metric collection in OpenTelemetry can be enabled by customizing following two parts in SDK:
Sampler: When the sampler decides to drop the data return SamplingResult.RecordOnly instead of SamplingResult.None. This will achieve following two things:
Span will be created and will be populated with all the information (tags, events etc.)SampledFlag will still be set to false. So, this will not impact propagated sampling decision i.e. traceflags information.
Custom Processor:Add a custom processor which will extract standard metrics information as required. These metrics can then be reported as standard metrics using a metric exporter.