-
Notifications
You must be signed in to change notification settings - Fork 544
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
[net/http] add http.server.active_requests metric #4543
base: main
Are you sure you want to change the base?
Conversation
I suppose we could add a comment somewhere about the fact that only metrics recorded after the handler is called are modifiable using a |
➕ |
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.
Can you please
- Cover the new metric in tests.
- Add a changelog entry.
Please resolve the conflicts. Otherwise I cannot even run the GitHub workflows. |
8a917ed
to
4be1478
Compare
Tried hard to add the route tag to the |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4543 +/- ##
=====================================
Coverage 80.8% 80.8%
=====================================
Files 150 150
Lines 10371 10361 -10
=====================================
- Hits 8387 8380 -7
+ Misses 1840 1835 -5
- Partials 144 146 +2
|
Anything else you need me to do on this 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.
- There is a vulnerability in the code
- The build fails
@@ -226,6 +244,10 @@ func (h *middleware) serveHTTP(w http.ResponseWriter, r *http.Request, next http | |||
labeler := &Labeler{} | |||
ctx = injectLabeler(ctx, labeler) | |||
|
|||
attrs := metric.WithAttributes(httpSchemeFromRequest(r), semconv.HTTPMethod(r.Method)) |
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.
This line would bring back the vulnerability caused by unbound cardinality metric attribute (HTTP method) GHSA-rcjv-mgp8-qvmr
It is OK to not add it. Take notice that this attribute is not specified in v1.20.0 (https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/metrics/semantic_conventions/http-metrics.md) nor in the latest (https://opentelemetry.io/docs/specs/semconv/http/http-metrics/#metric-httpserveractive_requests) |
Follow up of #2617. As @Aneurysm9 pointed out the
Labeler
can only be used after the next handler has been called. This means that for active requests it is impossible to inject attributes this way.