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 workflow logging output #1595

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

THardy98
Copy link
Contributor

@THardy98 THardy98 commented Jan 8, 2025

This change includes update info (update id and name) in workflow logging output when logging within an update handler.

  1. Closes Show update handler and ID in logging context #1532

  2. How was this tested:
    Added small integration test

  3. Any docs updates needed?
    Not sure

This change includes update info (update id and name) in workflow logging output when logging within an update handler.
@THardy98 THardy98 requested a review from mjameswh January 8, 2025 21:16
@THardy98 THardy98 requested a review from a team as a code owner January 8, 2025 21:16
return loggerSink[level](message, {
// Inject the call time in nanosecond resolution as expected by the worker logger.
[LogTimestamp]: activator.getTimeOfDay(),
sdkComponent: SdkComponent.workflow,
...getLogAttributes(workflowLogAttributes(activator.info)),
...attrs,
// Add update info if it exists
...(updateInfo && { updateId: updateInfo.id }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a single object spread operation for both id and name.

Something like that:

Suggested change
...(updateInfo && { updateId: updateInfo.id }),
...(updateInfo && { updateId: updateInfo.id, updateName: updateInfo.name }),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more about this, I'd say we should do this in workflowLogAttributes(), for coherency. That behavior will be visible to user interceptors.

Only problem though is that workflowLogAttributes() may be called from outside the workflow sandbox, so you will need to add a call to inWorkflowContext() first to not attempt this in other cases.

That could give something like this:

export function workflowLogAttributes(info: WorkflowInfo): Record<string, unknown> {
  const attributes = {
    namespace: info.namespace,
    taskQueue: info.taskQueue,
    workflowId: info.workflowId,
    runId: info.runId,
    workflowType: info.workflowType,
  };

  // Add details about the current update if appropriate
  if (inWorkflowContext()) {
    const updateInfo = currentUpdateInfo();
    if (updateInfo) {
      attributes['updateId'] = updateInfo.id;
      attributes['updateName'] = updateInfo.name;
    }
  }

  return attributes;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, updated with the snippet above.

Only problem though is that workflowLogAttributes() may be called from outside the workflow sandbox, so you will need to add a call to inWorkflowContext() first to not attempt this in other cases.

From the code references, does this only occur when a worker creates a new workflow?
https://github.com/temporalio/sdk-typescript/blob/sdk-t-add-update-info-to-logging-context/packages/worker/src/worker.ts#L1313

Copy link
Contributor

Choose a reason for hiding this comment

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

From the code references, does this only occur when a worker creates a new workflow?

Probably, yeah. This is used to provide some basic attributes very early during workflow initialization and in case starting the workflow fail for whatever reason.

Arguably, we could duplicate that code, once in Worker knowing that we are not in workflow context, and once in workflows/logs.ts where we'd know we are in workflow context, but I think that in this case it is best to keep this in a single place, to avoid eventual inconsistencies whenever we add extra workflow log attributes.

@THardy98 THardy98 force-pushed the sdk-t-add-update-info-to-logging-context branch from ed24ce5 to 496e575 Compare January 10, 2025 16:14
@THardy98 THardy98 requested a review from mjameswh January 10, 2025 16:14
@THardy98 THardy98 force-pushed the sdk-t-add-update-info-to-logging-context branch from 496e575 to 6b229a2 Compare January 10, 2025 16:44
@THardy98
Copy link
Contributor Author

Not sure what is causing CI to fail, its unrelated to the code changes

@mjameswh
Copy link
Contributor

Not sure what is causing CI to fail, its unrelated to the code changes

That's unrelated. I'm already on this: #1596.

@mjameswh mjameswh merged commit 6a4ea4a into main Jan 10, 2025
23 checks passed
@mjameswh mjameswh deleted the sdk-t-add-update-info-to-logging-context branch January 10, 2025 21:20
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