*: add drpc client and server metrics#33
Open
suj-krishnan wants to merge 1 commit intocockroachdb:mainfrom
Open
*: add drpc client and server metrics#33suj-krishnan wants to merge 1 commit intocockroachdb:mainfrom
suj-krishnan wants to merge 1 commit intocockroachdb:mainfrom
Conversation
suj-krishnan
force-pushed
the
add_drpc_metrics
branch
6 times, most recently
from
66f4653 to
af14586
Compare
suj-krishnan
force-pushed
the
add_drpc_metrics
branch
from
49ddc69 to
8f47786
Compare
suj-krishnan
force-pushed
the
add_drpc_metrics
branch
from
d9bfe89 to
7fc123f
Compare
Add metrics collection for drpc client connections, server connections, and stream pool. - Introduce Counter, Observer, and Gauge metric interfaces in the drpc package, allowing us to plug in the metrics implementation from cockroach - Client metrics: bytes sent/received - Server metrics: bytes sent/received, TLS handshake errors, inactivity timeouts, and per-RPC stream lifecycle metrics (total, active, duration, errors) with labels for service/method/type - Pool metrics: pool size, connection hits/misses, connection age
suj-krishnan
force-pushed
the
add_drpc_metrics
branch
from
7fc123f to
fbc10de
Compare
| BytesSent: opts.Metrics.BytesSent, | ||
| BytesRecv: opts.Metrics.BytesRecv, | ||
| } | ||
| c.tr = tr |
There was a problem hiding this comment.
You can save one line if you move this above c declaration :)
shubhamdhama
left a comment
shubhamdhama
left a comment
There was a problem hiding this comment.
drpcstatsis now superseded by the newdrpc.Counter/drpc.Observerinterfaces. Since we don't use the drpcstats package, I think we can safely remove it. Also, please move themetrics.goto a new package,drpcmetricswhich would provide this transport.RPCTyperandrpcLabelsfeel like they belong indrpcmux, the mux already knows whether an RPC is unary and can split the path. The current design requiresdrpcserverto do a type assertion against its own handler to get this information.
| if p.opts.Metrics.ConnectionHitsTotal != nil { | ||
| p.opts.Metrics.ConnectionHitsTotal.Add(nil, 1) | ||
| } | ||
| if p.opts.Metrics.ConnectionAgeSeconds != nil { |
There was a problem hiding this comment.
This != nil is kinda spilled all over the place. Not a blocker for merging this PR, only suggesting if we can do better here.
One suggestion would be to default to noop implementation to these interfaces,
type noopCounter struct{}
func (noopCounter) Add(map[string]string, float64) {}
type noopGauge = noopCounter
type noopObserver struct{}
func (noopObserver) Observe(map[string]string, float64) {}
Author
There was a problem hiding this comment.
Thanks - yes, will fix this one (bothers me too).
Author
1 is in my todo list. Will check if 2 would work. TFTF! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add metrics collection for drpc client connections, server connections, and connection pools.