Enable Worker heartbeating, plumb plugin names to core, update Core to 44a6576 #1818
Conversation
yuandrew
force-pushed
the
worker-heartbeat
branch
from
1557d50 to
de87933
Compare
yuandrew
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
chris-olszewski
left a comment
chris-olszewski
left a comment
There was a problem hiding this comment.
Only required change is fixing up tests. Rest are light suggestions or questions.
packages/core-bridge/src/worker.rs
Outdated
| } | ||
| } | ||
| CompleteWfError::WorkflowNotEnabled => { | ||
| BridgeError::UnexpectedError(format!("{err}")) |
There was a problem hiding this comment.
No strong opinions on this 🤷
| BridgeError::UnexpectedError(format!("{err}")) | |
| BridgeError::UnexpectedError(err.to_string()) |
| .into_iter() | ||
| .map(|name| PluginInfo { | ||
| name, | ||
| version: String::new(), |
There was a problem hiding this comment.
Just for my own knowledge, is version from the past or not yet implemented?
There was a problem hiding this comment.
Good question, I don't know. haha. The API was originally implemented with version, but not sure what the intent was. Will follow up on this.
| taskTypes: { | ||
| enableWorkflows, | ||
| enableLocalActivities, | ||
| enableRemoteActivities: opts.enableNonLocalActivities && opts.activities.size > 0, |
There was a problem hiding this comment.
That's basically saying:
enableRemoteActivities: (enableWorkflows && opts.activities.size > 0) && opts.activities.size > 0
Which is incorrect. We don't need workflows to enable remote activities. I'm surprised tests passed with this.
There was a problem hiding this comment.
I don't think that's right, you're referring to enableLocalActivities, but I'm using opts.enableNonLocalActivities here
| max_activities_per_second: Option<f64>, | ||
| max_task_queue_activities_per_second: Option<f64>, | ||
| shutdown_grace_time: Option<Duration>, | ||
| plugins: Vec<String>, |
There was a problem hiding this comment.
I think we should consider deduping plugins if we don't already. Depending on plugin implementation, it may be very normal to configure multiple of the same plugin on the same worker (maybe each has different settings).
This should be done in every SDK IMO too if we don't already (could consider making that Vec a Set on the Core side).
There was a problem hiding this comment.
good point, a set would ensure deduping. Created temporalio/sdk-core#1072
| const { metrics, noTemporalPrefixForMetrics } = options.telemetryOptions ?? {}; // eslint-disable-line deprecation/deprecation | ||
| const [logger, logExporter] = compileLoggerOptions(options); | ||
|
|
||
| const heartbeatMillis = msToNumber(options.workerHeartbeatInterval ?? '60s'); |
There was a problem hiding this comment.
Bug: Missing validation for heartbeat interval range
The documentation for workerHeartbeatInterval states "Accepted range is between 1s and 60s" but the implementation at line 378 doesn't validate this range. The code only checks if the value is 0 (to disable heartbeating) but accepts any other value without validation. Users could pass invalid values like 100ms or 2m which would be silently accepted, potentially causing unexpected behavior in worker heartbeating.
Additional Locations (1)
yuandrew
changed the title
4 participants
What was changed
WorkerTaskTypesbased on if workflows/activities/nexus handlers are registered to the workersdk-coreto44a6576Why?
New feature!
Checklist
Closes [Feature Request] Enable Worker Heartbeating #1810
How was this tested:
Manually tested that heartbeats with a simple plugin from the worker and client both show up in plugins section of the worker heartbeat.
Also tested setting
workerHeartbeatInterval: nulldisables heartbeating for the workerNote
Adds configurable worker heartbeating, passes explicit task types and plugin names to Core, updates client/worker bridge APIs (incl. TLS/options), and bumps Core deps with minor error mapping tweaks.
worker_heartbeat_interval_millisand sets Coreheartbeat_interval.WorkflowNotEnabled,ActivityNotEnabled,NexusNotEnabled) toUnexpectedErrorstrings.enable_non_local_activitieswithtask_typesand forwardsplugins(asPluginInfoset).TlsOptions/ClientTlsOptions; uses newClientOptions::builder()withmaybe_*setters.RuntimeOptionsgainsworkerHeartbeatIntervalMillis;WorkerOptionsgainstaskTypesandplugins; renamenewRuntimeparam.workerHeartbeatIntervaloption; compiles toworkerHeartbeatIntervalMillis.runtimeOptionsinstead oftelemetryOptionswhen creating runtime and configuring metrics.taskTypesfrom registered workflows/activities/nexus; forwardsplugins.taskTypes, andplugins.bon, bumpdarling, updatetemporalio-clientdeps).Written by Cursor Bugbot for commit 034eb3d. This will update automatically on new commits. Configure here.