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

fcollonval · 2024-11-06T15:18:11Z

Description

Fixes #4250

It applies correctly the exemplar filter to the observable instruments
It does not export the trace_id and span_id attributes of exemplars if undefined
It fixes the type of trace_id and span_id in exemplar classes

The two latter fixes were extracted from #4178

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

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

Check example docs/examples/metrics/instruments/example.py
Add tests by @xrmx in exporter/opentelemetry-exporter-otlp-proto-common/tests/test_metrics_encoder.py
Extend test to check consume_measurement is called with exemplar filter result for async instrument

Does This PR Require a Contrib Repo Change?

Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
~~Documentation has been updated~~

...to-common/src/opentelemetry/exporter/otlp/proto/common/_internal/metrics_encoder/__init__.py

fcollonval · 2024-11-06T15:46:17Z

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

emdneto · 2024-11-06T15:59:09Z

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 open-telemetry/opentelemetry-python-contrib#2964

aabmass

Change LGTM but can you add an integration test case for this bug? Here's an existing test you could model it after https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-sdk/tests/metrics/integration_test/test_disable_default_views.py

xrmx · 2024-11-07T09:19:32Z

@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 · 2024-11-07T14:48:34Z

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:
056313c

Signed-off-by: emdneto <[email protected]>

opentelemetry-sdk/tests/metrics/integration_test/test_exemplars.py

…s.py

xrmx · 2024-11-08T09:24:53Z

@aabmass Emidio fixed tests PTAL

opentelemetry-sdk/tests/metrics/integration_test/test_exemplars.py

…emplar

… without trace and span ids (#4251) * Deal with missing span and trace ids * Fix applying exemplar filter to observable instruments * Lint the code * add tests * Add entry in changelog * Fix span and trace id typing * Fix CI * Test consume_measurement is called for async instrument * Add integration tests * fix integration tests Signed-off-by: emdneto <[email protected]> * Update opentelemetry-sdk/tests/metrics/integration_test/test_exemplars.py * add test that default exemplar filter with no span does not create exemplar --------- Signed-off-by: emdneto <[email protected]> Co-authored-by: Frédéric Collonval <[email protected]> Co-authored-by: Riccardo Magliocchetti <[email protected]> Co-authored-by: Emídio Neto <[email protected]> Co-authored-by: Aaron Abbott <[email protected]>

fcollonval · 2024-11-09T14:48:03Z

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

fcollonval and others added 5 commits November 6, 2024 16:17

Deal with missing span and trace ids

80bd271

Fix applying exemplar filter to observable instruments

72519a1

Lint the code

7c58545

add tests

6ed6192

Add entry in changelog

0e6d185

fcollonval changed the title ~~Fix/exemplar-sampling-export~~ Fix: filter exemplar for observable instrument and export of exemplar without trace and span ids Nov 6, 2024

Fix span and trace id typing

48d786e

fcollonval force-pushed the fix/exemplar-sampling-export branch from de27bbc to 48d786e Compare November 6, 2024 15:22

xrmx reviewed Nov 6, 2024

View reviewed changes

fcollonval added 2 commits November 6, 2024 16:35

Fix CI

45d7a71

Test consume_measurement is called for async instrument

298cc1e

fcollonval marked this pull request as ready for review November 6, 2024 15:46

fcollonval requested a review from a team as a code owner November 6, 2024 15:46

emdneto added sdk Affects the SDK package. metrics labels Nov 6, 2024

xrmx approved these changes Nov 6, 2024

View reviewed changes

xrmx mentioned this pull request Nov 6, 2024

Fix workflows #4254

Closed

11 tasks

aabmass requested changes Nov 6, 2024

View reviewed changes

emdneto force-pushed the fix/exemplar-sampling-export branch from 14cbf8f to 298cc1e Compare November 6, 2024 19:50

Merge branch 'main' into fix/exemplar-sampling-export

02bdfa8

xrmx and others added 3 commits November 7, 2024 21:03

Add integration tests

dd8b269

fix integration tests

aadd4be

Signed-off-by: emdneto <[email protected]>

Merge branch 'main' into fix/exemplar-sampling-export

2ce2d28

xrmx reviewed Nov 8, 2024

View reviewed changes

opentelemetry-sdk/tests/metrics/integration_test/test_exemplars.py Outdated Show resolved Hide resolved

Update opentelemetry-sdk/tests/metrics/integration_test/test_exemplar…

575cd2d

…s.py

xrmx requested a review from aabmass November 8, 2024 09:24

xrmx mentioned this pull request Nov 8, 2024

sdk/metrics: performance impact of exemplars support even when configured as always_off #4243

Open

emdneto approved these changes Nov 8, 2024

View reviewed changes

Merge branch 'main' into fix/exemplar-sampling-export

882fd2c

lzchen approved these changes Nov 8, 2024

View reviewed changes

aabmass reviewed Nov 8, 2024

View reviewed changes

opentelemetry-sdk/tests/metrics/integration_test/test_exemplars.py Show resolved Hide resolved

add test that default exemplar filter with no span does not create ex…

d5069e8

…emplar

aabmass approved these changes Nov 8, 2024

View reviewed changes

xrmx merged commit c4fa8d7 into open-telemetry:main Nov 8, 2024
353 checks passed

fcollonval deleted the fix/exemplar-sampling-export branch November 9, 2024 14:30

fcollonval mentioned this pull request Nov 9, 2024

Add Prometheus Exporter for Exemplars #4178

Open

6 tasks

alexmojaki mentioned this pull request Nov 13, 2024

Daily CI with unpinned deps pydantic/logfire#583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fcollonval commented Nov 6, 2024 •

edited

Loading

fcollonval commented Nov 6, 2024

emdneto commented Nov 6, 2024 •

edited

Loading

aabmass left a comment •

edited

Loading

xrmx commented Nov 7, 2024 •

edited

Loading

xrmx commented Nov 7, 2024

xrmx commented Nov 8, 2024

fcollonval commented Nov 9, 2024 •

edited

Loading

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

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

Conversation

fcollonval commented Nov 6, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Contrib Repo Change?

Checklist:

fcollonval commented Nov 6, 2024

emdneto commented Nov 6, 2024 • edited Loading

aabmass left a comment • edited Loading

Choose a reason for hiding this comment

xrmx commented Nov 7, 2024 • edited Loading

xrmx commented Nov 7, 2024

xrmx commented Nov 8, 2024

fcollonval commented Nov 9, 2024 • edited Loading

fcollonval commented Nov 6, 2024 •

edited

Loading

emdneto commented Nov 6, 2024 •

edited

Loading

aabmass left a comment •

edited

Loading

xrmx commented Nov 7, 2024 •

edited

Loading

fcollonval commented Nov 9, 2024 •

edited

Loading