Add Labeler for custom metric attributes#204
Conversation
14817da to
23553fe
Compare
23553fe to
89accf0
Compare
|
I added additional tests for the streaming client: |
|
Hi @jhump, could you please take a look when you have a moment? I mainly want to know if this kind of change is acceptable for you, |
|
drive-by as I have no standing here, but I noticed that upstream they're considering if the Labeler API is going to be deprecated (or doing something else entirely): open-telemetry/opentelemetry-go-contrib#8402 |
7b9c64a to
d00f1d3
Compare
|
@stefanvanburen Thank you for the feedback! Since the Labeler API appears to be under reconsideration upstream (open-telemetry/opentelemetry-go-contrib#8402), I've removed it from this PR and kept only |
|
@timostamm Would it be possible to move forward with this PR please? |
|
@michaljurecko once again, not a maintainer, but it looks like upstream they've now reversed their decision and suggest they're going towards the |
|
@stefanvanburen ok, I didn't notice that the discussion took the opposite direction there yesterday. open-telemetry/opentelemetry-go-contrib#8402 (comment)
|
|
open-telemetry/opentelemetry-go-contrib#8587
I will update this PR. |
d00f1d3 to
75e472f
Compare
|
Hi @stefanvanburen, I've updated the PR based on the discussion. The upstream |
Introduce a Labeler type that allows instrumented ConnectRPC handlers to add custom attributes to metrics during request processing. The labeler is stored in context and provides thread-safe Add/Get methods. This follows the same pattern as otelhttp's Labeler implementation. Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
Inject the Labeler into the request context at the start of each RPC (unary, streaming client, streaming handler). Separate metric attributes from span attributes so that Labeler attributes are added only to metrics, not to spans. The streamingState now carries a reference to the Labeler and uses metricAttributes() for all metric recordings. Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
75e472f to
388f7c4
Compare
emcfarlane
left a comment
There was a problem hiding this comment.
Thanks! Looking good some comments around structure.
Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
emcfarlane
left a comment
There was a problem hiding this comment.
Ran a benchmark to check impact on perf (with the unary fix applied):
goos: darwin
goarch: arm64
pkg: connectrpc.com/otelconnect
cpu: Apple M2
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
StreamingBase-8 63.94µ ± 12% 61.80µ ± 26% ~ (p=0.937 n=6)
StreamingWithInterceptor-8 62.85µ ± 2% 65.00µ ± 3% +3.42% (p=0.009 n=6)
UnaryBase-8 37.59µ ± 21% 37.78µ ± 2% ~ (p=0.818 n=6)
UnaryWithInterceptor-8 38.11µ ± 2% 38.93µ ± 9% +2.16% (p=0.002 n=6)
geomean 48.98µ 49.30µ +0.65%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
StreamingBase-8 20.10Ki ± 8% 20.68Ki ± 7% ~ (p=0.180 n=6)
StreamingWithInterceptor-8 27.42Ki ± 5% 27.76Ki ± 4% ~ (p=0.394 n=6)
UnaryBase-8 17.30Ki ± 4% 17.45Ki ± 5% ~ (p=0.818 n=6)
UnaryWithInterceptor-8 20.21Ki ± 2% 20.43Ki ± 5% ~ (p=0.240 n=6)
geomean 20.95Ki 21.27Ki +1.52%
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
StreamingBase-8 172.5 ± 0% 173.0 ± 1% ~ (p=0.545 n=6)
StreamingWithInterceptor-8 234.0 ± 0% 238.0 ± 0% +1.71% (p=0.002 n=6)
UnaryBase-8 152.0 ± 0% 152.0 ± 0% ~ (p=1.000 n=6) ¹
UnaryWithInterceptor-8 190.0 ± 0% 194.0 ± 0% +2.11% (p=0.002 n=6)
geomean 184.8 186.7 +1.02%
¹ all samples are equal
@pkwarren ok?
More idiomatic Go per context.WithValue docs. Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
Skip slices.Clone + concat when the labeler has no custom attributes, matching the fast-path already used in streaming's metricAttributes(). Signed-off-by: Michal Jurecko <michal.jurecko@gmail.com>
f09b011 to
44d81e5
Compare
Summary
Add a
Labelertype — a context-bound, mutex-protected attribute accumulator that handlers can use to add custom metric attributes during request processing. Accessed viaLabelerFromContext/ContextWithLabeler.This follows the same pattern as
otelhttp.Labeler.Labelerinto the context automatically if one isn't already presentWhy Labeler and not WithMetricAttributesFn
The upstream
otelhttplibrary had bothLabelerandWithMetricAttributesFn. After discussion,WithMetricAttributesFnwas deprecated in favor ofLabeler. This PR follows that decision and only addsLabeler.Motivation
We need to add custom attributes (e.g.
product.variant) to the standard metrics (rpc.server.duration, etc.) to distinguish between product variants. Defining entirely custom metrics would be unnecessarily verbose when the standard metrics already capture the right measurements — they just lack the ability to carry custom attributes.The
otelhttpinstrumentation library solves this exact problem with itsLabeler. This PR adapts the same pattern for ConnectRPC.Relates to #199.