Skip to content

fix(exporter): convert body to str as fallback #4510

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

Closed
wants to merge 1 commit into from
Closed

fix(exporter): convert body to str as fallback #4510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#4494](https://github.com/open-telemetry/opentelemetry-python/pull/4494))
- Improve CI by cancelling stale runs and setting timeouts
([#4498](https://github.com/open-telemetry/opentelemetry-python/pull/4498))
- Fix logging of objects which are convertible to strings
([#4510](https://github.com/open-telemetry/opentelemetry-python/pull/4510))

## Version 1.31.0/0.52b0 (2025-03-12)

Expand Down
6 changes: 5 additions & 1 deletion ...rter-otlp-proto-common/src/opentelemetry/exporter/otlp/proto/common/_internal/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ def _encode_resource(resource: Resource) -> PB2Resource:


def _encode_value(
value: Any, allow_null: bool = False
value: Any,
allow_null: bool = False,
fallback: Optional[Callable[[Any], Any]] = None,
) -> Optional[PB2AnyValue]:
if allow_null is True and value is None:
return None
Expand Down Expand Up @@ -99,6 +101,8 @@ def _encode_value(
]
)
)
elif fallback is not None:
return _encode_value(fallback(value), allow_null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this create unexpected behaviour when complex objects passed incorrectly?
eg. we mask an actual error by logging <MyObject instance at 0x... instead of raising an exception

Copy link
Author

Choose a reason for hiding this comment

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

This function is executed during batch processing, and not when the complex object is passed. Therefore, the exception does not carry a useful stack trace. The exception would carry the type name, but the type name is also contained in the string representation. Also, raising an exception here impacts the whole batch (all lines in it), so not raising here also has some benefits.

More importantly, Python's native logging also converts to strings by default instead of raising an exception. Therefore, OpenTelemetry should follow this approach instead of raising surprising exceptions:

import logging

class Foo:
    pass

logging.warning(Foo())
# no exception, prints `WARNING:root:<__main__.Foo object at 0x…>`

Copy link
Contributor

@jomcgi jomcgi Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That would expand the scope of this function from serializing to the spec to transforming data to comply with the spec.

A different approach was suggested in #3389 which I think would be more appropriate here.

They sub-classed the LoggingHandler and modified it to account for this scenario.

from twisted.logger._stdlib import StringifiableFromEvent

def create_logging_handler():
    logger_provider = LoggerProvider()
    logger_provider.add_log_record_processor(
        BatchLogRecordProcessor(OTLPLogExporter(insecure=True))
    )

    class ModifiedHandler(LoggingHandler):
        def emit(self, record):
            to_str_types = (StringifiableFromEvent)
            if isinstance(record.msg, to_str_types):
                record.msg = str(record.message)
            super().emit(record)

    otel_handler = ModifiedHandler(logger_provider=logger_provider)
    return otel_handler

The logging handler already includes a translate function (here), this would make it easy to create a Logging Handler in opentelemetry-python-contrib that handles this scenario for the third party library.

There is a contrib example that we never merged here: open-telemetry/opentelemetry-python-contrib#2492

A similar issue (support for structlog) was discussed in #2993.

Copy link
Author

Choose a reason for hiding this comment

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

Is this about the _encode_value function specifically? If so, this could be rewritten somehow. I still think that stringification should be the default, because application developers generally don't know what some library may pass to the logging function, and these exceptions result in a bad user experience for app developers integrating OpenTelemetry. For example, I decided to modify your sample to unconditionally stringify all record.msg, because synapse including all of its dependencies is huge, and I don't want to continuously maintain a to_str_types list.

Thanks for the sample. It works for me now. I already used a custom LoggingHandler subclass, so only the emit method was missing.

Copy link
Author

Choose a reason for hiding this comment

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

Also, this PR is useful for #4514, although it may not be ideal. This indicates that the behavior is useful for general purpose, not just for the StringifiableFromEvent from twisted.

raise Exception(f"Invalid type {type(value)} of value {value}")


Expand Down
2 changes: 1 addition & 1 deletion ...to-common/src/opentelemetry/exporter/otlp/proto/common/_internal/_log_encoder/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def _encode_log(log_data: LogData) -> PB2LogRecord:
span_id=span_id,
trace_id=trace_id,
flags=int(log_data.log_record.trace_flags),
body=_encode_value(body, allow_null=True),
body=_encode_value(body, allow_null=True, fallback=str),
severity_text=log_data.log_record.severity_text,
attributes=_encode_attributes(log_data.log_record.attributes),
dropped_attributes_count=log_data.log_record.dropped_attributes,
Expand Down
15 changes: 15 additions & 0 deletions exporter/opentelemetry-exporter-otlp-proto-common/tests/test_log_encoder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,21 @@ def test_encode_no_body(self):

self.assertEqual(encode_logs(sdk_logs), expected_encoding)

def test_encode_stringifiable(self):
class Stringifiable:
def __init__(self, data):
self.data = data
def __str__(self):
return self.data

sdk_logs, expected_encoding = self.get_test_logs()
for log in sdk_logs:
body = log.log_record.body
if isinstance(body, str):
log.log_record.body = Stringifiable(body)

self.assertEqual(encode_logs(sdk_logs), expected_encoding)

def test_dropped_attributes_count(self):
sdk_logs = self._get_test_logs_dropped_attributes()
encoded_logs = encode_logs(sdk_logs)
Expand Down