add http client metrics #543
Conversation
| return nil, err | ||
| } | ||
| hm.ConnectionUsage, err = meter.Int64UpDownCounter("client.http.connections.usage", func(o *metrics.InstrumentOptions) { | ||
| o.UnitLabel = "{connection}" |
There was a problem hiding this comment.
nit: can we name this connections?
also, probably outside the scope of this PR but this would be nice if it had been named client.http.connections.total or something like that, usage doesn't make it clear what kind of usage we're talking about (bandwidth, idle or active connections)
There was a problem hiding this comment.
This comes from the SRA list of suggested metrics (which I copied in here) - I would rather not deviate where naming has been established.
There was a problem hiding this comment.
Yeah, that's what I was hinting with "outside the scope of this PR". I didn't know the unit was also described there, my bad.
|
|
||
| func (m *httpMetrics) PutIdleConn(ctx context.Context) func(error) { | ||
| return func(error) { | ||
| m.addConnAcquired(ctx, -1) |
There was a problem hiding this comment.
Can we count a connection set as idle as "not acquired"? I'm curious how this plays with keep-alive connections
There was a problem hiding this comment.
The SRA defined this as having one dimension: state = idle | acquired - acquired meaning "in use right now" and idle meaning "not in use" - might have been better to say be active | acquired but again this comes from external recommended naming.
| c.hm.doStart = now() | ||
| resp, err := c.ClientDo.Do(r) | ||
|
|
||
| c.hm.DoRequestDuration.Record(r.Context(), elapsed(c.hm.doStart)) |
There was a problem hiding this comment.
How do we keep track of which request is the one that just ended?
There was a problem hiding this comment.
Not sure I follow - this is all happening inside Do(), which is one request?
There was a problem hiding this comment.
Got it, then disregard the comment
Final list of metrics:
*** We are unable to determine the state of connections right now (more specifically, exactly when a connection is idle isn't known to us). We're adding the dimension of state=acquired for now in case that ever changes.