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

Include update info in logging output #664

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

dandavison
Copy link
Contributor

Closes #648: include update info in logging output when in the context of an update execution

Also changes behavior such that if user logging call uses one of our keys then their call has precedence; this is the behavior of merge_extra=True added in Python 3.13.

How this was tested

@dandavison dandavison requested a review from a team as a code owner October 9, 2024 22:02
@dandavison dandavison changed the title 648 update logging Include update info in logging output Oct 9, 2024
@@ -500,6 +500,14 @@ class UpdateInfo:
name: str
"""Update type name."""

@property
def logger_details(self) -> Mapping[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def logger_details(self) -> Mapping[str, Any]:
def _logger_details(self) -> Mapping[str, Any]:

I don't think we should make this public (and have it then be stable API)

update_info = current_update_info()
if update_info:
if self.update_info_on_extra:
extra["temporal_update"] = asdict(update_info)
Copy link
Member

@cretz cretz Oct 10, 2024

Choose a reason for hiding this comment

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

Arguably update info is just more workflow info and doesn't need a separate top-level key. I think it can just have the extra fields be put into temporal_workflow section same as appending to the message does.

@@ -1220,6 +1236,7 @@ def __init__(
super().__init__(logger, extra or {})
self.workflow_info_on_message = True
self.workflow_info_on_extra = True
self.update_info_on_extra = True
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this deserves a separate setting. I think these two pieces of update information can be considered workflow information (and we are considering them workflow information for purposes of appending to the message, might as well in the extra too consistently).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show update handler and ID in logging context
2 participants