Add RPC semantic convention stable opt-in support#15932
Add RPC semantic convention stable opt-in support#15932zeitlinger wants to merge 17 commits intoopen-telemetry:mainfrom
Conversation
8ada385 to
3bee40e
Compare
|
@trask this is now the first PR for rpc |
...c/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcClientMetrics.java
Outdated
Show resolved
Hide resolved
| default String getFullMethod(REQUEST request) { | ||
| String service = getService(request); | ||
| String method = getMethod(request); | ||
| if (service == null || method == null) { | ||
| return null; | ||
| } | ||
| return service + "/" + method; | ||
| } |
There was a problem hiding this comment.
rename to getRpcMethod() matching convention of other getters
can we just return null from default method? (requiring choice in each RPC implementation how to structure method)
also, how to handle:
When the method is not recognized, for example, when the server receives a request for a method that is not predefined on the server, or when instrumentation is not able to reliably detect if the method is predefined, the attribute MUST be set to _OTHER. In such cases, tracing instrumentations MUST also set rpc.method_original attribute to the original method value.
2ede454 to
58a163f
Compare
...ain/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcAttributesGetter.java
Outdated
Show resolved
Hide resolved
...ain/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcAttributesGetter.java
Outdated
Show resolved
Hide resolved
| } else { | ||
| oldClientDurationHistogram = null; |
There was a problem hiding this comment.
by default fields are initialized to null, is there anything to gain from explicitly assigning the null here?
There was a problem hiding this comment.
this allows the field to be final
...c/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcClientMetrics.java
Outdated
Show resolved
Hide resolved
| internalSet( | ||
| attributes, | ||
| RPC_SYSTEM_NAME, | ||
| system == null ? null : SemconvStability.stableRpcSystemName(system)); |
There was a problem hiding this comment.
maybe it would be better to handle null in SemconvStability.stableRpcSystemName
There was a problem hiding this comment.
the param would need to be nullable - which I'd like to avoid
| keys.add(RpcCommonAttributesExtractor.RPC_METHOD); | ||
|
|
||
| // Add status code key | ||
| if (SemconvStability.emitStableRpcSemconv()) { |
There was a problem hiding this comment.
should this use stable instead?
There was a problem hiding this comment.
method is in both stable and old
...c/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcMetricsAdvice.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcMetricsAdvice.java
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcSpanNameExtractor.java
Outdated
Show resolved
Hide resolved
|
|
||
| // virtual key to avoid clash with stable rpc.method | ||
| private static final AttributeKey<String> RPC_METHOD_OLD = | ||
| AttributeKey.stringKey("rpc.method.deprecated"); |
There was a problem hiding this comment.
makes me wonder whether we should bother with this at all since it is such a corner case, might as well let the attributes clash
There was a problem hiding this comment.
you mean not supporting rpc/dup?
There was a problem hiding this comment.
under rpc/dup, stable semconv takes priority when there's a conflict like this (no need for adding this extra unspec'd attribute)
There was a problem hiding this comment.
this is never emitted - it's only kept in memory and gets renamed before being added as rpc.method
There was a problem hiding this comment.
this is how this virtual attribute gets used:
b9017d6 to
298b23c
Compare
This adds support for the stable RPC semantic conventions via the `otel.semconv-stability.opt-in=rpc` configuration option. Key changes: - Add emitOldRpcSemconv()/emitStableRpcSemconv() to SemconvStability - Add stableRpcSystemName() mapping (apache_dubbo→dubbo, connect_rpc→connectrpc) - Add RpcAttributesGetter.getFullMethod() for "service/method" format - Update RpcCommonAttributesExtractor for dual semconv emission - Update RpcClientMetrics/RpcServerMetrics for dual histogram (ms→s unit change)
298b23c to
7690d20
Compare
|
I think this could be broken up into smaller PRs which would make reviews go faster, here's an AI generated suggestion that seems reasonable, though I haven't looked at it too closely: Plan: Break PR #15932 into Smaller PRsPR: Add RPC semantic convention stable opt-in support #15932 TL;DR: PR #15932 adds RPC stable semconv opt-in support across 25 files with +1,076/-350 changes. It can be decomposed into 5 incremental PRs that each deliver value independently while preserving backward compatibility. The key insight is that the PR 1: Add RPC SemconvStability infrastructure (foundation)Files:
Changes:
Impact: No behavioral change - just adds flags (defaults to old semconv only) PR 2: Expand RpcAttributesGetter interface (backward-compatible API change)Files:
Changes:
Impact: No behavioral change - default implementations return null/false preserving existing behavior PR 3: Add dual-semconv support to RPC attributes extractorsFiles:
Changes:
PR 4: Add dual-semconv support to RPC metricsFiles:
Changes:
PR 5: Update instrumentation implementationsFiles: All
Changes:
Verification
Design Decisions
Dependencies
|
.. I'll let the AI do it 😄 |
When both old and stable RPC semconv are enabled, both use the rpc.method attribute key with different values. Previously this was solved with a virtual rpc.method.deprecated key in SemconvStability that leaked onto spans. Instead, register a ContextCustomizer that stores the old method value directly in context via a ContextKey. RpcClientMetrics/RpcServerMetrics read from context in onStart() and use the stored value when recording old metrics. This keeps the old method value off spans entirely.
First part of #15879
This adds support for the stable RPC semantic conventions via the
otel.semconv-stability.opt-in=rpcconfiguration option.Key changes: