Skip to content

Conversation

ychebotarev
Copy link
Contributor

@ychebotarev ychebotarev commented May 28, 2025

MAYBE FOR MERGE.
The goal of this PR is to review the proposed APIs.
It will not necessary be merged, treat it as a part of Design review.

What changed?
2 new APIs added:

  • WorkerHeartbeat
  • ListWorkerStatus

Why?
Part of the worker visibility work.

Note:
Currently metrics are not supported as a part of List operation query. But it may happen, based on user feedback.

int64 memory_usage_bytes = 19;

// A Workflow Task found a cached Workflow Execution to run against.
int32 sticky_cache_hit = 20;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these counters since process start or since the last heartbeat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now this is something we already have: https://docs.temporal.io/references/sdk-metrics
So I guess since process start.

Copy link
Member

Choose a reason for hiding this comment

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

We do need clear delineation in docs between values that are reset every heartbeat and ones that are cumulative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I will add 'total/last/current' qualifies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

keeping this open

@ychebotarev ychebotarev requested review from cretz and dnr June 2, 2025 17:05
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

I think we're pretty much there. Only a few non-doc changes here.

IMO, let's add this to the polls too, since I know that's something we're going to want to do in the first SDK implementation anyway.

@ychebotarev ychebotarev requested a review from Sushisource June 3, 2025 02:31
@ychebotarev ychebotarev closed this Jun 4, 2025
ychebotarev added a commit that referenced this pull request Jun 5, 2025
<!-- Describe what has changed in this PR -->
**What changed?**
What changed?
* 2 new APIs added:
   * WorkerHeartbeat
   * ListWorkerStatus
* WorkerInfo information added to:
   * ShutDownWorker request
   * Poll(Activity|Nexus|Workflow)TaskQueue requests
   
This is "for merge".
Here is a previous PR, where we discuss this at length:
#599

<!-- Tell your future self why have you made these changes -->
**Why?**
Part of the worker visibility work. Workers should be able to report
their status.
Users should be able to get information about workers.

<!-- Are there any breaking changes on binary or code level? -->
**Breaking changes**
Yes - new API is introduce.

<!-- If this breaks the Server, please provide the Server PR to merge
right after this PR was merged. -->
**Server PR**
[Server PR](temporalio/temporal#7870)
temporal-cicd bot pushed a commit to temporalio/api-go that referenced this pull request Jun 5, 2025
<!-- Describe what has changed in this PR -->
**What changed?**
What changed?
* 2 new APIs added:
   * WorkerHeartbeat
   * ListWorkerStatus
* WorkerInfo information added to:
   * ShutDownWorker request
   * Poll(Activity|Nexus|Workflow)TaskQueue requests

This is "for merge".
Here is a previous PR, where we discuss this at length:
temporalio/api#599

<!-- Tell your future self why have you made these changes -->
**Why?**
Part of the worker visibility work. Workers should be able to report
their status.
Users should be able to get information about workers.

<!-- Are there any breaking changes on binary or code level? -->
**Breaking changes**
Yes - new API is introduce.

<!-- If this breaks the Server, please provide the Server PR to merge
right after this PR was merged. -->
**Server PR**
[Server PR](temporalio/temporal#7870)
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.

7 participants