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.
| 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
|
@yuandrew I see you renamed some bridge struct fields, but then reverted due to build issues. I would have kept it, but rename the corresponding TS side structures. The bridge layer only uses internal types, never directly linked to user facing APIs, so no risk. See https://github.com/temporalio/sdk-typescript/tree/pr/yuandrew/1818-mjameshwh-additions. |
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 runtime-configurable worker heartbeats, forwards plugin names in worker heartbeats, introduces task-type gating, and migrates client TLS/options to new Core APIs.
worker_heartbeat_interval_millisruntime option; plumb through to Coreheartbeat_interval.WorkerTaskTypesand wire to Core (task_types), replacingenableNonLocalActivitiesfor capability gating.pluginsto Core asPluginInfoset for inclusion in heartbeats.UnexpectedError.TlsOptions,client_tls_options) and switch to new Core builders (maybe_*setters).native.tsinterfaces to new TLS, runtime, and worker options.workerHeartbeatIntervalMillisin bridge tests.Written by Cursor Bugbot for commit fabb505. This will update automatically on new commits. Configure here.