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.

Sign up for GitHub

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

Jump to bottom

Handle none value for OTEL_PROPAGATORS #4236

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

Handle none value for OTEL_PROPAGATORS #4236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
672a69f
Added none value for OTEL_PROPAGATORS
Annosha Oct 23, 2024
41b0350
Merge branch 'main' into otel-propagators-none-value
Annosha Oct 23, 2024
b07670f
removed redundant code
Annosha Oct 23, 2024
0e5f610
lint fixes
Annosha Oct 24, 2024
a78ffd7
fixing the for-loop
Annosha Oct 25, 2024
883f296
Merge branch 'main' into otel-propagators-none-value
Annosha Oct 25, 2024
5cbe62f
test: validate empty propagators for OTEL_PROPAGATORS="none"
Annosha Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 24 additions & 21 deletions opentelemetry-api/src/opentelemetry/propagate/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,28 +129,31 @@ def inject(
"tracecontext,baggage",
)


for propagator in environ_propagators.split(","):
propagator = propagator.strip()

try:
propagators.append( # type: ignore
next( # type: ignore
iter( # type: ignore
entry_points( # type: ignore
group="opentelemetry_propagator",
name=propagator,
# Simplified logic to handle "none" case
if environ_propagators.lower() == "none":
propagators = []
Annosha marked this conversation as resolved.
Show resolved Hide resolved
else:
propagators = []
for propagator in environ_propagators.split(","):
propagator = propagator.strip()
try:
propagators.append( # type: ignore
next( # type: ignore
iter( # type: ignore
entry_points( # type: ignore
group="opentelemetry_propagator",
name=propagator,
)
)
)
).load()()
)
except StopIteration:
raise ValueError(
f"Propagator {propagator} not found. It is either misspelled or not installed."
)
except Exception: # pylint: disable=broad-exception-caught
logger.exception("Failed to load propagator: %s", propagator)
raise
).load()()
)
except StopIteration:
raise ValueError(
f"Propagator {propagator} not found. It is either misspelled or not installed."
)
except Exception: # pylint: disable=broad-exception-caught
logger.exception("Failed to load propagator: %s", propagator)
raise


_HTTP_TEXT_FORMAT = composite.CompositePropagator(propagators) # type: ignore
Expand Down
19 changes: 19 additions & 0 deletions opentelemetry-api/tests/propagators/test_propagators.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,25 @@ 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 •
edited
Loading

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 •
edited
Loading

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)

# Verify if exception is raised without the fix
with self.assertRaises(Exception):
from opentelemetry.propagate import get_global_textmap

get_global_textmap()

# Apply fix and verify propagators is empty, with no exception raised
try:
from opentelemetry.propagate import get_global_textmap

mock_compositehttppropagator.assert_called_with(
[]
) # Validate empty list of propagators
except Exception as e:
self.fail(f"Test failed unexpectedly with an exception: {e}")

@patch.dict(environ, {OTEL_PROPAGATORS: "a, b, c "})
@patch("opentelemetry.propagators.composite.CompositePropagator")
@patch("opentelemetry.util._importlib_metadata.entry_points")
Expand Down
Loading