Refactor frontend API handler and use generated code to emit metrics #5639
Conversation
Shaddoll
force-pushed
the
metered
branch
2 times, most recently
from
e394dea to
11e23e1
Compare
Pull Request Test Coverage Report for Build 018d5dcb-1dcd-4c0d-b966-8acfb4c243d2
💛 - Coveralls |
Shaddoll
requested review from
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -228,17 +226,12 @@ func (wh *WorkflowHandler) Health(ctx context.Context) (*types.HealthStatus, err | |||
| // acts as a sandbox and provides isolation for all resources within the domain. All resources belongs to exactly one | |||
| // domain. | |||
| func (wh *WorkflowHandler) RegisterDomain(ctx context.Context, registerRequest *types.RegisterDomainRequest) (retError error) { | |||
| defer func() { log.CapturePanic(recover(), wh.GetLogger(), &retError) }() | |||
There was a problem hiding this comment.
Do we have a global panic handler somewhere? I think handling panic is not part of metering.
There was a problem hiding this comment.
no, we don't. we can create a separate one later if we need that
There was a problem hiding this comment.
FYI: Panics in YARPC handlers are automatically caught and returned/logged as an error. This ensures that your process continues to run.
As of YARPC v1.43, YARPC now includes a metrics counter for panics.
So we can delete this completely.
| } | ||
|
|
||
| func (h *apiHandler) CountWorkflowExecutions(ctx context.Context, cp1 *types.CountWorkflowExecutionsRequest) (cp2 *types.CountWorkflowExecutionsResponse, err error) { | ||
| defer func() { log.CapturePanic(recover(), h.logger, &err) }() |
There was a problem hiding this comment.
this looks much better
| tags := []tag.Tag{tag.WorkflowHandlerName("{{$method.Name}}")} | ||
| {{- $scope := printf "metrics.Frontend%sScope" $method.Name}} | ||
| {{- $domainMetricTag := "metrics.DomainUnknownTag()"}} | ||
| {{- if not (has $method.Name $nonDomainSpecificAPIs) }} |
There was a problem hiding this comment.
not a problem with this PR, but the fact that this there are ... kind of groupings and yet it's annoyingly hard to match them out makes me feel like we should maybe separate them out more at a struct / interface level in the future.
this is fine, but I worry that in future templating like this will get messy beyond the pont of being readable
What changed?
Why?
Tidy the messy code and make it maintainable
How did you test it?
unit tests
Potential risks
Release notes
Documentation Changes