fix(flask): align http.server.active_requests metric with semconv helper #4094
Conversation
JWinermaSplunk
left a comment
JWinermaSplunk
left a comment
There was a problem hiding this comment.
Left a small comment, but it looks simple enough if the helper functions suffice. Also, this change probably requires a changelog entry, otherwise LGTM.
| if _report_old(_InstrumentedFlask._sem_conv_opt_in_mode): | ||
| duration_histogram_old = meter.create_histogram( | ||
| name=MetricInstruments.HTTP_SERVER_DURATION, | ||
| name="http.server.duration", |
There was a problem hiding this comment.
Are we sure we want the names here and line #815 to be hardcoded and not a constant?
There was a problem hiding this comment.
oh i am sorry, I can switch this to use a constant for consistency if one is available. I’ll check the existing semconv metrics helpers and update accordingly so we’re not introducing a new hardcoded string.
There was a problem hiding this comment.
I am probably missing something but why are we changing the name value from MetricInstruments.HTTP_SERVER_DURATION at all?
There was a problem hiding this comment.
@JWinermaSplunk sorry if that wasn’t clear from the diff.
The intent here isn’t to change the metric name or semantics. The legacy metric remains http.server.duration.
This follow-up was only to avoid introducing a new hardcoded string and instead make that legacy name explicit via a local constant, similar to how other legacy/compat paths are handled.
If you’d prefer, I’m also happy to keep using MetricInstruments.HTTP_SERVER_DURATION directly here I mainly wanted to avoid adding another literal string while keeping behavior unchanged.
hopefully i was able to explain the intent :)
There was a problem hiding this comment.
Yes, let's go ahead and keep using MetricInstruments.HTTP_SERVER_DURATION in the meantime. I would be happy to approve the PR after that :)
There was a problem hiding this comment.
@JWinermaSplunk I’ve rolled back to using MetricInstruments.HTTP_SERVER_DURATION as suggested and updated the PR accordingly.
Everything should be good to go now, but please feel free to ping me if you’d like any further tweaks before approval.
Thanks for these suggestions btw :P
|
@JWinermaSplunk I’ve pushed a small follow-up commit that replaces the hardcoded legacy metric name with a constant, keeping it consistent with the existing semantic convention helpers. |
|
A changelog entry never hurts. It is simple enough and even the current PR name would be fine as an entry, but it isn't up to me in the end :) |
|
@JWinermaSplunk you got a point, i surely will add a changelog update. |
2 participants
Description
This PR fixes inconsistent initialization of the
http.server.active_requestsmetric in Flask instrumentation by aligning it with the semantic conventions
helper used by other instrumentations.
Depending on how Flask is instrumented, different units and descriptions were
being used for the same metric. This change ensures consistent behavior by
using the semconv helper everywhere.
Fixes #4093
Type of change
How Has This Been Tested?
python -m ruff check instrumentation/opentelemetry-instrumentation-flaskDoes This PR Require a Core Repo Change?
Checklist: