Conversation
…ch proto deprecation
…-ga/describe-version-with-tq-stats
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| result := make(map[int32]TaskQueueStats, len(statsByPriorityKey)) | ||
| for priority, stats := range statsByPriorityKey { | ||
| result[priority] = *statsFromResponse(stats) |
There was a problem hiding this comment.
Nil pointer dereference when map contains nil stats value
High Severity
The statsByPriorityKeyFromResponse function dereferences the return value of statsFromResponse(stats) directly. Since statsFromResponse returns nil when its input is nil, if any entry in the statsByPriorityKey map has a nil value for stats, the expression *statsFromResponse(stats) will cause a nil pointer dereference panic.
| tqInfo.Stats.ApproximateBacklogAge.Nanoseconds() > 0 && | ||
| tqInfo.Stats.TasksAddRate > 0 && | ||
| tqInfo.Stats.TasksDispatchRate == 0 && // zero task dispatch due to no pollers | ||
| tqInfo.Stats.BacklogIncreaseRate > 0) { |
There was a problem hiding this comment.
Test checks aggregate stats instead of per-priority stats
Medium Severity
When checking the default priority key (3), the test verifies tqInfo.Stats (aggregate task queue stats) instead of priorityKeyStats (per-priority stats). The comment states the intent is to check the priority key's ApproximateBacklogCount, but the code checks the aggregate stats. This mismatch means the test doesn't actually validate that per-priority stats are correctly reported for the default priority key.
| // WorkerDeploymentTaskQueueInfo describes properties of the Task Queues involved | ||
| // in a Deployment Version. | ||
| // | ||
| // NOTE: Experimental |
There was a problem hiding this comment.
I am thinking this worker deployment client maybe should step back and have proper cross-SDK design. We are suffering some parity issues leaving this long-experimental thing inside of a single SDK. I think we may be able to approve the PR though. Is this just for CLI? Or is this for general user use and completeness sake?
There was a problem hiding this comment.
for the CLI.
I could rewrite the CLI to not depend on the Go SDK (and I think we maybe should implement CLI from gRPC in the future), but that involves a lot of code deletion, so I wanted to propose this change instead.
2 participants
What was changed
Expose Task Queue Stats in DescribeVersion
Why?
Mainly so that I can use this in the CLI, since the CLI describe-version already depends on Go SDK.
Worker controller, which also uses Go SDK, might also use this.
Checklist
Closes
How was this tested:
New functional test
Note
Medium Risk
Expands an experimental client API and wires through optional task-queue stats from server responses; risk is mainly compatibility/behavioral around nil vs populated stats and server support for the new fields.
Overview
WorkerDeploymentHandle.DescribeVersionnow supports an opt-inReportTaskQueueStatsflag and returnsTaskQueueInfoscontaining each polled task queue along with optionalStatsandStatsByPriorityKeybacklog metrics.This adds proto-to-SDK conversion helpers for per-priority stats, deprecates
WorkerDeploymentVersionInfo.TaskQueuesInfosin favor of the new response field, and includes a new functional test that verifies stats are returned only when requested (with and without workflow priority keys).Written by Cursor Bugbot for commit d6f3f68. This will update automatically on new commits. Configure here.