Skip to content

[pkg/ottl] Fix the context inference order to prioritize scope over resource #39307

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

Merged

Conversation

edmocosta
Copy link
Contributor

Description

Changed the context inferrer so the scope context has priority over the resource context, fixing the error described on the linked issue.

Link to tracking issue

Fixes #39155

@edmocosta edmocosta changed the title [pkg/ottl] Fix the context inference order to prioritize scope over resource [pkg/ottl] Fix the context inference order to prioritize scope over resource Apr 10, 2025
@edmocosta edmocosta marked this pull request as ready for review April 10, 2025 17:02
@edmocosta edmocosta requested a review from a team as a code owner April 10, 2025 17:02
Copy link
Contributor

@dehaansa dehaansa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test that mimics the reported context inference, using the default inferrer?

It seems all the tests use a test-provided priority rather than the default inferrer priority.

@edmocosta
Copy link
Contributor Author

edmocosta commented Apr 11, 2025

Can we add a test that mimics the reported context inference, using the default inferrer?
It seems all the tests use a test-provided priority rather than the default inferrer priority.

Hi @dehaansa, thanks for reviewing! the current testing strategy was ensuring the context priority list works (by mocking the priority values and checking the inferred value), and then testing if the default priority list is the one we expect so OTTL contexts would be properly inferred (see). Those tests cases should be enough to guarantee correctness, but I agree that it wouldn't hurt adding more explicit tests for the existing OTTL contexts. I've pushed the changes already.

@TylerHelmuth TylerHelmuth merged commit be72f9c into open-telemetry:main Apr 11, 2025
171 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 11, 2025
akshays-19 pushed a commit to akshays-19/opentelemetry-collector-contrib that referenced this pull request Apr 23, 2025
… `resource` (open-telemetry#39307)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Changed the context inferrer so the `scope` context has priority over
the `resource` context, fixing the error described on the linked issue.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes
open-telemetry#39155

<!--Please delete paragraphs that you did not use before submitting.-->
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
… `resource` (open-telemetry#39307)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Changed the context inferrer so the `scope` context has priority over
the `resource` context, fixing the error described on the linked issue.

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes
open-telemetry#39155

<!--Please delete paragraphs that you did not use before submitting.-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTTL context inference order is incorrect for resources & scopes
4 participants