Add metric when history replication message is too large #6558
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
service/history/handler/handler.go
Outdated
| @@ -1601,6 +1601,9 @@ func (h *handlerImpl) GetReplicationMessages( | |||
|
|
|||
| size := proto.FromReplicationMessages(tasks).Size() | |||
| if (responseSize + size) >= maxResponseSize { | |||
| metricsScope := h.GetMetricsClient().Scope(metrics.ReplicationMessageTooLargeScope, metrics.ShardIDTag(int(shardID))) | |||
There was a problem hiding this comment.
let's reuse the metric scope returned in line 1558 with shard id tag included
There was a problem hiding this comment.
changed. I only added the shard tag inside of that if statement.
Codecov ReportAll modified and coverable lines are covered by tests ✅
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
service/history/handler/handler.go
Outdated
| @@ -1601,6 +1601,9 @@ func (h *handlerImpl) GetReplicationMessages( | |||
|
|
|||
| size := proto.FromReplicationMessages(tasks).Size() | |||
| if (responseSize + size) >= maxResponseSize { | |||
| metricsScope.Tagged(metrics.ShardIDTag(int(shardID))) | |||
There was a problem hiding this comment.
it would be better to have a gauge metric instead of counter which would have value responseSize + size
There was a problem hiding this comment.
I thought about that but:
- Our purpose was to make an alert that can catch things when there are a lot of "replication message too large" warnings, so I think adding a counter could be a more straightforward way.
- The maxResponseSize is not a fixed value, so that it would be harder to find a threshold to trigger the alert.
There was a problem hiding this comment.
I see. If we are more interested in the number of occurrences then counter makes sense.
bowenxia
deleted the
xbowen/add_metric_for_history_replication_message_too_large
branch
What changed?
Add metric when history replication message is too large
Why?
to catch a previous on-call issue
How did you test it?
Potential risks
Release notes
Documentation Changes