fix(flask): align http.server.active_requests metric with semconv helper#4094
fix(flask): align http.server.active_requests metric with semconv helper#4094rite7sh wants to merge 17 commits intoopen-telemetry:mainfrom
Conversation
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.
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
|
@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. |
There was a problem hiding this comment.
The issue here is that the two instrumentor methods we have in this instrumentation use two different semconv for the same metric: one as old as the default implementation and one that does follow the stable one. The PR is changing the older one to use the stable semconv too so both in the default older semconv and when output of the stable one is configured. That's a behaviour change. Since the difference is only on unit and description I'm not sure that's a big deal.
|
@xrmx Thanks for the clarification, I agree this is a small behavior change limited to unit and description, and I’m glad it aligns both paths on the same semconv, I’ve updated the branch and am happy to tweak anything if needed. :) |
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
…lemetry/instrumentation/flask/__init__.py
| active_requests_counter = meter.create_up_down_counter( | ||
| name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS, | ||
| unit="requests", | ||
| description="measures the number of concurrent HTTP requests that are currently in-flight", | ||
| ) | ||
| active_requests_counter = create_http_server_active_requests(meter) |
There was a problem hiding this comment.
Can we do something like
if newsemconv:
active_requests_counter = create_http_server_active_requests(meter)
else:
active_requests_counter = meter.create_up_down_counter(
name=MetricInstruments.HTTP_SERVER_ACTIVE_REQUESTS,
unit="requests",
description="measures the number of concurrent HTTP requests that are currently in-flight",
)
in both hunks so that the metrics matches the semconv?
There was a problem hiding this comment.
@xrmx I’ve updated both paths to use the explicit semconv split as suggested. Let me know if this looks good now, thanks for suggestion.
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: