-
Notifications
You must be signed in to change notification settings - Fork 636
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
base: main
Are you sure you want to change the base?
Conversation
@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. |
@xrmx I ran both |
@@ -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): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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)
The test is |
none
value for OTEL_PROPAGATORS
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:
Verifies that when OTEL_PROPAGATORS="none", no propagators are loaded, and the list of propagators is empty.
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 when
OTEL_PROPAGATORS
is set to"none"
.The existing tests for default and non-default propagators remain intact.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
tox test keeps on running forever. I'm wondering if there are instructions for specific tests?
Successfully ran:
Does This PR Require a Contrib Repo Change?
Checklist: