Rework persistenceMetricClients to generated approach #5560
Conversation
3vilhamster
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
3vilhamster
force-pushed
the
metric-clients-tests
branch
from
41a398a to
b42f3bd
Compare
| @@ -53,11 +53,11 @@ require ( | |||
| go.uber.org/thriftrw v1.29.2 | |||
| go.uber.org/yarpc v1.70.3 | |||
| go.uber.org/zap v1.13.0 | |||
| golang.org/x/net v0.17.0 | |||
| golang.org/x/net v0.19.0 | |||
There was a problem hiding this comment.
should this be in a separate PR?
There was a problem hiding this comment.
That comes from
go get golang.org/x/exp
Not sure how to separate them apart. All x/ packages are linked to one another.
There was a problem hiding this comment.
ah, ok I didn't see exp being added. Is golang.org/x/exp used in this change?
There was a problem hiding this comment.
yes. It has a nice helper of maps.Keys
| err = c.call({{$scopeName}}, op, metrics.DomainTag(domainName)) | ||
| } | ||
| } | ||
| {{ else -}} |
There was a problem hiding this comment.
is an early return possible rather than having the else branch?
There was a problem hiding this comment.
Unfortunately, templates are really hard to read/format with continue/break statements.
3vilhamster
force-pushed
the
metric-clients-tests
branch
from
b42f3bd to
6cf018d
Compare
| // PersistenceVisibilityDeleteUninitializedWorkflowExecutionScope tracks DeleteUninitializedWorkflowExecution calls made by service to persistence layer | ||
| PersistenceVisibilityDeleteUninitializedWorkflowExecutionScope | ||
| // PersistenceDeleteUninitializedWorkflowExecutionScope tracks DeleteUninitializedWorkflowExecution calls made by service to persistence layer | ||
| PersistenceDeleteUninitializedWorkflowExecutionScope |
There was a problem hiding this comment.
I think you'll need to keep the word visibility on this one? This is applying to different table then the normal executions table. It's for when CAAS us being used to list workflows instead of elasticsearch (we have visibility table for this)
There was a problem hiding this comment.
It is used only inside VisibiltyManager, so Visibility prefix is not required. I'm thinking of separating this metrics by manager so it would be easier to track which component emits this, but for now it should be fine.
| } | ||
|
|
||
| func (p *base) updateErrorMetricPerDomain(scope int, err error, scopeWithDomainTag metrics.Scope) { | ||
| switch err.(type) { |
There was a problem hiding this comment.
Not needed in this diff, but we'll need to change this to use errors.Is/As at some point. Your change will make this a lot easier though.
| @@ -68,6 +68,7 @@ require ( | |||
| require ( | |||
| github.com/google/go-cmp v0.5.9 | |||
| github.com/jonboulle/clockwork v0.4.0 | |||
| golang.org/x/exp v0.0.0-20231226003508-02704c960a9b | |||
There was a problem hiding this comment.
just because I'm being dumb, what was this required for?
There was a problem hiding this comment.
golang.org/x/exp/maps.Keys.
3vilhamster
force-pushed
the
metric-clients-tests
branch
from
6cf018d to
4e74ca0
Compare
| metricsScope.IncCounter(metrics.PersistenceRequestsPerDomain) | ||
| } else { | ||
| metricsScope.IncCounter(metrics.PersistenceRequests) |
There was a problem hiding this comment.
Not related to this change but I don't like this pattern of defining multiple counters for same thing. For example I want to introduce shard-id tag and it doesn't make sense to create a new counter PersistenceRequestsPerDomainPerShard :) Ideally we could have a single counter for persistence latency with all the dimensions in it and when a dimension is not applicable it can just be empty.
| @@ -38,6 +38,17 @@ | |||
| //go:generate gowrap gen -g -p . -i DomainManager -t ./wrappers/templates/errorinjector.tmpl -o wrappers/errorinjectors/domain.go | |||
| //go:generate gowrap gen -g -p . -i QueueManager -t ./wrappers/templates/errorinjector.tmpl -o wrappers/errorinjectors/queue.go | |||
|
|
|||
| // Generate metered wrappers. | |||
There was a problem hiding this comment.
thanks for tackling this
3vilhamster
force-pushed
the
metric-clients-tests
branch
from
4e74ca0 to
d422e70
Compare
What changed?
I've reworked persistenceMetricClients into files that are generated automatically. This ensures unified approach to all persistence metered method and eliminates manual job to keep them in sync. Also, all metrics/logs are now follow the same logic.
All managers except for ExecutionManager follow same logic with some minor logical branches. ExecutionManager is very different, so I've extracted it in a separate template.
In the next PR I'll remove old clients and cleanup the logic.
Why?
To remove manual work to keep wrappers in sync with persistence managers implementation and unify approach to metering and logging.
How did you test it?
I've tested that I emit the same metrics/logs between old and new logic.
Potential risks
Release notes
Documentation Changes