HttpSemanticConventions - new metric in AspNetCore Instrumentation. #4802

TimothyMothra · 2023-08-24T21:49:54Z

Towards #4484
Supersedes #4766

This PR focus on introducing the new http.server.request.duration metrics in the AspNetCore Instrumentation library.

Changes

all the following changes are made behind the environment variable OTEL_SEMCONV_STABILITY_OPT_IN
update Instrumentation.AspNetCore class HttpInMetricsListener to emit new metric.
- http.server.duration (milliseconds) replaced with http.server.request.duration (~~seconds~~ ~~milliseconds~~ seconds)
  This is defined in the as-of-yet unreleased v1.22.0 https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md
remove opt-in attributes
- server.port
- server.address

Future work

~~The unit of this metric needs to be changed to seconds. This requires a different default histogram.~~ DONE!

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

codecov · 2023-08-24T22:05:24Z

Codecov Report

Merging #4802 (0015a20) into main (680115f) will increase coverage by 0.00%.
The diff coverage is 68.18%.

@@           Coverage Diff           @@
##             main    #4802   +/-   ##
=======================================
  Coverage   83.90%   83.90%           
=======================================
  Files         293      293           
  Lines       12001    12028   +27     
=======================================
+ Hits        10069    10092   +23     
- Misses       1932     1936    +4

Files Changed	Coverage
...AspNetCore/Implementation/HttpInMetricsListener.cs	`68.18%`

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

utpilla · 2023-09-13T02:39:58Z

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

-| Name  | Instrument Type | Unit | Description |
-|-------|-----------------|------|-------------|
-| `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. |
+Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`?


Suggested change

Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`?

Have you opted in to the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`?

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

utpilla · 2023-09-13T02:46:35Z

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

+    | `http.server.request.duration` | Histogram | `s` | Measures the duration of inbound HTTP requests. |
+
+    This metric is emitted in `seconds` as per the semantic convention. While
+    the convention [recommends using custom histogram buckets](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration)


Use the commit tag?

I'm conflicted here.
I would rather link to an explicit tag. There changes we're implementing now are post v1.21.0, but this repo hasn't made a new release yet.

I can change links in both the Readme and Changelog to a commit as you've suggested.
When the next release happens (presumably v1.22.0) I think we should update these links.

utpilla · 2023-09-13T02:46:49Z

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

+  into the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN`
+  environment variable. This metric measures time in seconds and offers unique
+  histogram buckets as per the
+  [spec](https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration)


Same as https://github.com/open-telemetry/opentelemetry-dotnet/pull/4802/files#r1323869142

utpilla

Left a few suggestions for CHANGELOG and README.

cijothomas · 2023-09-13T14:46:26Z

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

+* Introduced a new metric, `http.server.request.duration`, for users who opt
+  into the new semantic convention by configuring the `OTEL_SEMCONV_STABILITY_OPT_IN`
+  environment variable. This metric measures time in seconds and offers unique
+  histogram buckets as per the


This metric measures time in seconds and offers unique
histogram buckets

Not sure how to interpret "offers unique histogram buckets"? What does it mean?

"offers" is the wrong word here, "uses" would be more appropriate.
I'm trying to call attention to the fact that the buckets are different than the default.
4 lines below this I list the exact buckets.

I'm trying to find a succinct way to describe that this metric is different than what came before.
@cijothomas, do you think this would be better as a bulleted list?

ie:

- new metric: `http.server.request.duration` - unit: seconds - buckets: 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

the list is good. Hint that the buckets are so, because SDK specially handles this metric.

cijothomas · 2023-09-13T15:01:35Z

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

-| `http.server.duration` | Histogram | `ms` | Measures the duration of inbound HTTP requests. |
+Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`?
+
+* If yes, the instrumentation supports the following metric.


this is not clear. The env variable can take none,http,http/dup as values, This doc should also be structured in that way. i.e what if the env variable is set to "none", (also the default)
what is the env varaible is set to "http"
what is the env varaible is set to "http/dup" - both metric are emitted

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

TimothyMothra · 2023-09-13T22:27:49Z

@utpilla, @cijothomas, @vishweshbankwar
I've rewritten both the Readme and Changelog. Please re-review.

cijothomas · 2023-09-14T14:44:51Z

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md

@@ -2,6 +2,26 @@

 ## Unreleased

+* Introduced a new metric, `http.server.request.duration`, for users who opt-in


Introduced a new metric, http.server.request.duration, for users who opt-in
to the new semantic convention by configuring the OTEL_SEMCONV_STABILITY_OPT_IN
environment variable to value "fillthis". This metric measures time in seconds, but histogram buckets are adjusted
automatically for this metric by OTel SDK which specially treat this metric. This metric follows the OTel Semantic Conventions from (add link).

Metric Name: http.server.request.duration

Unit: seconds

Histogram Buckets: 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 (due to OpenTelemetry SDK's special treatment)

Replaces old metric: http.server.duration

Unit: miliseconds

Histogram Buckets: 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000 (the general default of Histogram buckets)

OTEL_SEMCONV_STABILITY_OPT_IN may be set to "fillthis", to help an easier transition, at the cost of emitting both old and new metrics.

^did some refactoring. Feel free to take it with modifications.
A user who do not have any experience with OTel sem. conventions and the new vs old, should find it easy to follow the changelog. Key things I'd prefer to convey here are:

No breaking changes if you just update to this new version.

Opt-in feature to emit a new metric that follows a new convention, which despite using "s", still allows good histogram precision due to SDK treating it specially.

Hint at emitting both to smooth transition.

Hint that the new metric is the long term bet, as it'll be the one getting stabilizied by Otel sem convention.

Thanks for the refactor. I'll take another pass at this today.

Do we typically put instructions in the changelog? I was trying to keep the changelog succinct, and use the readme for the full information. The challenge is that this change has a LOT of details :)

just pushed my rewrite. I think I've addressed all your points. please re-review.

…entelemetry-dotnet into 4484_newMetrics

src/OpenTelemetry.Instrumentation.AspNetCore/README.md

…entelemetry-dotnet into 4484_newMetrics

utpilla · 2023-09-14T23:06:47Z

@TimothyMothra Thanks for your patience on working on this PR!

TimothyMothra · 2023-09-14T23:14:27Z

@TimothyMothra Thanks for your patience on working on this PR!

This one was tricky and touched a lot of parts! Glad we could get this merged.

TimothyMothra added 4 commits August 24, 2023 10:19

update AspNetCore for new metric 'http.server.request.duration'

91f08c8

cleanup

c111eea

remove attributes

32d8946

comment

3208431

readme

d2f7ef4

TimothyMothra changed the title ~~HttpSemanticConventions - new metric is AspNetCore Instrumentation~~ HttpSemanticConventions - new metric in AspNetCore Instrumentation. Aug 24, 2023

comment

1a49dab

TimothyMothra marked this pull request as ready for review August 24, 2023 22:45

TimothyMothra requested a review from a team as a code owner August 24, 2023 22:45

TimothyMothra added 2 commits August 24, 2023 15:48

update readme

7c11551

cleanup

93fe6ad

vishweshbankwar reviewed Aug 25, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs Outdated Show resolved Hide resolved

cijothomas reviewed Aug 25, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs Outdated Show resolved Hide resolved

TimothyMothra added 2 commits August 25, 2023 09:40

check flag in ctor

3842aa8

Merge branch 'main' into 4484_newMetrics

ec2b825

trask mentioned this pull request Aug 28, 2023

Emit both old and new metrics under dup setting open-telemetry/opentelemetry-java-instrumentation#9320

Merged

vishweshbankwar reviewed Aug 29, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs Outdated Show resolved Hide resolved

TimothyMothra added 10 commits August 29, 2023 14:23

update tags on metrics

5f74488

refactor

9fb75e5

Merge branch 'main' into 4484_newMetrics

c335f65

fix name in log

6c86a4a

another fix

3bcefae

more fixes

e588453

big refactor, split Old and New into two methods

88355cd

investigating test fix

6a3f651

final fix for test issue

1a72356

Merge branch 'main' into 4484_newMetrics

b9d194c

vishweshbankwar mentioned this pull request Sep 5, 2023

[sdk] Hardcode known histograms using seconds #4820

Merged

4 tasks

vishweshbankwar reviewed Sep 5, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/README.md Outdated Show resolved Hide resolved

utpilla reviewed Sep 13, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md Outdated Show resolved Hide resolved

utpilla reviewed Sep 13, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/README.md Show resolved Hide resolved

utpilla reviewed Sep 13, 2023

View reviewed changes

utpilla approved these changes Sep 13, 2023

View reviewed changes

cijothomas reviewed Sep 13, 2023

View reviewed changes

rajkumar-rangaraj reviewed Sep 13, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md Outdated Show resolved Hide resolved

rajkumar-rangaraj approved these changes Sep 13, 2023

View reviewed changes

TimothyMothra added 2 commits September 13, 2023 15:21

rewrite changelog

0b257af

rewrite Readme

4987540

TimothyMothra added 2 commits September 13, 2023 16:17

markdownlint

0cfa424

Merge branch 'main' into 4484_newMetrics

163d4d7

cijothomas reviewed Sep 14, 2023

View reviewed changes

TimothyMothra added 5 commits September 14, 2023 13:49

rewrite changelog

80df848

Merge branch '4484_newMetrics' of https://github.com/TimothyMothra/op…

9f83898

…entelemetry-dotnet into 4484_newMetrics

edit changelog & readme

744a980

update

1d694bc

Merge branch 'main' into 4484_newMetrics

390d563

utpilla reviewed Sep 14, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.AspNetCore/README.md Outdated Show resolved Hide resolved

TimothyMothra added 2 commits September 14, 2023 15:37

Merge branch '4484_newMetrics' of https://github.com/TimothyMothra/op…

6d4bb65

…entelemetry-dotnet into 4484_newMetrics

feedback

0015a20

utpilla approved these changes Sep 14, 2023

View reviewed changes

utpilla merged commit 9139b72 into open-telemetry:main Sep 14, 2023
50 checks passed

TimothyMothra deleted the 4484_newMetrics branch September 14, 2023 23:14

karataliu mentioned this pull request Oct 17, 2023

OTEL_SEMCONV_STABILITY_OPT_IN=http doesn't enable new metric convention #4951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpSemanticConventions - new metric in AspNetCore Instrumentation. #4802

HttpSemanticConventions - new metric in AspNetCore Instrumentation. #4802

TimothyMothra commented Aug 24, 2023 •

edited

codecov bot commented Aug 24, 2023 •

edited

utpilla Sep 13, 2023

utpilla Sep 13, 2023

TimothyMothra Sep 13, 2023

utpilla Sep 13, 2023

utpilla left a comment

cijothomas Sep 13, 2023

TimothyMothra Sep 13, 2023

cijothomas Sep 13, 2023

cijothomas Sep 13, 2023

TimothyMothra commented Sep 13, 2023

cijothomas Sep 14, 2023

TimothyMothra Sep 14, 2023

TimothyMothra Sep 14, 2023

utpilla commented Sep 14, 2023

TimothyMothra commented Sep 14, 2023

	Have you opt-ed into the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`?
	Have you opted in to the new Http Semantic Conventions using `OTEL_SEMCONV_STABILITY_OPT_IN`?

		@@ -2,6 +2,26 @@

		## Unreleased

		* Introduced a new metric, `http.server.request.duration`, for users who opt-in

HttpSemanticConventions - new metric in AspNetCore Instrumentation. #4802

HttpSemanticConventions - new metric in AspNetCore Instrumentation. #4802

Conversation

TimothyMothra commented Aug 24, 2023 • edited

Changes

Future work

Merge requirement checklist

codecov bot commented Aug 24, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyMothra commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla commented Sep 14, 2023

TimothyMothra commented Sep 14, 2023

TimothyMothra commented Aug 24, 2023 •

edited

codecov bot commented Aug 24, 2023 •

edited