open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK
https://opentelemetry.io
Apache License 2.0
1.82k stars 635 forks source link

Fix: filter exemplar for observable instrument and export of exemplar without trace and span ids #4251

Closed fcollonval closed 2 weeks ago

fcollonval commented 2 weeks ago

Description

Fixes #4250

The two latter fixes were extracted from #4178

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 Contrib Repo Change?

Checklist:

fcollonval commented 2 weeks ago

Thanks @xrmx for the quick review - I don't know what to do about the remaining failure for the generate-workflows job.

emdneto commented 2 weeks ago

Thanks @xrmx for the quick review - I don't know what to do about the remaining failure for the generate-workflows job.

~We need to update the workflows. I can fix it for you~. It seems we have another issue with internal generate-workflows-lib. Fix here https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2964

xrmx commented 2 weeks ago

@fcollonval I think we are also missing tests for the OTEL_METRICS_EXEMPLAR_FILTER env var, when I set OTEL_METRICS_EXEMPLAR_FILTER=always_off, the correct sampler is set but I get exemplars in the exported metrics but this is fixed by this PR too.

UPDATE: maybe the test requested by Aaron is enough

xrmx commented 2 weeks ago

I've started writing some integration tests but the always_off and trace_based are wrong because there should be an exemplar in the trace_based one: https://github.com/open-telemetry/opentelemetry-python/commit/056313cfe738d0aa8d3a0f7e2ee07102d1288d09

xrmx commented 2 weeks ago

@aabmass Emidio fixed tests PTAL

fcollonval commented 2 weeks ago

Thanks a lot to all of you for helping fixing this and more generally for the maintenance of this great repo.