Fix exemplar support

[fcollonval]

Describe your environment

OS: Debian 12
Python version: Any
Package version: 0.49b0 and later

What happened?

Since 1.28.0, the opentelemetry Python SDK supports exemplars. However some instrumentation libraries does not add exemplars because the measure are added/recorded outside of the active span.

For example, in the asgi instrumentation, all measures should be taken within the use_span context rather than outside:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

Lines 707 to 709 in ae13079

	self.active_requests_counter.add(1, active_requests_count_attrs)
	try:
	with trace.use_span(span, end_on_exit=False) as current_span:

Steps to Reproduce

You can test the repository https://github.com/webscit/opentelemetry-demo-python with OTEL_AUTO_INSTRUMENTATION_LEVEL = 1, it uses fastapi instrumentation (but the flaw is actually in the asgi instrumentation).

Expected Result

Exemplars are recorded for all Python instrumentations

Actual Result

Some instrumentations are missing exemplars

Additional context

I don't know if all packages or only a subset of them are concerned by this. I have at least identified 3 cases:

Package	Line to blame
asgi	opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py Line 709 in ae13079 with trace.use_span(span, end_on_exit=False) as current_span:
wsgi	opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Line 664 in ae13079 with trace.use_span(span):
tornado	opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py Line 388 in ae13079 ctx = _start_span(tracer, handler)

Would you like to implement a fix?

None

[felix-hilden]

Hi! I'm here to second this issue and express an interest to potentially contribute with some guidance. I hit this with FastAPI, and currently I have to mirror the ASGI request duration metric as a middleware.

I also tried to poke around in the code and tests, and I found that at least for the ASGI instrumentation, there is an active span context right when the metric is recorded. So I'm not quite sure what leaves the exemplars out. For WSGI the context might not be there though, and can't speak of Tornado. If someone could point me at the proper places or documentation to look, I'd be happy to try to remedy this for ASGI/WSGI.

[Chromatius]

I ran into this issue as well for FastAPI instrumentation, and FastAPI inherits the issue from the ASGI instrumentation.

I have played around with two solutions in my local installation and test project and validated that both works for getting the exemplars added. Presumably WSGI and Tornado have similar root causes but I have not looked at those in detail.

Option A: defer closing the span until after metrics are calculated

Currently the span is ended here in _get_otel_send prior to calculating the metrics:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

Line 957 in ae13079

server_span.end()

By removing this and just relying on ending the span here:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

Line 804 in ae13079

span.end()

The exemplars are added. My best guess is that the current implementation looks like it does because the span timestamps will be more correct since they do not include metric calculation and cleanup time.

Option B: close span early and pass context explicitly to metrics

This would look like this:

finally:
    ...
    # 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 allows for more correct span timestamps but is a bit less intuitive and less clean.

[felix-hilden]

Nice! Personally I'd be happy with either, but maybe some OTEL-initiated people will have stronger opinions 👀

[tammy-baylis-swi]

Hi again, I also commented on linked PR #3739

I put in a similar issue for FlaskInstrumentor exemplars (#3913, PR #3912) which generates its own metrics independently of Wsgi/AsgiInstrumentors. The issue in that case seems to be that the metrics SDK implementation depends on its received OTel context being valid with a real span context inside it -- see ExemplarBucket.offer -- while some values like request duration aren't known until spans are complete and no longer in the current context. I think the Wsgi/AsgiInstrumentor issue is the same.

I agree the current draft fix is not the cleanest, but I think it's the fastest without overhauling the design of each Instrumentor.

I wanted to also add that I tested http.server.request.duration exemplars generation manually and found they're missing with these instrumentors (full list for clarity):

1. FlaskInstrumentor
2. DjangoInstrumentor
3. WsgiInstrumentor
4. AsgiInstrumentor
5. StarletteInstrumentor (via AsgiInstrumentor)
6. FastApiInstrumentor (via AsgiInstrumentor)

I did find that http.server.request.duration exemplars generation is ok for:

1. FalconInstrumentor
2. AiohttpInstrumentor, but it is still on the old semconv (so instead it's http.server.duration)
3. PyramidInstrumentor, (as http.server.duration)
4. TornadoInstrumentor, (as http.server.duration)

[felix-hilden]

Thank you for the comment! I've taken the PR out of draft for now and added changelog entries.

@tammy-baylis-swi did you mean that even after the PR there's something wrong with the instrumentors or was this the current state?

In any case, I'd be happy to have some comments on the code and maybe we could get it in 🙏 @xrmx a gentle ping if I may since I saw you review a related change by Tammy recently (in #3912).

[tammy-baylis-swi]

@tammy-baylis-swi did you mean that even after the PR there's something wrong with the instrumentors or was this the current state?

After the PR, yes there are still issues but only for FlaskInstrumentor and DjangoInstrumentor -- I have PRs out for those two.

But also after the PR, StarletteInstrumentor and FastApiInstrumentor should be fixed automatically!