-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rafthttp: add Raft send latency metric for writes #10023
Conversation
Address #9438. |
Codecov Report
@@ Coverage Diff @@
## master #10023 +/- ##
==========================================
- Coverage 71.59% 71.51% -0.09%
==========================================
Files 390 390
Lines 36258 36265 +7
==========================================
- Hits 25960 25935 -25
- Misses 8488 8519 +31
- Partials 1810 1811 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two questions, everything else lgtm.
etcdserver/api/rafthttp/stream.go
Outdated
// heartbeats are tracked via prober https://github.com/coreos/etcd/pull/10022 | ||
if m.Type == raftpb.MsgProp || m.Type == raftpb.MsgApp || m.Type == raftpb.MsgAppResp { | ||
raftSendSeconds.WithLabelValues(m.Type.String(), types.ID(m.To).String()).Observe(took.Seconds()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that MsgProp, MsgApp and MsgAppResp are chosen here? I was reading the doc, and you explained snapshot and heartbeats, how about other message types?
// highest bucket start of 0.0001 sec * 2^15 == 3.2768 sec | ||
Buckets: prometheus.ExponentialBuckets(0.0001, 2, 16), | ||
}, | ||
[]string{"Type", "To"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean []string{"To"}?
Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
…econds" metric Currently, only v2 metrics ("stats.FollowerStats") tracks Raft message send latencies. Add Prometheus histogram to track Raft messages for writes, since heartbeats are probed (see etcd-io#10022) and snapshots are already being tracked via etcd-io#9997. ``` etcd_network_raft_send_total_duration_seconds_bucket{To="7339c4e5e833c029",Type="MsgProp",le="0.0001"} 1 etcd_network_raft_send_total_duration_seconds_bucket{To="7339c4e5e833c029",Type="MsgProp",le="0.0002"} 1 etcd_network_raft_send_total_duration_seconds_bucket{To="729934363faa4a24",Type="MsgApp",le="0.0001"} 9 etcd_network_raft_send_total_duration_seconds_bucket{To="729934363faa4a24",Type="MsgApp",le="0.0002"} 9 etcd_network_raft_send_total_duration_seconds_bucket{To="7339c4e5e833c029",Type="MsgAppResp",le="0.0001"} 8 etcd_network_raft_send_total_duration_seconds_bucket{To="7339c4e5e833c029",Type="MsgAppResp",le="0.0002"} 8 ``` Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
Those are most frequent ones that would give us enough sample size to investigate any network issues. Other important ones are heartbeat and v3 snapshot messages:
By adding "From" field, we can just use one metrics to track multiple message types.
Can you confirm with GKE team, if this does not break anything in production? I will check with other production users. |
Oh, this is a new metrics, so should not break anything when backported. |
@gyuho I do need to check if it's safe to add label though. Checking now, get back to you soon. |
// snapshot sends are tracked via separate metrics https://github.com/etcd-io/etcd/pull/9997 | ||
// heartbeats are tracked via prober https://github.com/etcd-io/etcd/pull/10022 | ||
// TODO: track other messages? | ||
if m.Type == raftpb.MsgProp || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not the sending time (flushing the buffer to the network card). we actually send the messages at line 211 when flush is called explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would rather just remove this metrics. it is not easy to measure after all with buffers all over the places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiang90 Good point. For now, tracking snapshots and heartbeats is good enough. Will come back to this later.
Will get back to this later. |
Currently, only v2 metrics ("stats.FollowerStats") tracks Raft message
send latencies. Add Prometheus histogram to track Raft messages for
writes, since heartbeats are probed (see #10022)
and snapshots are already being tracked via #9997.