-
Notifications
You must be signed in to change notification settings - Fork 779
WIP: Support exemplars in WSGI and ASGI histogram #3739
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
base: main
Are you sure you want to change the base?
WIP: Support exemplars in WSGI and ASGI histogram #3739
Conversation
The committers listed above are authorized under a signed CLA. |
26d8015
to
dd03138
Compare
I recently came across this issue in the FastAPI instrumentation and tracked it down to the underlying ASGI instrumentation, though it seems to be present in WSGI and Tornado as well as mentioned in the issue. Just thought I would chip in here a bit. I see two possible solutions: Option A: defer closing the span until metrics are recordedThis would be what you have started on for WSGI and seems like the more intuitive and clean method. The span end time will become a bit longer with this method but it should be negligible. Option B: re-create span context after closing the span and pass it to metricsfinally:
...
# Create span context for exemplars
span_ctx = set_span_in_context(span)
if self.duration_histogram_old:
duration_attrs_old = _parse_duration_attrs(
req_attrs, _StabilityMode.DEFAULT, context=span_ctx,
)
... This is a more minimal and local patch but less clean. I'd be happy to chip in if you need any assistance with the PR. |
Thanks for the help! I properly linked the issue now if there's more discussion here. Like I said in the ticket, I'm not sure if the span context is the issue (unless I have misunderstood something), but let's continue discussing there 🙏 I'd love another set of eyes. |
I made a PR felix-hilden#1 with my proposed change into your branch including some tests as well. The diff is a bit large since it's based on a more recent version of the upstream repo. |
dd03138
to
dba7521
Compare
Description
I'm attempting to fix exemplar support for WSGI and ASGI applications by moving the metric recording to happen within the active span. This will eliminate the need for similar custom middleware metrics that do emit exemplars out of the box already.
Fixes #3031
Type of change
I'm not sure whether this is an omission, or something that was deliberately not developed yet. It has not been explicitly advertised, but I would certainly expect it to behave like this.
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.