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

Jump to bottom

OWIN HTTP server metrics #1335

Merged
merged 27 commits into from
Sep 20, 2023
Merged

OWIN HTTP server metrics #1335

merged 27 commits into from
Sep 20, 2023

Conversation

matt-hensley
Copy link
Contributor

@matt-hensley matt-hensley commented Aug 29, 2023 •
edited
Loading

Fixes #1306.

Changes

Adds HTTP server metrics for OWIN. There is a new MeterProviderBuilder extension method AddOwinInstrumentation that takes no options.

Currently only three attributes are published: method, scheme, and status code. These are the only attributes published by the ASP.NET instrumentation, making the two instrumentations interchangeable.

Tests were modified to cover metrics with and without an active trace.

When there is an active trace, the Activity duration is used, otherwise a separate timer is spun up.

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@matt-hensley matt-hensley requested a review from a team August 29, 2023 23:07
@Kielek Kielek mentioned this pull request Aug 30, 2023
Closed
CodeBlanch
CodeBlanch approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

Couple nits but LGTM

@utpilla utpilla added the comp:instrumentation.owin Things related to OpenTelemetry.Instrumentation.Owin label Aug 31, 2023
vishweshbankwar
vishweshbankwar approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

LGTM: Noticed that traces use the older semantic conventions. not sure if those can be updated without going through the transition phase as we are doing in httpclient and aspnetcore libraries. @CodeBlanch - Could you please confirm?

@CodeBlanch
Copy link
Member

I'm good merging as-is. @matt-hensley I think we need a follow-up effort to look at OTEL_SEMCONV_STABILITY_OPT_IN evvar and send the new tracing conventions based on the value. Interested in taking that on?

@cijothomas
Copy link
Member

LGTM. Please modify the readme file to include this change. In particular, document that Metrics can be enabled irrespective of tracing (since some other libraries have this limitation).

cijothomas
cijothomas reviewed Sep 19, 2023
View reviewed changes
test/OpenTelemetry.Instrumentation.Owin.Tests/DiagnosticsMiddlewareTests.cs

// metric value and span duration should match
// https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpserverrequestduration
Assert.Equal(activity.Duration.TotalSeconds, metricPoint.GetHistogramSum());
Copy link
Member

Choose a reason for hiding this comment

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

is there test covering the scenario where instrumentTraces = false?, and it relies on its own timestamp to do duration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests were updated to to cover metrics + traces, only traces, only metrics and neither enabled. There is not a check for the Stopwatch based duration, however there is an assertion to ensure the histogram has a value.

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

There is not a check for the Stopwatch based duration, however there is an assertion to ensure the histogram has a value

Nonblocking: It'd be good to cover that to check the actual duration in a future improvement! (the test can setup its own stopwatch, and check that the duration calculated by instrumentaion is within a reasonable range of that.)

cijothomas
cijothomas approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM. Please add to readme file. Also left a minor comment about potential test gap.

cijothomas
cijothomas reviewed Sep 19, 2023
View reviewed changes
src/OpenTelemetry.Instrumentation.Owin/README.md Outdated
Call the `AddOwinInstrumentation` `MeterProviderBuilder` extension to register
OpenTelemetry instrumentation which generates request duration metrics for OWIN requests.

The metric implemention does not rely on tracing, and will generate metrics even if tracing is disabled.
Copy link
Member

Choose a reason for hiding this comment

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

💯 love this part!

cijothomas
cijothomas reviewed Sep 19, 2023
View reviewed changes
src/OpenTelemetry.Instrumentation.Owin/README.md
using var openTelemetry = Sdk.CreateMeterProviderBuilder()
.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("Owin-Example"))
.AddOwinInstrumentation()
.AddConsoleExporter()
Copy link
Member

Choose a reason for hiding this comment

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

nit: show adding the nuget for console.

@utpilla
Copy link
Contributor

utpilla commented Sep 19, 2023

@matt-hensley The CI check for markdown is failing. Please fix that and we can merge this PR.

@utpilla utpilla merged commit 49365fc into open-telemetry:main Sep 20, 2023
20 of 21 checks passed
@Kielek
Copy link
Contributor

Kielek commented Sep 20, 2023

I'm good merging as-is. @matt-hensley I think we need a follow-up effort to look at OTEL_SEMCONV_STABILITY_OPT_IN evvar and send the new tracing conventions based on the value. Interested in taking that on?

@CodeBlanch, I do not think it is worth to invest in OTEL_SEMCONV_STABILITY_OPT_IN. IMO it will be enough to wait for ~2 more weeks and just switch all http metrics to the new convention. See warning section under https://github.com/open-telemetry/opentelemetry-specification/blob/ad1537a46eb856eeb915786fd7cf81e07bdeb426/specification/metrics/semantic_conventions/http-metrics.md

@matt-hensley matt-hensley mentioned this pull request Sep 20, 2023
Closed
6 tasks
@cijothomas
Copy link
Member

I'm good merging as-is. @matt-hensley I think we need a follow-up effort to look at OTEL_SEMCONV_STABILITY_OPT_IN evvar and send the new tracing conventions based on the value. Interested in taking that on?

@CodeBlanch, I do not think it is worth to invest in OTEL_SEMCONV_STABILITY_OPT_IN. IMO it will be enough to wait for ~2 more weeks and just switch all http metrics to the new convention. See warning section under https://github.com/open-telemetry/opentelemetry-specification/blob/ad1537a46eb856eeb915786fd7cf81e07bdeb426/specification/metrics/semantic_conventions/http-metrics.md

Agree. Since this is starting fresh and already doing the new conventions, no need of the opt-in/out behavior. It made sense for other instrumentations which were producing old metrics for quite some time.

@utpilla
Copy link
Contributor

utpilla commented Sep 20, 2023 •
edited
Loading

@Kielek @cijothomas

@CodeBlanch was referring to using the environment variable for traces and NOT metrics which this library has been producing for quite some time.

@cijothomas
Copy link
Member

Got it! My bad!

@CodeBlanch CodeBlanch mentioned this pull request Oct 2, 2023
Merged
4 tasks
@gliljas
Copy link
Contributor

gliljas commented Mar 19, 2024

This has been merged for quite some time, yet it's not available in any Nuget version.

@utpilla
Copy link
Contributor

utpilla commented Mar 19, 2024

This has been merged for quite some time, yet it's not available in any Nuget version.

@gliljas Could you please create a PR for releasing a new version of the package? Check this for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utpilla utpilla utpilla approved these changes

@pyohannes pyohannes pyohannes left review comments

@cijothomas cijothomas cijothomas approved these changes

@vishweshbankwar vishweshbankwar vishweshbankwar approved these changes

@CodeBlanch CodeBlanch Awaiting requested review from CodeBlanch

Assignees

@CodeBlanch CodeBlanch

Labels
comp:instrumentation.owin Things related to OpenTelemetry.Instrumentation.Owin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add metrics support for Owin
8 participants
@matt-hensley @CodeBlanch @cijothomas @utpilla @Kielek @gliljas @pyohannes @vishweshbankwar