-
Notifications
You must be signed in to change notification settings - Fork 764
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
[ASP.NET Core] Remove suppression for metrics generated from prometheus #5044
[ASP.NET Core] Remove suppression for metrics generated from prometheus #5044
Conversation
…/github.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/remove-prometheus-special-handling
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5044 +/- ##
==========================================
+ Coverage 83.34% 83.58% +0.24%
==========================================
Files 296 296
Lines 12475 12473 -2
==========================================
+ Hits 10397 10426 +29
+ Misses 2078 2047 -31
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -35,6 +35,11 @@ exception. The attribute value will be set to full name of exception type. | |||
* Fixed `network.protocol.version` attribute values to match the specification. | |||
([#5007](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5007)) | |||
|
|||
* Removed filtering of metrics when the request path contained `/metrics`. This | |||
change may affect prometheus pull scenario if the prometheus server sends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably nit picking, but the wording "may affect" may not be clear as to what is the effect? The effect of this PR is - metrics now include measurements from prometheus scrape requests. So one possible effect is - users see more count and their avge, etc might be affected. Eg: the service's latency is one-digit-ms only, but prometheus scrapes take seconds.. Now, by including prometheus scrapes to the latency metric, it may give false impression that the service slowed! It could be immaterial, if prom scrapes are a minute fraction (quite likely.)
Not a blocker for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded first line a bit more. The point about latency is valid but I don't think it falls under the instrumentation library's scope (library simply reports metrics). I think it was a bug that we were filtering measurements and not giving the full picture.
…/github.com/vishweshbankwar/opentelemetry-dotnet into vibankwa/remove-prometheus-special-handling
Fixes #
Design discussion issue #
Changes
Remove the special handling of metrics generated as a result of metrics pull from Prometheus endpoint. Any filtering (if at all) should be handled by generic filtering mechanism (if one is available in future).
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes