Update frontend poller metrics to include tasklist tag #6237
Conversation
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| scope := h.metricsClient.Scope(metrics.FrontendPollForDecisionTaskScope).Tagged(metrics.DomainTag(pp1.GetDomain())).Tagged(metrics.GetContextTags(ctx)...) | ||
| scope.IncCounter(metrics.CadenceRequests) | ||
| sw := scope.StartTimer(metrics.CadenceLatency) | ||
| scope := common.NewPerTaskListScope(pp1.Domain, pp1.TaskList.GetName(), pp1.TaskList.GetKind(), h.metricsClient, metrics.FrontendPollForDecisionTaskScope).Tagged(metrics.GetContextTags(ctx)...) | ||
| scope.IncCounter(metrics.CadenceRequestsPerTaskList) | ||
| sw := scope.StartTimer(metrics.CadenceLatencyPerTaskList) |
There was a problem hiding this comment.
this might deserve a rollup value for the timer, as we'll lose domain-wide aggregates with this change.
(or is that somehow already handled? I find it super hard to figure that out tbh)
There was a problem hiding this comment.
Agree. I also though that might be a problem and based on this line it looks like there's rollup defined for this metric.
There was a problem hiding this comment.
UPDATED! Also fixed a bug in metrics package.
There was a problem hiding this comment.
ah, just doing a manual non-TL-containing timer. yea, that seems fine 👍
| func (m *metricsScope) Tagged(tags ...Tag) Scope { | ||
| domainTagged := false | ||
| domainTagged := m.isDomainTagged | ||
| tagMap := make(map[string]string, len(tags)) |
There was a problem hiding this comment.
aaah. yeah, this makes sense - otherwise we'd lose the value when we make a cached scope with a domain tag. 👍
There was a problem hiding this comment.
Yep, looks good now. thank you!
What does the isDomainTagged value do, anyway? The fix looks definitely correct, I'm just not familiar with what it actually does.
edit: from chat messages:
if it's domain tagged, it emits a summary metric with
domain:all
so yea, that's definitely a good fix to have. thanks!
What changed?
Update frontend poller metrics to include tasklist tag
Why?
Improve observability
How did you test it?
Potential risks
Release notes
Documentation Changes