Update matching to emit metric for tasklist backlog size #5448
Conversation
| @@ -191,7 +191,7 @@ func TestDescribeTaskList(t *testing.T) { | |||
| require.NotNil(t, taskListStatus) | |||
| require.Zero(t, taskListStatus.GetAckLevel()) | |||
| require.Equal(t, taskCount, taskListStatus.GetReadLevel()) | |||
| require.Equal(t, taskCount, taskListStatus.GetBacklogCountHint()) | |||
| require.Equal(t, int64(0), taskListStatus.GetBacklogCountHint()) | |||
There was a problem hiding this comment.
Not sure I understand. In the test 3 tasks are pushed to the tasklist. Why a backlog is still 0? Or are they immediately processed?
I suggest adding a test that verifies the backlog count/metric emition with tally.TestScope.
There was a problem hiding this comment.
The original value is in-memory backlog, and the new value is the in-db backlog. The test generates the backlog by changing the in-memory backlog, so after the change, the value is changed.
service/matching/taskListManager.go
Outdated
| @@ -459,7 +459,7 @@ func (c *taskListManagerImpl) DescribeTaskList(includeTaskListStatus bool) *type | |||
| response.TaskListStatus = &types.TaskListStatus{ | |||
| ReadLevel: c.taskAckManager.GetReadLevel(), | |||
| AckLevel: c.taskAckManager.GetAckLevel(), | |||
| BacklogCountHint: c.taskAckManager.GetBacklogCount(), | |||
| BacklogCountHint: c.db.BacklogCount(), | |||
There was a problem hiding this comment.
who is calling DescribeTaskList() and how will this BacklogCountHint be used?
There was a problem hiding this comment.
It's called by cadence-web and cadence-cli. And the value is only used to tell if there is backlog.
There was a problem hiding this comment.
based on your other comment this value will be potentially 5m old. If that's sufficient for now the change makes sense. otherwise we can maybe keep an accurate in-memory counter as tasks are added/removed from a tasklist in the future.
There was a problem hiding this comment.
Well, after some thought, I decide to not use the cached value. The api is not called frequently, and I don't think it will increase db load a lot.
Shaddoll
force-pushed
the
tasklist-size
branch
from
d705b15
to
3445037
Compare
service/matching/taskListManager.go
Outdated
| @@ -459,7 +459,7 @@ func (c *taskListManagerImpl) DescribeTaskList(includeTaskListStatus bool) *type | |||
| response.TaskListStatus = &types.TaskListStatus{ | |||
| ReadLevel: c.taskAckManager.GetReadLevel(), | |||
| AckLevel: c.taskAckManager.GetAckLevel(), | |||
| BacklogCountHint: c.taskAckManager.GetBacklogCount(), | |||
| BacklogCountHint: c.db.BacklogCount(), | |||
There was a problem hiding this comment.
based on your other comment this value will be potentially 5m old. If that's sufficient for now the change makes sense. otherwise we can maybe keep an accurate in-memory counter as tasks are added/removed from a tasklist in the future.
Shaddoll
force-pushed
the
tasklist-size
branch
from
39ea5bb
to
d12698f
Compare
Shaddoll
requested review from
neil-xie,
Groxx,
shijiesheng,
agautam478,
jakobht,
sankari165,
dkrotx and
demirkayaender
as code owners
What changed?
Update matching to emit metric for tasklist backlog size
Why?
To provide visibility of tasklist backlog size
How did you test it?
local test
Potential risks
Release notes
Documentation Changes