Add TaskQueueStats to DescribeTaskQueueEnhanced #1553

ShahabT · 2024-07-19T07:25:45Z

What was changed

Add TaskQueueStats to DescribeTaskQueueEnhanced to report backlog info and task add/dispatch rate.

Why?

Useful for worker scaling.

Checklist

Closes
How was this tested:

Functional test added.

Any docs updates needed?

Yes, will do in a separate PR.

cretz

Mostly LGTM, just want to wait until it is available to users server side so we can test it like users will

cretz · 2024-07-19T12:16:27Z

client/client.go

@@ -345,24 +345,25 @@ type (
 	TaskQueueVersionSelection = internal.TaskQueueVersionSelection

 	// TaskQueueDescription is the response to [Client.DescribeTaskQueueEnhanced].
-	// WARNING: Worker versioning is currently experimental.


I assume this is being removed because this stuff is now considered GA? Isn't this all part of worker versioning technically the stats are per build ID, or is this RPC and the way it comes back considered GA?

That's something I wanted to discuss actually. The versioning stuff are not GA for sure. I'd like to be able to say the Enhanced DecribeTaskQueue API parts that don't have to do with versioning is GA. It's a little tricky because of the versions_info map.

What do you guys think of a change like below to the API? It removes the empty string ugliness, also it allows us to keep the build_id related parts experimental and unversioned part GA.

cc @cretz @Quinn-With-Two-Ns

considering to rename TaskQueueVersionInfo to just TaskQueueInfo as well. That would fit better with unversioned.

That API change works for me, but the empty string does too. I have no strong opinion, though I could understand not wanting to go through a cycle of API changes just to be able to split up the "unstable" comment. Remember, we can't reasonably release something in the SDK until a user can actually use it in the server (we test the same way our users do to make sure things work for us the same way they do for them).

cretz · 2024-07-19T12:18:06Z

internal/internal_versioning_client.go

@@ -174,6 +216,7 @@ func (o *DescribeTaskQueueEnhancedOptions) validateAndConvertToProto(namespace s
 		TaskQueueTypes:         taskQueueTypes,
 		ReportPollers:          o.ReportPollers,
 		ReportTaskReachability: o.ReportTaskReachability,
+		ReportStats: o.ReportStats,


Did this file get properly go fmtd?

cretz · 2024-07-19T12:18:31Z

internal/internal_versioning_client.go

@@ -174,6 +216,7 @@ func (o *DescribeTaskQueueEnhancedOptions) validateAndConvertToProto(namespace s
 		TaskQueueTypes:         taskQueueTypes,
 		ReportPollers:          o.ReportPollers,
 		ReportTaskReachability: o.ReportTaskReachability,
+		ReportStats: o.ReportStats,


Is this available in the current dev server? We won't want to make this available until users can actually use it and we have tested it the same way they might.

internal/internal_versioning_client.go

Quinn-With-Two-Ns · 2024-08-08T15:46:02Z

internal/internal_worker.go

@@ -1608,7 +1608,14 @@ func NewAggregatedWorker(client *WorkflowClient, taskQueue string, options Worke
 		tagNamespace, client.namespace,
 		tagTaskQueue, taskQueue,
 		tagWorkerID, workerParams.Identity,
+		tagBuildID, workerParams.getBuildID(),


Are you adding tagBuildID twice here?

# Conflicts: # contrib/datadog/go.sum # contrib/opentracing/go.mod # contrib/opentracing/go.sum # contrib/tally/go.mod # go.mod # go.sum # internal/cmd/build/go.sum # test/go.mod # test/go.sum

ShahabT · 2024-08-13T17:18:00Z

@cretz @Quinn-With-Two-Ns it seems we are unblocked, dependency-wise. Can we get this PR merged?

Quinn-With-Two-Ns · 2024-08-13T17:42:03Z

test/worker_versioning_test.go

+	ts.NoError(err)
+
+	// Give time for the task to be sent to the TQ
+	time.Sleep(500 * time.Millisecond)


We should try to avoid sleep in tests, it is very flaky, we should wait for the condition to become true

replaced it with eventually.

Quinn-With-Two-Ns

LGTM, good to merge once CI is passing. If you need any help there please reach out

ShahabT · 2024-08-14T00:50:01Z

internal/internal_versioning_client.go

+	return &TaskQueueStats{
+		ApproximateBacklogCount: stats.GetApproximateBacklogCount(),
+		ApproximateBacklogAge:   stats.GetApproximateBacklogAge().AsDuration(),
+		TasksAddRate:            stats.TasksAddRate,
+		TasksDispatchRate:       stats.TasksDispatchRate,


@cretz @Quinn-With-Two-Ns, realizing that eager activities are enabled by default, I'm now inclined to remove separate add/dispatch rate fields and replace both with a BacklogIncreaseRate field. Reason being that the individual rates can be very off an easily mislead people because they'd not include eager activities and sticky workflow tasks.
wdyt?

(The API can remain the same, or I can change that in the next release as well by deprecating the separate rates and adding a new backlog_increase_rate filed.)

Suggested change

return &TaskQueueStats{

ApproximateBacklogCount: stats.GetApproximateBacklogCount(),

ApproximateBacklogAge: stats.GetApproximateBacklogAge().AsDuration(),

TasksAddRate: stats.TasksAddRate,

TasksDispatchRate: stats.TasksDispatchRate,

return &TaskQueueStats{

ApproximateBacklogCount: stats.GetApproximateBacklogCount(),

ApproximateBacklogAge: stats.GetApproximateBacklogAge().AsDuration(),

ApproximateBacklogIncreaseRate: stats.TasksAddRate - stats.TasksDispatchRate,

IMO we should give the same thing that the underlying API gives. If there are concerns about user confusion, they can be documented. If we must also provide helpers or derived fields we can (and we should document how they are derived). The SDK should not deviate so far from the API on these things (many users will use our API directly e.g. via HTTP, if it's not too confusing for them it's not too confusing for SDK API users).

OK. I kept the separate fields with enough documentation and add a derivative field as well.

Add TaskQueueStats to DescribeTaskQueueEnhanced

a810f84

ShahabT requested a review from a team as a code owner July 19, 2024 07:25

cretz reviewed Jul 19, 2024

View reviewed changes

reformat

6128711

Quinn-With-Two-Ns reviewed Jul 22, 2024

View reviewed changes

internal/internal_versioning_client.go Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Aug 8, 2024

View reviewed changes

ShahabT added 2 commits August 13, 2024 10:10

Merge remote-tracking branch 'origin/master' into shahab/backlog-stats

8276c4f

# Conflicts: # contrib/datadog/go.sum # contrib/opentracing/go.mod # contrib/opentracing/go.sum # contrib/tally/go.mod # go.mod # go.sum # internal/cmd/build/go.sum # test/go.mod # test/go.sum

Address comments

8133f27

Fix test

4256c41

Quinn-With-Two-Ns reviewed Aug 13, 2024

View reviewed changes

ShahabT added 2 commits August 13, 2024 10:47

Get rid of time.sleep in the test

d2cf1a1

Attempt to fix test in CI

23d74a5

Quinn-With-Two-Ns approved these changes Aug 13, 2024

View reviewed changes

Fix test failure and add Eager comments

7bd88f9

ShahabT commented Aug 14, 2024

View reviewed changes

ShahabT added 3 commits August 14, 2024 09:28

Convert BacklogIncreaseRate to a field

c98f2c8

improve tests

40eb7f1

Merge remote-tracking branch 'origin/master' into shahab/backlog-stats

0937183

cretz approved these changes Aug 14, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into shahab/backlog-stats

7cd1b27

cretz mentioned this pull request Aug 15, 2024

High-level task queue describe call with default-build-ID stats temporalio/features#525

Open

8 tasks

ShahabT added 2 commits August 15, 2024 11:39

improve tests

4f5080c

skip test in 1.24

874aa04

ShahabT merged commit b5db2b7 into master Aug 16, 2024
14 checks passed

ShahabT deleted the shahab/backlog-stats branch August 16, 2024 00:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TaskQueueStats to DescribeTaskQueueEnhanced #1553

Add TaskQueueStats to DescribeTaskQueueEnhanced #1553

ShahabT commented Jul 19, 2024

cretz left a comment •

edited

Loading

cretz Jul 19, 2024

ShahabT Jul 19, 2024

ShahabT Jul 19, 2024

cretz Jul 19, 2024 •

edited

Loading

cretz Jul 19, 2024

cretz Jul 19, 2024

Quinn-With-Two-Ns Aug 8, 2024

ShahabT Aug 13, 2024

ShahabT commented Aug 13, 2024

Quinn-With-Two-Ns Aug 13, 2024

ShahabT Aug 13, 2024

Quinn-With-Two-Ns left a comment

ShahabT Aug 14, 2024

cretz Aug 14, 2024 •

edited

Loading

ShahabT Aug 14, 2024

Add TaskQueueStats to DescribeTaskQueueEnhanced #1553

Add TaskQueueStats to DescribeTaskQueueEnhanced #1553

Conversation

ShahabT commented Jul 19, 2024

What was changed

Why?

Checklist

cretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShahabT commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment •

edited

Loading

cretz Jul 19, 2024 •

edited

Loading

cretz Aug 14, 2024 •

edited

Loading