-
Notifications
You must be signed in to change notification settings - Fork 3.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
rpc: add heartbeat metrics #37361
rpc: add heartbeat metrics #37361
Conversation
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @DuskEagle)
pkg/rpc/context.go, line 398 at r1 (raw file):
version *cluster.ExposedClusterVersion metrics Metrics
Missing import for this?
2ad25fd
to
5d8a5cc
Compare
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.
Thanks for the quick review. I had forgotten to add an entire file. RFAL.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DuskEagle)
pkg/rpc/context.go, line 398 at r1 (raw file):
Previously, DuskEagle (Joel Kenny) wrote…
Missing import for this?
Forgot to git add a whole file
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.
Reviewed 2 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @DuskEagle)
a discussion (no related file):
Aside from the one comment.
pkg/rpc/metrics.go, line 113 at r2 (raw file):
// updateHeartbeatState increments the gauge for the current state and // decrements the gauge for the new state, returning the new state.
This comment is backward, it should be something likedecrements the gauge for the old state.
5d8a5cc
to
5d890d0
Compare
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DuskEagle)
a discussion (no related file):
Previously, DuskEagle (Joel Kenny) wrote…
:lgtm: Aside from the one comment.
TFTR!
pkg/rpc/metrics.go, line 113 at r2 (raw file):
Previously, DuskEagle (Joel Kenny) wrote…
This comment is backward, it should be something like
decrements the gauge for the old state.
Done.
dfaaa42
to
7ddb91c
Compare
This PR adds metrics to the rpc context. The initial set of metrics includes gauges to track the current state of heartbeat loops as well as counters for the number of loops started and failed. The gauges give a nice point-in-time view of the state of a server's connections but are subject to aliasing while the counters fail to capture how many of the connections are actually observing problems. Taken together they ought to provide some insight into the state of network health between nodes. Release note: None
7ddb91c
to
4850713
Compare
bors r+ |
37361: rpc: add heartbeat metrics r=ajwerner a=ajwerner This PR adds metrics to the rpc context. The initial set of metrics includes gauges to track the current state of heartbeat loops as well as counters for the number of loops started and failed. The gauges give a nice point-in-time view of the state of a server's connections but are subject to aliasing while the counters fail to capture how many of the connections are actually observing problems. Taken together they ought to provide some insight into the state of network health between nodes. Release note: None Co-authored-by: Andrew Werner <[email protected]>
Build succeeded |
This PR adds metrics to the rpc context. The initial set of metrics includes
gauges to track the current state of heartbeat loops as well as counters for
the number of loops started and failed. The gauges give a nice point-in-time
view of the state of a server's connections but are subject to aliasing while
the counters fail to capture how many of the connections are actually observing
problems. Taken together they ought to provide some insight into the state of
network health between nodes.
Release note: None