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

bugfix: OpenTracing Shim not converting exceptions to format expected by OpenTelemetry #4184

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#4154](https://github.com/open-telemetry/opentelemetry-python/pull/4154))
- sdk: Add support for log formatting
([#4137](https://github.com/open-telemetry/opentelemetry-python/pull/4166))
- Fixed OpenTracing Shim to properly convert exceptions to the format expected by OpenTelemetry
([#4184](https://github.com/open-telemetry/opentelemetry-python/pull/4184))

## Version 1.27.0/0.48b0 (2024-08-28)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,21 @@ def set_tag(self, key: str, value: ValueT) -> "SpanShim":
self._otel_span.set_attribute(key, value)
return self

def _sanitize_attribute_value(self, key, value):
if isinstance(value, (str, bool, int, float)):
return value
if isinstance(value, (list, tuple)):
return [self._sanitize_attribute_value(key, v) for v in value]
# For unsupported types, convert to string
logger.warning(
"Invalid type %s for attribute '%s' value. Expected one of "
"['bool', 'str', 'bytes', 'int', 'float'] or a sequence of those types. "
"Converting to string.",
type(value).__name__,
key,
)
return str(value)

def log_kv(
self, key_values: Attributes, timestamp: float = None
) -> "SpanShim":
Expand Down Expand Up @@ -280,7 +295,16 @@ def log_kv(
event_timestamp = None

event_name = util.event_name_from_kv(key_values)
self._otel_span.add_event(event_name, key_values, event_timestamp)

# Sanitize the key_values dictionary
sanitized_key_values = {
k: self._sanitize_attribute_value(k, v)
for k, v in key_values.items()
}

self._otel_span.add_event(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since add_event uses BoundedAttributes aren't all values already sanitized to conform to the OpenTelemetry AttributeValue definition?

Copy link
Contributor Author

@zhihali zhihali Sep 10, 2024

Choose a reason for hiding this comment

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

Indeed, but if we don't have these code, the value will not be reformated to str.

Should I do some change in BoundedAttributes?

Copy link
Member

Choose a reason for hiding this comment

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

@zhihali do you mean change _clean_attribute to cast everything else to str?

event_name, sanitized_key_values, event_timestamp
)
return self

@deprecated(reason="This method is deprecated in favor of log_kv")
Expand Down
52 changes: 52 additions & 0 deletions shim/opentelemetry-opentracing-shim/tests/test_shim.py
Original file line number Diff line number Diff line change
Expand Up @@ -673,3 +673,55 @@ def test_mixed_mode(self):
scope.span.unwrap().parent,
opentelemetry_span.context,
)

def test_log_kv_sanitization(self):
"""Test that log_kv correctly sanitizes non-primitive types and logs warnings."""
with self.assertLogs(level="WARNING") as log_context:
with self.shim.start_span("TestSpan") as span:
# Test primitive types
span.log_kv(
{"str": "string", "int": 123, "float": 1.23, "bool": True}
)

# Test sequence of primitive types
span.log_kv({"list": [1, 2, 3], "tuple": ("a", "b", "c")})

# Test non-primitive type
class CustomClass:
def __str__(self):
return "CustomClass"

span.log_kv(
{"custom": CustomClass(), "exception": Exception("Test")}
)

# Verify the logged key-value pairs
events = span.unwrap().events
self.assertEqual(len(events), 3) # Three log_kv calls

# Check primitive types
self.assertEqual(events[0].attributes["str"], "string")
self.assertEqual(events[0].attributes["int"], 123)
self.assertEqual(events[0].attributes["float"], 1.23)
self.assertEqual(events[0].attributes["bool"], True)

# Check sequence types
self.assertEqual(events[1].attributes["list"], (1, 2, 3))
self.assertEqual(events[1].attributes["tuple"], ("a", "b", "c"))

# Check non-primitive types
self.assertEqual(events[2].attributes["custom"], "CustomClass")
self.assertEqual(str(events[2].attributes["exception"]), "Test")

# Verify that warnings were logged
self.assertEqual(
len(log_context.records), 2
) # Two warnings should be logged
self.assertIn(
"Invalid type CustomClass for attribute 'custom' value",
log_context.output[0],
)
self.assertIn(
"Invalid type Exception for attribute 'exception' value",
log_context.output[1],
)