Add logSource and taskQueue metadata on log messages
#1391
Conversation
|
Ideas of names for the "subsystem" attribute:
|
mjameswh
changed the title
logSource and workerId metadata on log messages
mjameswh
changed the title
logSource and workerId metadata on log messageslogSource and workerId metadata on log messages
mjameswh
force-pushed
the
debug-shutdown-issue
branch
from
1dd22df
to
469dbca
Compare
mjameswh
changed the title
logSource and workerId metadata on log messageslogSource and taskQueue metadata on log messages
packages/core-bridge/src/helpers.rs
Outdated
|
|
||
| // Convert from Core's log "targets" (actually full module name, like "temporal_sdk_core::worker::workflow"), | ||
| // to the format used in our JavaScript side logger (i.e. "core/worker/workflow"). | ||
| pub fn rust_package_to_js_style(input: &str) -> String { |
There was a problem hiding this comment.
This seems slightly overkill to me, but you've done it so no reason to get rid of it.
There was a problem hiding this comment.
+1, just slap a core tag on every log record and be done with it.
Leave the Rust style tags verbatim so they're consistent when logging to console or being forwarded to lang.
There was a problem hiding this comment.
I'd strongly prefer consistency of the TS SDK with itself than TS SDK with other SDKs. We already took the decision of converting attribute names to TS style in the past, for that reason, and I don't think we should diverge here.
There was a problem hiding this comment.
However, I wouldn't mind making TS SDK itself adopt the rust syntax for that specific purpose. Does that make sense to you?
packages/common/src/logger.ts
Outdated
| /** | ||
| * Log Source prefix for messages emited by the Core SDK's Native Client subsystem. | ||
| */ | ||
| coreClient = 'core/client', | ||
|
|
||
| /** | ||
| * Log Source prefix for messages emited by the Core SDK's Worker subsystem. | ||
| */ | ||
| coreWorker = 'core/worker', |
There was a problem hiding this comment.
Don't think it's worth getting into this resolution. Just emit these as core IMHO, we barely make this distinction.
packages/core-bridge/src/helpers.rs
Outdated
|
|
||
| // Convert from Core's log "targets" (actually full module name, like "temporal_sdk_core::worker::workflow"), | ||
| // to the format used in our JavaScript side logger (i.e. "core/worker/workflow"). | ||
| pub fn rust_package_to_js_style(input: &str) -> String { |
There was a problem hiding this comment.
+1, just slap a core tag on every log record and be done with it.
Leave the Rust style tags verbatim so they're consistent when logging to console or being forwarded to lang.
packages/common/src/logger.ts
Outdated
| /** | ||
| * Log Source value for messages emited from an Activity Worker. | ||
| * This includes: | ||
| * - Activity Lifecycle events; | ||
| * - Activity Task processing events; | ||
| * - Activity Worker's lifecycle events. | ||
| */ | ||
| activityWorker = 'worker/activity', | ||
|
|
||
| /** | ||
| * Log Source value for messages emited from a Workflow Worker. | ||
| * This includes: | ||
| * - Workflow Lifecycle events; | ||
| * - Workflow Activations processing events; | ||
| * - Sink processing issues; | ||
| * - Workflow Worker's lifecycle events. | ||
| */ | ||
| workflowWorker = 'worker/workflow', | ||
|
|
||
| /** | ||
| * Log Source value for messages emited by the Workflow Bundling process, | ||
| * when being executed implicitly by the Workflow Worker. | ||
| */ | ||
| workflowBundler = 'worker/bundler', |
There was a problem hiding this comment.
I'd just call all of this worker so users can easily filter out worker generated logs.
There are tradeoffs between the different approaches so I'm fine with keeping this as well.
mjameswh
force-pushed
the
debug-shutdown-issue
branch
from
245a5f3
to
b0934e1
Compare
|
Flattened log sources: |
…poralio#1391)" This reverts commit 1708529.
What was changed
Add
taskQueuemetadata attribute on all log messages emitted inside the context of a worker. It is there to make it easier to correlate logs when there are multiple workers running in a same process while diagnosing low level issues (e.g. while running tests).Add a
logSourceproperty in metadata. That property indicates where that log message was emitted from; possible values at this time are:workflow: Messages emitted from workflow code (i.e. user code, through theworkflow.logobject);activity: Messages emitted from activity code (i.e. user code, through theactivity.logobject);worker: messages emitted by the worker itself, including workflow and activity life cycle events;core: messages coming from Core SDK.That extra property will make it easier for users to implement fine-grained filtering of messages (eg. show DEBUG level messages for user-code, but only WARN for messages from the worker). Note that this is only about annotating log messages appropriately to make that possible. We don't plan to actually add support for that kind of filtering in the SDK itself. This can easily be implemented by users themselves in their custom runtime logger.
MockActivityEnvironmentno longer defaults the context logger toRuntime.logger, as it is not desirable forRuntimeto get instantiated only for that purpose. Instead, it defaults to sending logs to the console.MockActivityEnvironmentnow accept a logger option, which makes it easier to capture log messages emitted while testing an activity.Fixed a Core-bridge build failure on Windows caused by Node's recent fix for CVE-2024-27980.