Update DescribeTaskList to return Isolation Group metrics #6580
Conversation
natemort
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Update emaFixedWindowQPSTracker to support tracking several qps metrics and use it to track the AddTask QPS per isolation group. Update DescribeTaskList to return the AddTask QPS and number of pollers per isolation group. Additionally include the global AddTask QPS.
| } | ||
|
|
||
| func (r *emaFixedWindowQPSTracker) getOrCreate(group string) *emaFixedWindowState { | ||
| res, ok := r.groups.Load(group) |
There was a problem hiding this comment.
nit: groups are never cleaned up. when an isolation group is gone (decommissioned zone) it will stay in the map. There is no unexpected behavior due to these leftover groups so it's a minor nit. We have other bigger gaps to make "isolation group auto discovery" a true restart-free feature.
| isolationGroupMetrics := make(map[string]*types.IsolationGroupMetrics, len(isolationGroups)) | ||
| for _, group := range isolationGroups { | ||
| isolationGroupMetrics[group] = &types.IsolationGroupMetrics{ | ||
| NewTasksPerSecond: c.qpsTracker.GroupQPS(group), |
There was a problem hiding this comment.
nit: I wonder if it matters that much but having a c.qpsTracker.Snapshot() would help with accuracy of NewTasksPerSeconds in the response. Atomically gathering qps info of all groups would help us avoid future surprises.
There was a problem hiding this comment.
With the current implementation we can't actually get a snapshot. With AddTask being high QPS and this being just metric collection I wanted to minimize the performance impact. sync.Map is lock-free once the values have been written and unless we introduce a lock to serialize the writes we can't actually collect a consistent state.
I think it's fine if these numbers are a little fuzzy.
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes