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.

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

[Instrumentation.AspNet] Update semantic conventions for metrics #1429

Merged
merged 21 commits into from
Dec 12, 2023

Conversation

qhris
Copy link
Contributor

@qhris qhris commented Nov 8, 2023

Updates metrics to match semantic conventions for 1.23.

This was previously discussed in #1407.

Changes

Replaces http.server.duration with the new http.request.server.duration metric and adds missing attributes.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes

Remaining issues

  • Unable to get Application_Error to fire in my sample application in order to verify error.type for exceptions. It works fine for status codes. Excluded for now.
  • Unable to write a test for network.protocol.version which uses request.ServerVariables["SERVER_PROTOCOL"] because that property/class is heavily locked down. I have verified this to work on a sample application.
  • The HttpRequestRouteHelper works fine for fetching the route template but it is marginally useful because the route easily ends up like /{controller}/{action} which doesn't say much.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #1429 (3dd0fae) into main (71655ce) will increase coverage by 0.16%.
Report is 93 commits behind head on main.
The diff coverage is 54.44%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1429      +/-   ##
==========================================
+ Coverage   73.91%   74.07%   +0.16%     
==========================================
  Files         267      264       -3     
  Lines        9615     9794     +179     
==========================================
+ Hits         7107     7255     +148     
- Misses       2508     2539      +31     
Flag Coverage Δ
unittests-Exporter.Geneva 57.92% <32.14%> (?)
unittests-Exporter.OneCollector 89.72% <100.00%> (?)
unittests-Extensions 82.75% <100.00%> (?)
unittests-Instrumentation.AspNet 75.96% <ø> (?)
unittests-Instrumentation.EventCounters 75.92% <ø> (?)
unittests-Instrumentation.Owin 85.71% <ø> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)
unittests-Instrumentation.Runtime 100.00% <ø> (?)
unittests-Instrumentation.StackExchangeRedis 71.00% <ø> (?)
unittests-Instrumentation.Wcf 78.47% <ø> (?)
unittests-PersistentStorage 58.80% <ø> (?)
unittests-ResourceDetectors.Azure 80.90% <ø> (?)
unittests-ResourceDetectors.ProcessRuntime 76.08% <ø> (?)
unittests-Solution 81.83% <90.62%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
....Exporter.Geneva/GenevaExporterHelperExtensions.cs 100.00% <ø> (+31.81%) ⬆️
...Telemetry.Exporter.Geneva/GenevaExporterOptions.cs 90.00% <ø> (ø)
...OpenTelemetry.Exporter.Geneva/GenevaLogExporter.cs 85.00% <ø> (+7.50%) ⬆️
...lemetry.Exporter.Geneva/GenevaLoggingExtensions.cs 100.00% <ø> (+14.28%) ⬆️
...enTelemetry.Exporter.Geneva/GenevaTraceExporter.cs 82.50% <ø> (+7.50%) ⬆️
...xporter.Geneva/Internal/ConnectionStringBuilder.cs 92.94% <100.00%> (ø)
...ry.Exporter.Geneva/Internal/ExporterEventSource.cs 14.28% <ø> (+9.52%) ⬆️
...eneva/Internal/ReentrantActivityExportProcessor.cs 100.00% <ø> (ø)
...porter.Geneva/Internal/ReentrantExportProcessor.cs 100.00% <ø> (ø)
...ry.Exporter.Geneva/Internal/TableNameSerializer.cs 98.93% <ø> (ø)
... and 104 more

... and 165 files with indirect coverage changes

@qhris
Copy link
Contributor Author

qhris commented Nov 8, 2023

Hey,

I'm leaving this up as a draft until I can finish up the remaining tasks and run some more tests.
I could however use some input from @Kielek and @vishweshbankwar for the direction of this PR and the remaining tasks?

@Kielek Kielek added the comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet label Nov 9, 2023
@vishweshbankwar
Copy link
Member

Hey,

I'm leaving this up as a draft until I can finish up the remaining tasks and run some more tests. I could however use some input from @Kielek and @vishweshbankwar for the direction of this PR and the remaining tasks?

@qhris

  1. Let's handle error.type outside of this PR. It would be tricky to get that value on metrics side. Take a look at [ASP.NET Core] Add error.type attribute for tracing and metrics opentelemetry-dotnet#4986 if you can do something similar here.
  2. I see the test creates HttpContext.Current. Is it possible to do HttpContext.Current.Request.ServerVariables.Add and validate that you get the network.protocol.version?
  3. For http.route, this is a known issue. Please open a separate issue for this if there isn't one out there already. For now, you can copy how tracing is doing it.

@qhris
Copy link
Contributor Author

qhris commented Nov 10, 2023

@vishweshbankwar thanks for the input!

  1. Thanks for the link, I can look into this a bit more. Are you suggesting to remove it entirely for this PR if not?
  2. A tried a variety of things to get it working.
    • ServerVariables.Add() throw with System.NotSupportedException.
    • ServerVariables only has a getter so I can't assign to it.
    • I also tried setting the backing field _serverVariables through reflection but ended up running into an issue since the type HttpServerVarsCollection is internal to System.Web. I could try to instantiate that type through reflection as well but I didn't feel comfortable with that as it would get very messy. Might be the only way to go though.
    • I tried making HttpRequest a mock but that throws: System.NotSupportedException : Type to mock (HttpRequest) must be an interface, a delegate, or a non-sealed, non-static class.
    • It seems using HttpContextBase is the recommended way to make the code more testable but that would require making quite a bigger change to the codebase (i.e. rewriting HttpTelemetryModule, ActivityHelper, et al.).
  3. Sounds good 👍

@qhris qhris changed the title [Instrumentation.AspNet] Use semantic conventions for metrics [Instrumentation.AspNet] Update semantic conventions for metrics Nov 10, 2023
@vishweshbankwar
Copy link
Member

@vishweshbankwar thanks for the input!

  1. Thanks for the link, I can look into this a bit more. Are you suggesting to remove it entirely for this PR if not?

  2. A tried a variety of things to get it working.

    • ServerVariables.Add() throw with System.NotSupportedException.
    • ServerVariables only has a getter so I can't assign to it.
    • I also tried setting the backing field _serverVariables through reflection but ended up running into an issue since the type HttpServerVarsCollection is internal to System.Web. I could try to instantiate that type through reflection as well but I didn't feel comfortable with that as it would get very messy. Might be the only way to go though.
    • I tried making HttpRequest a mock but that throws: System.NotSupportedException : Type to mock (HttpRequest) must be an interface, a delegate, or a non-sealed, non-static class.
    • It seems using HttpContextBase is the recommended way to make the code more testable but that would require making quite a bigger change to the codebase (i.e. rewriting HttpTelemetryModule, ActivityHelper, et al.).
  3. Sounds good 👍

Yes - A separate PR for error.type.

For network.protocol.version, You could remove this one too for now in favor of making progress. I will try to take a look at it when I get chance.

@qhris
Copy link
Contributor Author

qhris commented Nov 13, 2023

Hey, I made some additional findings here.

The reason why I wasn't able to get Application_Error to fire was because I was testing a ASP Web API application. This type of app has a completely different error flow as desribed in this Microsoft documentation.

This means that traces do not behave as expected either, since the error status of the span will never be set here.

For now, I went ahead and removed the error.type as suggested.

Would it be a blocker to leave in the protocol version as is? It is a very useful metric and it would be a shame to leave it out.

Edit: It would appear this line makes the error span be set correctly even though the exception path is never fired.

@qhris qhris marked this pull request as ready for review November 13, 2023 21:24
@qhris qhris requested a review from a team November 13, 2023 21:24
@vishweshbankwar
Copy link
Member

Would it be a blocker to leave in the protocol version as is

No, I think it is ok.

@Kielek
Copy link
Contributor

Kielek commented Nov 14, 2023

@qhris, I think that. semantic convention also for spans/traces has to be adjusted. It can be done in separate PR, but it should be done before the release to make the transition from unstable to stable world only once.

@carlos-neto-maersk
Copy link

carlos-neto-maersk commented Nov 14, 2023

Can you guys please check #1431 ?

It is related to this, because due to the changes done to telemetry module to have the activity stopped on this OnActivityStoppedCallback, under load we are experiencing that weird pattern (and I think that it is because it because it is closing activities that aren't to be closed in other OnActivityStoppedCallback handlers).

I'm working on not closing the activity after the handlers, and using the current timestamp to figure it out the duration instead of using Activity.Duration, but the code isn't great.

And thank you so much for your great work.

@github-actions github-actions bot added the Stale label Nov 26, 2023
@Kielek Kielek removed the Stale label Nov 28, 2023
Copy link
Contributor

github-actions bot commented Dec 6, 2023

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Dec 6, 2023
@Kielek Kielek removed the Stale label Dec 6, 2023
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

@cijothomas, can we merge it with current wording?

@cijothomas
Copy link
Member

@cijothomas, can we merge it with current wording?

Since we know the exact version of the SDK which is required to get correct histogram buckets, I'd suggest to call it out in the changelog.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/CHANGELOG.md#170-rc1

@qhris
Copy link
Contributor Author

qhris commented Dec 9, 2023

Fixed all the remaining issues but the tests won't pass until the SDK is updated.

@utpilla
Copy link
Contributor

utpilla commented Dec 12, 2023

@qhris Thanks for your patience!

@utpilla utpilla merged commit 3b0c472 into open-telemetry:main Dec 12, 2023
97 of 98 checks passed
@qhris qhris deleted the features/asp-metric-attributes branch December 19, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants