Skip to content
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

Handle none value for OTEL_PROPAGATORS #4236

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Annosha
Copy link

@Annosha Annosha commented Oct 23, 2024

Fixes #4143 OTEL_PROPAGATORS does not support "none" value #4143

Description

This PR introduces changes to the test suite to verify the behavior of the OTEL_PROPAGATORS environment variable when set to "none". The following updates were made:

  1. Added a new test case:

Verifies that when OTEL_PROPAGATORS="none", no propagators are loaded, and the list of propagators is empty.

  1. Modified test setup:

Reloaded the opentelemetry.propagate module in the new test to apply the environment variable settings and trigger the correct propagator configuration.
These changes ensure that:

When OTEL_PROPAGATORS="none", propagators are an empty list.
Default propagators (tracecontext, baggage) are not loaded whenOTEL_PROPAGATORSis set to "none".
The existing tests for default and non-default propagators remain intact.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

tox test keeps on running forever. I'm wondering if there are instructions for specific tests?

Successfully ran:

tox -e py312-test-opentelemetry-api
tox -e py312-test-opentelemetry-sdk

tox -e ruff

.tox\lint\Scripts\black . 
.tox\lint\Scripts\isort . 

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR: Will open a PR once the code changes are approved.
  • No.

Checklist:

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

@Annosha Annosha requested a review from a team as a code owner October 23, 2024 13:19
@Annosha
Copy link
Author

Annosha commented Oct 23, 2024

@lzchen @xrmx Please check the changes and provide your valuable feedback

@Annosha
Copy link
Author

Annosha commented Oct 23, 2024

@xrmx Sorry about the redundant imports. Please check the changes.

@xrmx
Copy link
Contributor

xrmx commented Oct 24, 2024

@xrmx Sorry about the redundant imports. Please check the changes.

Are you running tests locally? Please do that before asking people to review. Also please take a look at CONTRIBUTING.md and run linting tools too.

@Annosha
Copy link
Author

Annosha commented Oct 24, 2024

@xrmx I ran both tox -e opentelemetry-api and tox -e opentelemetry-sdk and tox -e ruff for lint errors. I hope the tests will pass now.
Apologies for the inconvenience, I was working on multiple otel-language-sdks and somehow missed the steps.

@@ -49,6 +49,18 @@ def test_propagators(propagators):

reload(opentelemetry.propagate)

@patch.dict(environ, {OTEL_PROPAGATORS: "none"})
@patch("opentelemetry.propagators.composite.CompositePropagator")
def test_no_propagators_loaded(self, mock_compositehttppropagator):
Copy link
Member

@emdneto emdneto Oct 24, 2024

Choose a reason for hiding this comment

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

I ran this test locally, and it doesn't fail against the main branch

Copy link
Author

Choose a reason for hiding this comment

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

@emdneto I apologize for the confusion. Could you clarify what you would like me to address?

Copy link
Contributor

@xrmx xrmx Oct 25, 2024

Choose a reason for hiding this comment

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

@Annosha The test should fail on main and raise and exception (as reported in the issue). If it doesn't fail with the current code it means it's not a good test to evaluate if the fix is working.

Copy link
Author

Choose a reason for hiding this comment

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

@xrmx and @emdneto The test is failing after fixes in testing logic. This failure confirms that the test accurately detects the issue. Here is the test log on my machine:
test-fail-for-PR-OTEL-Propagators-NONE-vlaue
Could you please review this on your end and advise on any further improvements needed?

Copy link
Member

Choose a reason for hiding this comment

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

I was expecting something like that: this should fail when running against main with the ValueError present on the issue

        global_textmap = propagate.get_global_textmap()
        self.assertIsInstance(global_textmap, composite.CompositePropagator)
        self.assertEqual(len(global_textmap._propagators), 0)

@emdneto
Copy link
Member

emdneto commented Oct 24, 2024

@xrmx I ran both tox -e opentelemetry-api and tox -e opentelemetry-sdk and tox -e ruff for lint errors. I hope the tests will pass now. Apologies for the inconvenience, I was working on multiple otel-language-sdks and somehow missed the steps.

The test is tox -e py312-test-opentelemetry-api

@emdneto emdneto changed the title Fixes #4143 Added "none" value for OTEL_PROPAGATORS Handle "none" value for OTEL_PROPAGATORS Oct 24, 2024
@emdneto emdneto changed the title Handle "none" value for OTEL_PROPAGATORS Handle none value for OTEL_PROPAGATORS Oct 24, 2024
@Annosha
Copy link
Author

Annosha commented Oct 25, 2024

@xrmx @emdneto indeed the commands I used were tox -e py312-test-opentelemetry-api and tox -e py312-test-opentelemetry-sdk for py 3.12
Lint checks are complete too. Please review the code and let me know if further changes are required. TIA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OTEL_PROPAGATORS does not support "none" value
3 participants