fix(middleware)!: name HTTP spans by route template, not raw path#30
Merged
Conversation
HTTP SERVER spans were named from the resolved request path with IDs
embedded (e.g. "GET /v1/dict/statistics/keys/<pix-key>"), which
(1) exploded spanmetrics cardinality — one service produced ~50k
distinct span.name values, ~72% of active series in prod Mimir — and
(2) leaked PII (Pix keys, CPF/CNPJ) raw into span names shipped to the
trace backend.
Name spans per the OTel convention "{method} {http.route}" using the
low-cardinality route template, which structurally excludes PII:
- Create the span with a method-only name (PII-free at creation, so the
sampler and any dropped/unmatched spans never carry an identifier).
- After routing resolves, rename to "{method} {http.route}" (guarded by
IsRecording()); on unmatched/404 keep the method-only name and omit
http.route, per spec.
Fix the span-lifecycle ordering hazard this exposed: a separately
registered EndTracingSpans unwinds first under Fiber LIFO and ended the
span before WithTelemetry applied the rename/attributes, silently
dropping them (present in the standard ordering too, not just misordered
consumers). Add an `owned` flag so WithTelemetry is the sole ender of its
root span; EndTracingSpans skips owned spans but still ends
foreign/handler-created spans (fallback preserved). sync.Once is retained
for double-end idempotency (gRPC pair + foreign-span path).
Remove the now-dead replaceUUIDWithPlaceholder helper (the span name no
longer derives from the path). gRPC interceptors are intentionally left
unchanged (named from info.FullMethod; no PII/cardinality).
Tests: 3 middleware-ordering permutations, method-only creation name,
template naming with numeric+UUID keys collapsing, routeAttribute units.
BREAKING CHANGE: HTTP server span names change from the raw request path
to "{method} {http.route}". Dashboards/queries grouping traces by the old
span name must be updated. Name-based samplers now see a method-only name
at span creation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
X-Lerian-Ref: 0x1
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHTTP telemetry now creates spans with method-only names, renames them to route templates after routing, and assigns root-span finalization to ChangesHTTP Span Naming and Lifecycle
Sequence Diagram(s)sequenceDiagram
participant FiberRouter
participant WithTelemetry
participant OpenTelemetrySpan
participant EndTracingSpans
WithTelemetry->>OpenTelemetrySpan: Start span with method-only name
WithTelemetry->>FiberRouter: Execute request routing
FiberRouter-->>WithTelemetry: Return route template
WithTelemetry->>OpenTelemetrySpan: Set route-based name and attributes
EndTracingSpans->>WithTelemetry: Check span ownership
WithTelemetry->>OpenTelemetrySpan: End owned root span once
Possibly related PRs
✨ Finishing Touches✨ Simplify code
Comment |
lerian-studio
added
size/L
Contributor
🔍 PR Validation Summary✅ PR Mergeable — no blocking failures
|
Contributor
🔒 Security Scan Results —
|
| Stage | Status | Blocking? |
|---|---|---|
| Filesystem Scan | 🔴 Yes | |
| Docker Image Scan | ➖ Skipped | — |
| Docker Hub Health Score | ➖ Skipped | — |
| Pre-release Version Check | ✅ Clean | — |
Trivy
Filesystem Scan
| Severity | Library | Vulnerability | Installed | Fixed | Title |
|---|---|---|---|---|---|
| ⚪ UNKNOWN | golang.org/x/text |
CVE-2026-56852 | v0.38.0 | 0.39.0 | Infinite loop on invalid input in golang.org/x/text |
Pre-release Version Check
✅ No unstable version pins found.
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 85.7% ✅ PASS |
| Threshold | 80% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/lib-observability/v2/assert |
97.9% |
github.com/LerianStudio/lib-observability/v2/constants |
83.3% |
github.com/LerianStudio/lib-observability/v2/log |
94.9% |
github.com/LerianStudio/lib-observability/v2/metrics |
91.4% |
github.com/LerianStudio/lib-observability/v2/middleware |
76.8% |
github.com/LerianStudio/lib-observability/v2/redaction |
95.8% |
github.com/LerianStudio/lib-observability/v2/runtime |
80.4% |
github.com/LerianStudio/lib-observability/v2/tracing |
84.8% |
github.com/LerianStudio/lib-observability/v2/zap |
96.0% |
github.com/LerianStudio/lib-observability/v2 |
91.5% |
Generated by Go PR Analysis workflow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@middleware/helpers_test.go`:
- Around line 90-92: Replace assert.NoError with require.NoError for the
app.Test calls in the affected test cases before accessing resp, including the
instances around the user request, subsequent request at the additional
locations, and any resp.Body dereferences. Keep the existing response cleanup
and assertions unchanged.
In `@middleware/telemetry_span_lifecycle_test.go`:
- Around line 137-183: Add a test case in the existing telemetry middleware
ordering tests for registering EndTracingSpans before WithTelemetry, using
app.Use(mid.EndTracingSpans) followed by app.Use(mid.WithTelemetry(tel)). Send
the request and assert a successful response, then verify the finalized route
name and http.route through assertRootSpanEndedOnce.
In `@middleware/telemetry.go`:
- Around line 525-530: Update the gRPC interceptor span setup near the existing
ownership comment to mark the span as owned, preserving finalization until
WithTelemetryInterceptor completes its post-handler attribute and status
recording. Remove or revise the outdated explanation that intentionally leaves
the span unowned; do not rely on a regression test as a substitute for restoring
the ordering guarantee.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38c41a0d-fb22-4b58-8de0-37182f3ae7eb
📒 Files selected for processing (9)
docs/pre-dev/fix-span-name-cardinality-pii/prd.mddocs/pre-dev/fix-span-name-cardinality-pii/research.mddocs/pre-dev/fix-span-name-cardinality-pii/tasks.mddocs/pre-dev/fix-span-name-cardinality-pii/trd.mdmiddleware/helpers.gomiddleware/helpers_test.gomiddleware/telemetry.gomiddleware/telemetry_span_lifecycle_test.gomiddleware/telemetry_test.go
💤 Files with no reviewable changes (1)
- middleware/helpers.go
- Mark the gRPC WithTelemetryInterceptor span as owned and skip it in EndTracingSpansInterceptor, mirroring the HTTP pair. The gRPC span records rpc.method / rpc.grpc.status_code / handler error status AFTER the handler returns, so an end-interceptor that unwinds first could drop them — the same ordering hazard the HTTP owned flag fixes. (CodeRabbit) - helpers_test.go: use require.NoError before dereferencing resp so a failed app.Test aborts instead of panicking on resp.Body and hiding the real cause. (CodeRabbit) - telemetry_span_lifecycle_test.go: add the 4th ordering permutation — EndTracingSpans registered BEFORE WithTelemetry (outermost, unwinds last) — asserting the owned span is not double-ended and finalization is preserved. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> X-Lerian-Ref: 0x1
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/telemetry.go (1)
449-460: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOnly skip when the active span matches the owned root span
state.ownedis inherited by child contexts, so a handler/interceptor that installs a distinct span can still trip this early return and leave that active span open. Comparetrace.SpanFromContext(endCtx)withstate.spanbefore skipping, and apply the same check in both:
middleware/telemetry.go#L449-L460middleware/telemetry.go#L569-L576🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/telemetry.go` around lines 449 - 460, In middleware/telemetry.go lines 449-460 and 569-576, update the state.owned early-return condition to skip only when trace.SpanFromContext(endCtx) matches state.span; otherwise call state.End(). Apply the same matching check at both sites so child contexts with distinct active spans are finalized correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@middleware/telemetry.go`:
- Around line 449-460: In middleware/telemetry.go lines 449-460 and 569-576,
update the state.owned early-return condition to skip only when
trace.SpanFromContext(endCtx) matches state.span; otherwise call state.End().
Apply the same matching check at both sites so child contexts with distinct
active spans are finalized correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f19e18d2-4935-44bb-9230-926dd5933564
📒 Files selected for processing (3)
middleware/helpers_test.gomiddleware/telemetry.gomiddleware/telemetry_span_lifecycle_test.go
Contributor
Author
|
| Step | Resultado |
|---|---|
| Trivy Filesystem Scan | ✅ success — Report Summary: go.mod / gomod / Secrets: - (0 findings) |
| Pre-release Version Check | ✅ success |
| Post Security Scan Results to PR | ❌ failure |
O step de report falha com:
::error::pr-security-reporter: github-script produced no output.
The script step likely failed (API error, rate limit, or runtime exception).
Sem output do github-script, o gate falha por precaução (Failing to prevent bypass) e emite a mensagem enganosa "Security vulnerabilities found" — mas nenhuma vulnerabilidade foi encontrada (Trivy limpo, gosec pass, Go Analysis/Security pass).
Causa: bug no reporter do workflow compartilhado LerianStudio/github-actions-shared-workflows (step "Post Security Scan Results to PR"), não neste código. É consistente (falhou em 2 runs seguidos), então re-run não resolve. O diff deste PR não toca go.mod/go.sum.
Ação: reportar o bug ao time dono do workflow. O merge não deve ser bloqueado por este check falso-positivo.
…return Add the required blank line before the final `return resp, err` in EndTracingSpansInterceptor's owned-span branch (wsl_v5 "too many lines above return"). No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> X-Lerian-Ref: 0x1
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.
Problema
Spans HTTP SERVER eram nomeados a partir do caminho resolvido com os IDs embutidos (ex.:
GET /v1/dict/statistics/keys/<chave-pix>), causando:span.name, ~72% das séries ativas no Mimir de prod (o spanmetrics connector usaspan.namecomo dimensão).Solução
Nomear spans pela convenção OTel
{method} {http.route}usando o template de rota (baixa cardinalidade), que exclui PII estruturalmente:{method} {http.route}(guardado porIsRecording()); em 404/não-casado mantém só-método e omitehttp.route(conforme spec).Correção do hazard de ciclo de vida (ADR-001)
O fix expôs que o
EndTracingSpans, por LIFO do Fiber, desenrola antes doWithTelemetryaplicar o rename/atributos — encerrando o span cedo e descartandohttp.route/status/rename silenciosamente (ocorre inclusive na ordem correta do standard, não só em consumidores mal-ordenados). Adicionado flagowned:WithTelemetryé o único encerrador do seu span raiz;EndTracingSpanspula spans owned mas preserva o encerramento de spans estrangeiros/handler-criados.sync.Oncemantido para idempotência (par gRPC + fallback).Processo (Ring pre-dev)
Planejado via fluxo Ring completo (research → PRD → TRD → tasks, em
docs/pre-dev/fix-span-name-cardinality-pii/). TRD revisado por engenheiro Go. Implementação revisada por 3 reviewers (qualidade + correção + segurança) — 3/3 PASS, sem achados bloqueantes.Testes
go build+go test -tags=unit ./...verdes;golangci-lint0 issues.routeAttribute.TestEndTracingSpans_EndsFinalContextSpan(fallback de span estrangeiro).O formato do nome dos spans HTTP muda do caminho cru para
{method} {http.route}. Dashboards/queries que agrupam traces pelo nome antigo do span precisam ser atualizados. Samplers baseados em nome agora veem só-método na criação.Follow-ups (não neste PR)
url.pathainda carrega o path resolvido (sanitizeURL limpa só query params, não templatiza segmentos) → 2º vetor de PII para segmentos como/keys/<cpf>. Fix separado.ownedao par gRPC se um rename pós-handler for adicionado no futuro (hoje desnecessário).🤖 Generated with Claude Code