Skip to content
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.

Sign up for GitHub

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

HttpSemanticConventions - update AspNetCore histogram #4766

Closed
wants to merge 9 commits into from
Closed

HttpSemanticConventions - update AspNetCore histogram #4766

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
975accc
update histogram metrics
TimothyMothra Aug 11, 2023
b8d69ac
changelog
TimothyMothra Aug 11, 2023
72b429e
remove extra using
TimothyMothra Aug 11, 2023
4f016cf
record seconds
TimothyMothra Aug 14, 2023
99d54c1
EnvVar will now affect metric. old: http.server.duration. new: http.s…
TimothyMothra Aug 22, 2023
56676a9
fix
TimothyMothra Aug 22, 2023
598ca2f
changelog
TimothyMothra Aug 22, 2023
d4776ba
Merge branch 'main' into 4484_aspnetcoreMetrics
TimothyMothra Aug 22, 2023
b2c5b82
Merge branch 'main' into 4484_aspnetcoreMetrics
TimothyMothra Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## Unreleased

* **Breaking change** The metric `http.server.duration` has been updated to the
latest spec for [metrics](https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-metrics.md).
Unit has been changed from millisecond to second and the histogram bounds
have been updated accordingly.
([#4766](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4766))

## 1.5.1-beta.1

Released 2023-Jul-20
Expand Down
10 changes: 7 additions & 3 deletions src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ namespace OpenTelemetry.Instrumentation.AspNetCore.Implementation;

internal sealed class HttpInMetricsListener : ListenerHandler
{
private const string HttpServerDurationMetricName = "http.server.duration";
internal const string HttpServerDurationMetricName = "http.server.duration";

// Http Metrics use custom histogram boundaries. See the spec: https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-metrics.md
internal static double[] HttpServerDurationMetricExplicitBounds = new double[] { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 };

private const string OnStopEvent = "Microsoft.AspNetCore.Hosting.HttpRequestIn.Stop";
private const string EventName = "OnStopActivity";

Expand All @@ -42,7 +46,7 @@ internal HttpInMetricsListener(string name, Meter meter, AspNetCoreMetricsInstru
{
this.meter = meter;
this.options = options;
this.httpServerDuration = meter.CreateHistogram<double>(HttpServerDurationMetricName, "ms", "Measures the duration of inbound HTTP requests.");
this.httpServerDuration = meter.CreateHistogram<double>(HttpServerDurationMetricName, "s", "Measures the duration of inbound HTTP requests.");
TimothyMothra marked this conversation as resolved.
Show resolved Hide resolved

this.emitOldAttributes = this.options.HttpSemanticConvention.HasFlag(HttpSemanticConvention.Old);

Expand Down Expand Up @@ -144,7 +148,7 @@ public override void OnEventWritten(string name, object payload)
// We are relying here on ASP.NET Core to set duration before writing the stop event.
// https://github.com/dotnet/aspnetcore/blob/d6fa351048617ae1c8b47493ba1abbe94c3a24cf/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L449
// TODO: Follow up with .NET team if we can continue to rely on this behavior.
this.httpServerDuration.Record(Activity.Current.Duration.TotalMilliseconds, tags);
this.httpServerDuration.Record(Activity.Current.Duration.TotalSeconds, tags);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/MeterProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ public static MeterProviderBuilder AddAspNetCoreInstrumentation(

builder.AddMeter(AspNetCoreMetrics.InstrumentationName);

builder.AddView(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, that seems like a bad user experience.
If the Instrumentation library ships with the wrong buckets and users need to manually adjust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can include a TODO here to call out that this needs to be changed to Hint API when available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

instrumentName: HttpInMetricsListener.HttpServerDurationMetricName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a check for both Meter name and Instrument name.

metricStreamConfiguration: new ExplicitBucketHistogramConfiguration { Boundaries = HttpInMetricsListener.HttpServerDurationMetricExplicitBounds });

builder.AddInstrumentation(sp =>
{
var options = sp.GetRequiredService<IOptionsMonitor<AspNetCoreMetricsInstrumentationOptions>>().Get(name);
Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry.Instrumentation.AspNetCore/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,12 @@ public void ConfigureServices(IServiceCollection services)
#### List of metrics produced

The instrumentation is implemented based on [metrics semantic
conventions](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#metric-httpserverduration).
conventions](https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/http/http-metrics.md#metric-httpserverduration).
Currently, the instrumentation supports the following metric.

| Name | Instrument Type | Unit | Description |
|-------|-----------------|------|-------------|
| `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. |
| `http.server.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. |

## Advanced configuration

Expand Down
12 changes: 12 additions & 0 deletions test/OpenTelemetry.Instrumentation.AspNetCore.Tests/MetricTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,18 @@ private static KeyValuePair<string, object>[] AssertMetricPoint(
Assert.Contains(route, attributes);
Assert.Equal(expectedTagsCount, attributes.Length);

// Inspect Histogram Bounds
var histogramBuckets = metricPoint.GetHistogramBuckets();
var histogramBounds = new List<double>();
foreach (var t in histogramBuckets)
{
histogramBounds.Add(t.ExplicitBound);
}

Assert.Equal(
expected: new List<double> { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10, double.PositiveInfinity },
actual: histogramBounds);

return attributes;
}
}