Capture http.server.duration metric for ASP.NET Core instrumentation#1347
Capture http.server.duration metric for ASP.NET Core instrumentation#1347alanwest wants to merge 22 commits intoopen-telemetry:metricsfrom
Conversation
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## metrics #1347 +/- ##
===========================================
- Coverage 84.61% 82.40% -2.21%
===========================================
Files 245 232 -13
Lines 6974 6156 -818
===========================================
- Hits 5901 5073 -828
- Misses 1073 1083 +10
|
| } | ||
|
|
||
| // TODO: Currently these options are unused for metrics. | ||
| // Do we need separate options for metrics, or will the same options used for traces make sense for metrics? |
There was a problem hiding this comment.
I think having one Options across traces/logs/metrics is more friendly for the user.
FYI - I think some of the exporters will move towards that direction (e.g. a single ConsoleExporter targeting logs/traces/metrics)
There was a problem hiding this comment.
What are some of the possible things which can be configured in metricOptions? Do we need to allow user to specify labels? Or we always get all the labels as per semantic convention, and users can configure metric Views to drop any labels they dont care.
There was a problem hiding this comment.
There's nothing I'm aware of currently in the spec regarding the ability to configure labels to include/exclude. The concept of metric views is still in a proposed state open-telemetry/oteps#89.
For the purpose of this work, it may just make sense to remove the ability to specify options for now. That is, the AspNetCoreInstrumentationOptions currently has options that are not really yet applicable in the context of metrics. I think the one thing that makes sense in the Filter delegate to suppress capturing metrics based on certain criteria.
src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInMetricsListener.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| // TODO: Prometheus pulls metrics by invoking the /metrics endpoint. Decide if it makes sense to suppress this. | ||
| // Below is just a temporary way of achieving this suppression for metrics (we should consider suppressing traces too). |
There was a problem hiding this comment.
yes, eventually we can leverage the SupressInstrumentation API for this.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
Overall LGTM.
We may want to do benchmark about Ds Subsciption before finalizing this approach.
3 participants
First steps in addressing #1269.
This PR captures the
http.server.durationmetric for ASP.NET Core.Opening as draft for now as there are a number of TODOs peppered through the code. A few more TODOs not called out in the code: