History Handler cleanup #9125
History Handler cleanup #9125
Conversation
| if strings.HasPrefix(info.FullMethod, chasmRequestPrefix) { | ||
| defer metrics.CapturePanic(i.logger, i.metricsHandler, &retError) | ||
| } | ||
| defer metrics.CapturePanic(i.logger, i.metricsHandler, &retError) |
There was a problem hiding this comment.
Why in the CHASM interceptor? Not where I would look for this necessarily. Is a separate, dedicated interceptor unreasonable (e.g. perf reasons)?
There was a problem hiding this comment.
Perf reasons. Agree it's not the ideal place for it.
There was a problem hiding this comment.
Could add a comment in the file to redirect readers to the right place.
| ctx context.Context, | ||
| _ *historyservice.DeepHealthCheckRequest, | ||
| ) (_ *historyservice.DeepHealthCheckResponse, retError error) { | ||
| defer log.CapturePanic(h.logger, &retError) |
There was a problem hiding this comment.
I vaguely remember from my attempt 2+ years ago that this changed the panic details somehow; but I don't recall the details ... okay, I ran an experiment and there's one meaningful difference: the current version has a service log tag that the new one doesn't. Now, is that relevant is the question.
There was a problem hiding this comment.
Hmm... yeah, I can add the tag to the interceptor's logger.
| func (h *Handler) StreamWorkflowReplicationMessages( | ||
| server historyservice.HistoryService_StreamWorkflowReplicationMessagesServer, | ||
| ) (retError error) { | ||
| defer metrics.CapturePanic(h.logger, h.metricsHandler, &retError) |
There was a problem hiding this comment.
Since this is a streaming endpoint; is the CHASM interceptor applied here?
2 participants
What changed?