Repository navigation
Fix crashes from unguarded str()/repr() on invalid attribute keys/values - #5598
Open
piyush182004 wants to merge 1 commit into
Open
piyush182004 wants to merge 1 commit into
piyush182004 wants to merge 1 commit into
Conversation
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-17 10:43 UTC Review the latest changes. Status above doesn't look right?
|
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
GauravR31
approved these changes
Sep 25, 2026
Contributor
Author
|
Still active on this, just waiting on review. Happy to rebase or split it up if that helps. |
herin049
reviewed
Oct 14, 2026
piyush182004
force-pushed
the
fix/unguarded-str-crashes-on-invalid-attributes
branch
2 times, most recently
from
October 16, 2026 17:41
f616f46 to
17cdf10
Compare
Two call sites use "best-effort stringify" as a fallback for attribute keys/values and log exception messages, but neither guards the str() call itself, so an object whose __str__/__repr__ raises crashes the caller's application: - opentelemetry-api/src/opentelemetry/attributes/__init__.py: _clean_attribute_value's docstring says an unstringifiable value "is replaced with None", but str(key)/str(value) were unguarded, so an object whose __str__/__repr__ itself raises crashed span.set_attribute(...) instead. - opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py: LoggingHandler._get_attributes called str(value.args[0]) unguarded when translating a stdlib LogRecord's exc_info, crashing the common logger.exception(...) pattern whenever the raised exception's first arg has a raising __str__. Both now catch the stringification failure, log a warning, and degrade gracefully (drop the offending key or replace the value) instead of propagating the exception into the caller's application code -- which observability/telemetry code should never do. The baggage propagation change from the original version of this PR has been dropped per review: baggage keys and values are required to be strictly UTF-8 strings, so W3CBaggagePropagator is not the place to guard against a raising __str__/__repr__. Fixes open-telemetry#5597
piyush182004
force-pushed
the
fix/unguarded-str-crashes-on-invalid-attributes
branch
from
October 16, 2026 17:53
17cdf10 to
539115f
Compare
Contributor
Author
|
@herin049 can you please review this once? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two call sites use "best-effort stringify" as a fallback for attribute
keys/values and log exception messages, but neither guards the
str()call itself, so an object whose
__str__/__repr__raises crashes thecaller's application:
opentelemetry-api/src/opentelemetry/attributes/__init__.py:_clean_attribute_value's docstring says an unstringifiable value "isreplaced with None", but
str(key)/str(value)were unguarded, so anobject whose
__str__/__repr__itself raises crashedspan.set_attribute(...)instead.opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py:LoggingHandler._get_attributescalledstr(value.args[0])unguardedwhen translating a stdlib
LogRecord'sexc_info, crashing the commonlogger.exception(...)pattern whenever the raised exception's firstarg has a raising
__str__.Both now catch the stringification failure, log a warning, and degrade
gracefully (drop the offending key, or replace the value with
None/ aplaceholder message) instead of propagating the exception into the
caller's application code — which observability/telemetry code should
never do.
Note: An earlier version of this PR also guarded
_encode_baggage_pairsin the W3C baggage propagator. That change has been dropped per review
(baggage keys and values are required to be strictly UTF-8 strings, so
W3CBaggagePropagatoris not the place to guard against a raising__str__/__repr__). The branch has been rebased onto the latestmainand squashed to a single commit.
Fixes #5597
Type of change
How Has This Been Tested?
Added 4 regression tests across the two affected test files:
test_clean_attribute_value_raising_repr_does_not_crash,test_bounded_attributes_raising_repr_value_does_not_crash,test_bounded_attributes_raising_repr_key_does_not_crash(
opentelemetry-api/tests/attributes/test_attributes.py)test_log_record_exception_with_raising_arg_does_not_crash(
opentelemetry-sdk/tests/logs/test_handler.py)Verified each test is load-bearing: reverting the matching production fix
makes the test fail with the original crash traceback, and restoring the
fix makes it pass.
Ran the affected
opentelemetry-apiandopentelemetry-sdksuites, plusruff check,ruff format --check, andpylint(rated 10.00/10) — allclean.
test_clean_attribute_value_raising_repr_does_not_crashDoes This PR Require a Contrib Repo Change?
Checklist: