refactor(trace/semantics): lift conditional gRPC status fallback into mappings.json#50397
Draft
edwardhu-datadog wants to merge 6 commits intofeat/ehu/OTEL-2718-otlp-grpc-status-trace-metricsfrom
Conversation
… mappings.json Move the rpc.system.name/rpc.system → rpc.response.status_code special case out of transform.getOTelGRPCStatusCode and into the semantic registry as `when` clauses on TagInfo. This removes the gRPC-specific helper from the transform package and lets future conditional fallbacks be expressed declaratively in mappings.json. Conditions read attributes by exact key (no transitive concept resolution) — to gate on a renamed attribute, list one fallback row per condition attribute. This matches the dd-source trace-semantics precedent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop the float64-precision sub-test from TestLookupFloat64; it tests a structural property unrelated to conditional fallback that this PR doesn't touch. - Drop the conditional.eq cases; the gRPC test cases already exercise Eq accept/reject on a realistic concept. - Drop the conditional.present/conditional.absent cases; Present is unused in mappings.json today and the AND test covers Present:true implicitly. - Rename testBoolPtr to boolPtr (the _test.go suffix already conveys it is test-only). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nal check inline
Drop the lookupWithConditions/readTag/readFloat64Tag/readInt64Tag
extraction. The conditional fallback feature only needs a single
`if !conditionsMatch(...) { continue }` line at the top of each loop;
extracting per-type readers and a generic helper was orthogonal
churn that obscured the diff.
Net diff vs main: +37 lines, 0 deletions to existing code.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The pre-refactor getOTelGRPCStatusCode used GetOTelAttrFromEitherMap with
keys ("rpc.system.name", "rpc.system"), which returns the first non-empty
key — so when an SDK sets rpc.system.name=jsonrpc and (incorrectly) also
rpc.system=grpc, the new key wins and rpc.response.status_code is rejected.
Without this fix the two independent registry rows would each evaluate
against their own attribute, and the legacy rpc.system=grpc row would
match — accepting a status code that pre-PR rejected.
Gate the legacy rows on rpc.system.name being absent so the new semconv
key remains authoritative when both are present.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TestConditionalLookup now uses NewEmbeddedRegistry() against the real mappings.json instead of a hand-rolled fake. Removes ~50 lines of Provider/Type struct-literal boilerplate and the synthetic conditional.and concept (the rpc.system+rpc.system.name two-condition AND in production gives implicit AND coverage). boolPtr helper is no longer needed. - Collapse the duplicated `c.Present != nil` check at the tail of conditionMatches into a single return. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
🎯 Code Coverage (details) 🔗 Commit SHA: 51cd1fe | Docs | Datadog PR Page | Give us feedback! |
…C keys
rpc.response.status_code and rpc.system.name were both first added to
OTel semantic-conventions in v1.39.0 (Jan 2026), not v1.24.0 (Dec 2023).
Verified against the upstream repo:
- rpc.response.status_code: commit cd5b45c6 (2025-12-04), .chloggen/rpc-status-code.yaml
"Deprecate per-rpc status code attributes in favor of rpc.response.status_code"
- rpc.system.name: commit 18db3e44 (2025-12-09), .chloggen/rpc-system-name.yaml
"Align RPC conventions with naming guidelines. Renames rpc.system to rpc.system.name."
Both commits land in v1.39.0 (published 2026-01-12). model/registry/rpc.yaml
at v1.24.0 has neither attribute.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Files inventory check summaryFile checks results against ancestor 874cceb7: Results for datadog-agent_7.80.0~devel.git.456.51cd1fe.pipeline.111602222-1_amd64.deb:No change detected |
Contributor
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
15 successful checks with minimal change (< 2 KiB)
On-wire sizes (compressed)
|
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
1 participant
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.
Summary
Stacked on top of #50272 (please review that one first).
Moves the gRPC-specific conditional logic out of
transform.getOTelGRPCStatusCodeand into the semantic registry aswhenclauses onTagInfo, addressing the reviewer's request in #50272 (comment) and aligning with the dd-source trace-semantics precedent.After this PR, the
rpc.response.status_code→ConceptGRPCStatusCodemapping (gated onrpc.system.name=grpc, orrpc.system=grpcwhenrpc.system.nameis absent) is expressed declaratively inmappings.json. Thetransformpackage no longer carries gRPC-specific code;LookupString(reg, spanAccessor, ConceptGRPCStatusCode)does the routing.Design notes
rpc.system→rpc.system.name), list one fallback row per condition attribute. Matches the dd-source pattern and avoids aresolving map[Concept]struct{}cycle-detection thread through the lookup machinery.rpc.system.name=grpcrows + tworpc.system=grpc(withrpc.system.nameabsent) rows. dd-source's precedent uses onlyrpc.system=grpcbecause it predates OTel semconv v1.39.0 (no references torpc.system.nameorrpc.response.status_code). Our code needs both because the originating commit (dc66769) explicitly handles SDKs that have migratedrpc.response.status_codewithout updatingrpc.systemtorpc.system.name. New-semconv rows are listed first so they win precedence; the legacy rows additionally requirerpc.system.nameto be absent so the new key remains authoritative when both are present (matching the pre-refactorgetOTelGRPCStatusCodeprecedence).Conditionschema.{attribute, present, eq}. The dd-source precedent usesvalueIsfor single-value match and has no array form;Inwould be unused in production today, so it's not in the schema.Verifying the
"version": "1.39.0"fieldrpc.response.status_codeandrpc.system.namewere both introduced in OTel semantic-conventions v1.39.0 (published 2026-01-12). Verified against the upstream repo:rpc.response.status_coderpc.response.status_code".chloggen/rpc-status-code.yamlrpc.system.namerpc.system→rpc.system.name).chloggen/rpc-system-name.yamlCross-checked:
model/registry/rpc.yamlatv1.24.0definesrpc.connect_rpc.error_code,rpc.grpc.status_code,rpc.method,rpc.service,rpc.systemetc., but notrpc.response.status_codeorrpc.system.name.model/rpc/registry.yamlatv1.39.0defines both.What changed
pkg/trace/semantics/lookup.goconditionMatches/conditionsMatchhelpers; insertif !conditionsMatch(...) { continue }in each ofLookup/LookupFloat64/LookupInt64(3 lines each, otherwise unchanged frommain).pkg/trace/semantics/semantics.goConditiontype andWhen []ConditiononTagInfo.pkg/trace/semantics/mappings.jsonrpc.system.nameto therpc.systemfallback chain. Add four conditionalrpc.response.status_coderows underrpc.grpc.status_code(string/int64 ×rpc.system.name/rpc.system+rpc.system.name absent).pkg/trace/semantics/lookup_test.goTestConditionalLookup— 8 sub-tests built onNewEmbeddedRegistry()(production rules), no synthetic concepts.pkg/trace/transform/transform.gogetOTelGRPCStatusCode; callLookupString(..., ConceptGRPCStatusCode)directly.Net diff (against #50272's branch): +143 / -25 across 5 files.
Test plan
go test ./pkg/trace/semantics/... -run TestConditionalLookup -v -count=1— 8 sub-tests pass.go test ./pkg/trace/semantics/... -count=1— passes.go test ./pkg/trace/transform/... -count=1— passes; no behavioral change.go test ./pkg/trace/otel/stats/... -count=1— gRPC OTLP fixture continues to populaterpc.grpc.status_codein span Meta.Notes for reviewer
apm-otlp-grpc-status-code-bf3137b34cb82136.yamlreno still describes the fix accurately.main.🤖 Generated with Claude Code