[Feature Request] Make Temporal logger adapter accomodate to OpenTelemetry

[RoggeOhta]

Problem

Temporal's activity/workflow logger adapter uses a Dict for its "extra" field, violating the OpenTelemetry format that requires basic data types (bool, str, bytes, int, float).

Solution

#838

[RoggeOhta]

#838 I've write a draft solution for this problem

[cretz]

Thanks for the issue!

We cannot incompatibly change default logging for all users because of OpenTelemetry-only limitations. Also per https://opentelemetry.io/docs/languages/python/, OTel logging is still in development. Once it does stabilize, we can definitely look into opt-in options for either splatting the details as done in the PR, or adding something to temporalio.contrib.opentelemetry to help handle this nested "extra" data on log records.

Digging through the OTel source, it seems attributes can have nested dicts. It links to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes which says the attribute values can be any which supports nested dicts I think: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-any. I wonder what type of error you are seeing here?

[RoggeOhta]

Thanks for the issue!

We cannot incompatibly change default logging for all users because of OpenTelemetry-only limitations. Also per https://opentelemetry.io/docs/languages/python/, OTel logging is still in development. Once it does stabilize, we can definitely look into opt-in options for either splatting the details as done in the PR, or adding something to temporalio.contrib.opentelemetry to help handle this nested "extra" data on log records.

Digging through the OTel source, it seems attributes can have nested dicts. It links to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-attributes which says the attribute values can be any which supports nested dicts I think: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-any. I wonder what type of error you are seeing here?

Thank you for the response. I wasn't aware of the opentelemetry-specification. The error is actually a warning generated by the opentelemetry-sdk Python library.
https://github.com/open-telemetry/opentelemetry-python/blob/742171e50c70f4a4540a01d2c6c3cbcf4882d810/opentelemetry-api/src/opentelemetry/attributes/__init__.py#L111

The library verifies if an attribute type is among the _VALID_ATTR_VALUE_TYPES, but it has a broader category called _VALID_ANY_VALUE_TYPES that includes Mapping types, which was not utilized. When it finds attributes that are not of basic types, it removes them, complicating the ability to trace specific activities or workflows.
https://github.com/open-telemetry/opentelemetry-python/blob/742171e50c70f4a4540a01d2c6c3cbcf4882d810/opentelemetry-api/src/opentelemetry/attributes/__init__.py#L25

Could we potentially implement a workaround to address this, like adding an option to flatten the logger's extra field?

[cretz]

Could we potentially implement a workaround to address this, like adding an option to flatten the logger's extra field?

I am concerned about flattening Temporal-specific values just for OTel purposes. To do this right, we'd really need to prefix temporal_activity. and temporal_workflow. to all the extra value names when flattening. But it is technically something we can do, though we may want to wait until logging stabilizes in case there is a better handler-level approach we can devise instead of doing this at the logger level.

I think a better approach may be to encourage custom adapters. You could set temporalio.activity.logger and temporalio.workflow.logger to your own class extensions of temporalio.activity.LoggerAdapter and temporalio.workflow.LoggerAdapter setting the activity_info_on_extra and workflow_info_on_extra to False on super init or post super init. Then you can override process to set whatever "extra" values from the activity/workflow context you'd like instead of using the preset default ones. This is the primary reason we exposed and documented those adapter classes is for this kind of advanced customization.

[RoggeOhta]

Okay, I found a workaround. Posting in case it helps others who encounter this issue.

First, set global activity and workflow logger at your program init. Turn off the extra info.

activity.logger.activity_info_on_extra = False
workflow.logger.workflow_info_on_extra = False

Then, in every activity/workflow, add this info manually to your own logger.

# start of some activity
my_logger.extra = activity.info()._logger_details()

# start of some workflow
my_logger.extra = workflow.info()._logger_details()

This should bypass the python oTel library warning while retaining activity trace logs.

[cretz]

👍

Then, in every activity/workflow, add this info manually to your own logger

An alternative is to replace those loggers with your own extension of activity.LoggerAdapter and workflow.LoggerAdapter that overrides process to set these values.

The concern here for us adding this option as-is is that we need to qualify all keys with temporal_activity. and temporal_workflow. prefixes probably if we're not allowed nested maps. We may be able to add that option today or wait until OTel logging stabilizes.