Add Labeler and instrumentation integration for custom attribute addition to HTTP request duration metrics (Flask, Django, Falcon, WSGI, ASGI)#3689
Conversation
tammy-baylis-swi
force-pushed
the
add-metrics-attributes-labeler
branch
from
d676f55 to
6cec2f1
Compare
tammy-baylis-swi
force-pushed
the
add-metrics-attributes-labeler
branch
from
6cec2f1 to
ce12b1b
Compare
tammy-baylis-swi
changed the title
tammy-baylis-swi
changed the title
tammy-baylis-swi
moved this from Ready for review
to Reviewed PRs that need fixes
in @xrmx's Python PR digest
tammy-baylis-swi
moved this from Reviewed PRs that need fixes
to Ready for review
in @xrmx's Python PR digest
|
@open-telemetry/opentelemetry-python-contrib-approvers ptal, thanks. |
Hi @emdneto could I have another look, please |
| @@ -0,0 +1,235 @@ | |||
| # Copyright The OpenTelemetry Authors | |||
There was a problem hiding this comment.
can we add the implementation in the typecheck?
There was a problem hiding this comment.
Sure! Added in bac83e2. I had to tweak some Labeler-internal return types to pass.
| req_attrs, _StabilityMode.HTTP | ||
| ) | ||
| # Enhance attributes with any custom labeler attributes | ||
| duration_attrs_new = enrich_metric_attributes( |
There was a problem hiding this comment.
i'm wondering what we should do here in case user overrides semconv attributes. can we test that?
There was a problem hiding this comment.
Yes! Added Labeler unit tests, and WSGI/ASGI unit tests in 12ef547
| returns a copy of the original base attributes. | ||
| """ | ||
| if not enrich_enabled: | ||
| return base_attributes.copy() |
There was a problem hiding this comment.
since the hot path will be this, can't we just return base_attributes and update the typing to Mapping[str, Any]?
There was a problem hiding this comment.
I think with this PR it would be fine, but I think it's less brittle to keep "always copy" moving forward when the instrumentors' parse/record calls are so order-dependent.
| _labeler_context.set(None) | ||
|
|
||
|
|
||
| def get_labeler_attributes() -> Dict[str, Union[str, int, float, bool]]: |
There was a problem hiding this comment.
we probably need to update typing here
2 participants
Description
Adds new a Python instrumentation Labeler, inspired by Go's
net/httpinstrumentation: open-telemetry/opentelemetry-go-contrib#306. It stores KVs added to it in the current request context. Theadd/add_attributescan be called by user's instrumented code manually, or by custom distros. The following instrumentors have been updated in this PR so that custom attributes are read from the Labeler in context, then merged into the other attributes (new or old semconv) that are passed to therecordcalls of the request duration histograms:opentelemetry-instrumentation-flaskopentelemetry-instrumentation-djangoopentelemetry-instrumentation-falconopentelemetry-instrumentation-wsgiopentelemetry-instrumentation-asgiExample OTel Collector debug metrics from ASGI OpenTelemetryMiddleware instrumentation, with histograms including custom attributes: https://gist.github.com/tammy-baylis-swi/974ec71d5fb9e88968327b175f2b59a5
Addresses #3702, #3703
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.