Add StatsReporter component to estimate QPS #6286
Conversation
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -258,6 +264,8 @@ func (c *taskListManagerImpl) AddTask(ctx context.Context, params AddTaskParams) | |||
| if params.ForwardedFrom == "" { | |||
| // request sent by history service | |||
| c.liveness.MarkAlive() | |||
| c.qpsReporter.ReportCounter(1) | |||
| c.scope.UpdateGauge(metrics.EstimatedAddTaskQPSGauge, c.qpsReporter.QPS()) | |||
There was a problem hiding this comment.
could we pass c.scope to the qpsReporter so it can handle this UpdateGauge operation as well?
There was a problem hiding this comment.
I'll not do that for now. This is a new component and I want to keep its function as minimal as possible so that we could iterate on this easily.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| c.qpsTracker.ReportCounter(1) | ||
| c.scope.UpdateGauge(metrics.EstimatedAddTaskQPSGauge, c.qpsTracker.QPS()) |
There was a problem hiding this comment.
it would be better if this was one line and qpsTracker also emits the metric.
| defer r.wg.Done() | ||
| ticker := r.timeSource.NewTicker(r.bucketInterval) | ||
| defer ticker.Stop() | ||
|
|
There was a problem hiding this comment.
nit: suggest putting a defer/recover in case of any panics
What changed?
Why?
We want to change the behavior of scalable tasklists based on AddTask QPS.
How did you test it?
unit tests, and verified in staging2
Potential risks
Release notes
Documentation Changes