-
Notifications
You must be signed in to change notification settings - Fork 151
fix(logging): make LoggerAdapter OpenTelemetry-safe (Issue #837) #1283
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
base: main
Are you sure you want to change the base?
fix(logging): make LoggerAdapter OpenTelemetry-safe (Issue #837) #1283
Conversation
- Add TestFlattenModeOTelSafety class with critical assertions - Verify zero dict values exist for temporal keys in flatten mode - Verify legacy nested keys (temporal_workflow, temporal_activity) don't exist - Test both workflow and activity contexts - Test update context handling in flatten mode
Resolve conflict in temporalio/workflow.py by keeping both: - temporal_extra_mode from this branch - disable_sandbox from main Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| namespaced prefix. Values that are not primitives (str/int/float/bool) | ||
| are converted to strings. This mode is recommended for OpenTelemetry | ||
| and other logging pipelines that require flat, scalar attributes. | ||
| json: Add context as a JSON string under a single key. Useful when |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually have any known use case for json mode?
| as passthrough modules. | ||
|
|
||
| **For performance and behavior reasons, users are encouraged to pass through all third party modules whose calls will be | ||
| **Passthrough modules are about import-time behavior and determinism, not just about whether a module is third-party.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be a number of unrelated readme changes.
| activity.logger.temporal_extra_mode = "flatten" | ||
|
|
||
| handler = logging.handlers.QueueHandler(queue.Queue()) | ||
| activity.logger.base_logger.addHandler(handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do this pattern a few times, it is probably worth creating a context manager to reduce duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But actually, I think we might be better served here by a single parameterized test with different assertions
| Args: | ||
| extra: The mutable extra dict to update. | ||
| key: The key to use for dict/json modes (e.g., "temporal_workflow"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a great reason to distinguish key and prefix. Do you have any compelling argument not to use the same one for both?
| ) | ||
|
|
||
|
|
||
| class TestApplyTemporalContextToExtra: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might also be a way to merge some of these tests with parameterization. Not entirely sure, but give it a pass and see.
Summary
This PR adds an OTel-safe logging mode for Temporal's Python
LoggerAdapterclasses, addressing Issue #837 where nested dicts inLogRecord.extrabreak OpenTelemetry logging pipelines.Problem
Temporal's
workflow.LoggerAdapterandactivity.LoggerAdapterinject nested dictionaries intoLogRecord.extra:OpenTelemetry log attributes must be AnyValue types (scalars, not nested dicts). This causes Temporal context to be dropped or cause errors in OTel pipelines.
Solution
Add a
temporal_extra_modeattribute to bothLoggerAdapterclasses with three modes:"dict"(default)temporal_workflow/temporal_activity"flatten"temporal.workflow.*/temporal.activity.*prefixes"json"Usage
Changes
temporalio/_log_utils.py- Shared helper functions andTemporalLogExtraModetypetemporalio/workflow.py- Addedtemporal_extra_modetoLoggerAdaptertemporalio/activity.py- Addedtemporal_extra_modetoLoggerAdaptertests/test_log_utils.py- Unit tests for all modestests/worker/test_workflow.py- Integration tests for workflow logging modestests/worker/test_activity.py- Integration tests for activity logging modesREADME.md- Clarified passthrough vsinvalid_module_membersfor datetimeVerification Checklist
"dict") preserves existing behavior - no breaking changestemporal.workflow.*andtemporal.activity.*prefixesLogRecordcore attributesTest Plan
Verify default behavior unchanged:
Verify flatten mode is OTel-safe:
Verify JSON mode:
Fixes #837
🤖 Generated with Claude Code