HttpSemanticConventions - update AspNetCore histogram #4766
Conversation
| @@ -78,6 +78,10 @@ public static class MeterProviderBuilderExtensions | |||
|
|
|||
| builder.AddMeter(AspNetCoreMetrics.InstrumentationName); | |||
|
|
|||
| builder.AddView( | |||
| instrumentName: HttpInMetricsListener.HttpServerDurationMetricName, | |||
There was a problem hiding this comment.
Add a check for both Meter name and Instrument name.
| @@ -78,6 +78,10 @@ public static class MeterProviderBuilderExtensions | |||
|
|
|||
| builder.AddMeter(AspNetCoreMetrics.InstrumentationName); | |||
|
|
|||
| builder.AddView( | |||
There was a problem hiding this comment.
This should be done by the application user, not the instrumentation library. Instrumentation Library can set Hint (once that feature is available), but it should not try to Configure View. View is a SDK concept, and instrumentation library should not have sdk dependency at all. (its an issue that this library has an undesired dependency on sdk, thats tracked separately)...
There was a problem hiding this comment.
Experimental spec for Hint API ("instrument advice") is here:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#instrument-advice
Seems that Trask is ready to move it to Stable:
open-telemetry/opentelemetry-specification#3391
I had a brief chat with Utkarsh about this.
Seems the Hint API will be a blocker for this PR.
There was a problem hiding this comment.
Hint API require DiagnosticSource changes, so not coming this year.
But i don't know if Hint API support is required before making this change. As users always have View API to adjust bounds.
There was a problem hiding this comment.
IMO, that seems like a bad user experience.
If the Instrumentation library ships with the wrong buckets and users need to manually adjust.
There was a problem hiding this comment.
mm. If that is the case, then the stable release of instrumentation must be tied with next .NET release (Nov 2024). dotnet/runtime#63650 (comment)
Or some options must be added for instrumentation library to use ms instead of s. I recommend to create a dedicated issue to discuss this.
There was a problem hiding this comment.
@cijothomas On thinking a bit more about this, I think it should be okay for the instrumentation library to use View until the Hint API is offered from DiagnosticSource. When the Hint API is available, the instrumentation library could get rid of View and switch to using the Hint API.
As of today, anyone using AspNetCoreInstrumentation library is going to take a dependency on the SDK anyway.
There was a problem hiding this comment.
I can include a TODO here to call out that this needs to be changed to Hint API when available.
There was a problem hiding this comment.
I think it should be okay for the instrumentation library to use
Viewuntil the Hint API is offered from DiagnosticSource. When the Hint API is available, the instrumentation library could get rid ofViewand switch to using the Hint API.As of today, anyone using AspNetCoreInstrumentation library is going to take a dependency on the SDK anyway.
I would open a dedicated issue to discuss and record this decision. It's a pretty fundamental principle that instrumentations should only depend on the API and not on the SDK, and so you'll want to document the rationale clearly in this case and make sure your entire community is onboard and understands the ramifications.
There was a problem hiding this comment.
It can still cause other issues as Views are additive. If user adds another view matching the same metric, to drop an unwanted attribute, it'll result in 2 metrics stream.
Its a bug that the instrumentation has a SDK dependency. It is fixable (once we move the things the instrumentation need from SDK to API itself.). Its even worse if the instrumentation leverages the SDK dependency to define behavior, reserved for application owner to define.
There are other instrumentations where this trick/hack of using views from SDK cannot be used - like asp.net framework.
At the cost of exposing yet another public API, an option like RecordDurationInMillisecond could work, and is probably more desirable than misusing SDK dependency.
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4766 +/- ##
==========================================
+ Coverage 85.59% 85.70% +0.10%
==========================================
Files 284 284
Lines 11686 11697 +11
==========================================
+ Hits 10003 10025 +22
+ Misses 1683 1672 -11
|
| this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags); | ||
| this.httpServerDuration.Record(Activity.Current.Duration.TotalSeconds, tags); |
There was a problem hiding this comment.
Java gates this change with the OTEL_SEMCONV_STABILITY_OPT_IN setting. Maybe we should too since this is potentially disruptive to end users and backends.
Unfortunately, it's not explicitly stated that this change should be gated, but maybe it should be... @trask what do you think?
There was a problem hiding this comment.
within the scope of HTTP semconv, I think(?) this is implied by the spec, since unit seconds is part of the frozen http semconv for http.server.request.duration
There was a problem hiding this comment.
I think(?) this is implied by the spec
This was my original interpretation... but I also can understand how someone might have a different interpretation since the words from the warning text are very attribute-centric.
I'd be happy to take a stab at some clarifying verbiage to the text if folks would find that helpful.
There was a problem hiding this comment.
the words from the warning text are very attribute-centric.
ah, thx, I see the confusion now
I'd be happy to take a stab at some clarifying verbiage to the text if folks would find that helpful.
that would be great
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
Towards #4484.
CHANGED THIS PR TO DRAFT.
This PR is blocked until we have support for new default histogram bounds.
Changes
OTEL_SEMCONV_STABILITY_OPT_IN.HttpInMetricsListenerto emit new metric.http.server.duration(milliseconds) replaced withhttp.server.request.duration(seconds)As of today (Aug 22) this is implemented using the Views API.
This is a blocker for this PR while we explore alternatives.
MetricTests.RequestMetricIsCapturedto evaluate all states of the environment variable (old, new, both).Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes