From d3b33f804ee26211af9cdbfb8ec8e97650cfa39c Mon Sep 17 00:00:00 2001 From: Fellipe Benoni Date: Mon, 20 Jul 2026 16:42:17 -0300 Subject: [PATCH 1/3] fix(middleware)!: name HTTP spans by route template, not raw path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HTTP SERVER spans were named from the resolved request path with IDs embedded (e.g. "GET /v1/dict/statistics/keys/"), 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) X-Lerian-Ref: 0x1 --- .../fix-span-name-cardinality-pii/prd.md | 120 ++++++++ .../fix-span-name-cardinality-pii/research.md | 109 +++++++ .../fix-span-name-cardinality-pii/tasks.md | 157 ++++++++++ .../fix-span-name-cardinality-pii/trd.md | 131 +++++++++ middleware/helpers.go | 8 - middleware/helpers_test.go | 83 ++++++ middleware/telemetry.go | 57 +++- middleware/telemetry_span_lifecycle_test.go | 268 ++++++++++++++++++ middleware/telemetry_test.go | 23 +- 9 files changed, 938 insertions(+), 18 deletions(-) create mode 100644 docs/pre-dev/fix-span-name-cardinality-pii/prd.md create mode 100644 docs/pre-dev/fix-span-name-cardinality-pii/research.md create mode 100644 docs/pre-dev/fix-span-name-cardinality-pii/tasks.md create mode 100644 docs/pre-dev/fix-span-name-cardinality-pii/trd.md create mode 100644 middleware/telemetry_span_lifecycle_test.go diff --git a/docs/pre-dev/fix-span-name-cardinality-pii/prd.md b/docs/pre-dev/fix-span-name-cardinality-pii/prd.md new file mode 100644 index 0000000..c925c7a --- /dev/null +++ b/docs/pre-dev/fix-span-name-cardinality-pii/prd.md @@ -0,0 +1,120 @@ +# PRD — Fix: nomes de operação HTTP com alta cardinalidade e dado sensível + +## Metadata +- **Date:** 2026-07-17 +- **Feature:** fix-span-name-cardinality-pii +- **Track:** Small (4 gates) +- **Gate:** 1 (PRD) +- **Research:** [research.md](./research.md) +- **Confidence:** 90/100 (problema quantificado em produção; solução é convenção padrão de indústria; valor/ROI claros) + +--- + +## 1. Problema + +A biblioteca de observabilidade nomeia cada operação HTTP recebida usando o **caminho concreto da requisição, com os identificadores embutidos** (ex.: a chave Pix / CPF de uma consulta). Como cada requisição tem um identificador diferente, cada uma vira um "nome de operação" único. Isso gera dois danos simultâneos: + +1. **Explosão de custo e degradação de observabilidade.** Nomes únicos por requisição multiplicam as séries de métricas derivadas. Em produção, **um único serviço gerou ~50.000 nomes de operação distintos — cerca de 72% de todas as séries ativas** do backend de métricas, encarecendo o armazenamento e tornando impossível agrupar/analisar operações semelhantes (todas as consultas de chave viram "operações diferentes"). + +2. **Vazamento de dado pessoal (LGPD).** Identificadores sensíveis — chave Pix, CPF, CNPJ, telefone — trafegam **em texto puro dentro do nome da operação** até o backend de traces, onde ficam armazenados. É exposição de dado pessoal em um local não previsto para isso. + +### Evidência / impacto quantificado +- ~50.000 nomes distintos em um serviço; ~72% das séries ativas do backend de métricas (medido em produção). +- Redução comprovada de ~99% na cardinalidade desse serviço quando os identificadores deixam de compor o nome da operação (validado por mitigação temporária no pipeline central). +- Dado sensível (CPF/CNPJ/chave Pix) presente hoje nos nomes de operação armazenados no backend de traces. + +### Quem é afetado +- **Times de plataforma/observabilidade (Lerian):** pagam o custo da cardinalidade e operam o backend saturado. +- **Times de produto que consomem a biblioteca:** dashboards e buscas ficam poluídos; não conseguem analisar latência/erro por rota de forma agregada. +- **Área de compliance/privacidade:** responsável pelo risco regulatório do dado pessoal exposto. +- **Clientes finais (indireto):** têm seus identificadores pessoais expostos em telemetria interna. + +### Workaround atual +Uma mitigação foi aplicada **no pipeline central de telemetria** (agrega/remove o nome de operação de alta cardinalidade). Funciona para conter custo, **mas não impede o vazamento de dado pessoal** (o identificador já saiu do ambiente de origem) e precisa ser mantida serviço a serviço. Não resolve a causa raiz, que está na biblioteca. + +--- + +## 2. Requisitos de Negócio + +### Resumo executivo +Corrigir, na origem, como a biblioteca nomeia as operações HTTP: passar a usar o **padrão de rota** (o gabarito da URL, sem os identificadores concretos) em vez do caminho resolvido. Isso elimina estruturalmente o dado pessoal do nome da operação e limita a cardinalidade das métricas, alinhando-se à convenção padrão de mercado de observabilidade e removendo a necessidade de mitigações espalhadas. + +### Personas +- **Engenheiro de plataforma/o11y** — *Objetivo:* manter o backend de telemetria com custo previsível e consultável. *Frustração:* um serviço domina as séries; alertas e dashboards ficam lentos/caros; precisa aplicar remendos manuais. +- **Desenvolvedor de produto (consumidor da lib)** — *Objetivo:* ver latência/erro por endpoint agregado. *Frustração:* cada requisição vira uma "operação" diferente; impossível agrupar por rota. +- **Responsável por privacidade/compliance** — *Objetivo:* garantir que dado pessoal só exista onde é previsto e protegido. *Frustração:* CPF/chave Pix aparecem crus na telemetria de traces. + +### Histórias de usuário + +**US-1 — Nome de operação sem dado pessoal** +> Como responsável por privacidade, quero que os nomes das operações HTTP nunca contenham identificadores pessoais (CPF, chave Pix, etc.), para que a telemetria não seja um vetor de exposição de dado pessoal. +- **Critério:** nenhum identificador concreto da requisição aparece no nome da operação; o nome usa o padrão de rota. +- **Critério:** dado pessoal continua fora do nome mesmo para rotas com parâmetros numéricos, slugs ou outros formatos (não só formatos específicos). + +**US-2 — Métricas de operação agregáveis e baratas** +> Como engenheiro de plataforma, quero que operações semelhantes compartilhem o mesmo nome, para que as métricas fiquem em baixa cardinalidade e o custo/consulta do backend seja saudável. +- **Critério:** requisições para o mesmo endpoint (diferindo só nos identificadores) compartilham um único nome de operação. +- **Critério:** a cardinalidade de nomes de operação por serviço cai para a ordem do número de rotas do serviço (não do número de requisições). + +**US-3 — Análise por rota preservada** +> Como desenvolvedor de produto, quero continuar filtrando e agrupando telemetria por rota, para não perder capacidade de análise que já tenho hoje. +- **Critério:** a informação de rota (padrão da URL) continua disponível para filtro/agrupamento nas métricas e nos traces. +- **Critério:** requisições sem rota reconhecida (ex.: caminho inexistente / 404) não são forçadas a um nome enganoso — o nome degrada de forma segura e não polui as séries. + +**US-4 — Consistência entre sinais** +> Como engenheiro de o11y, quero que o nome da operação e a métrica de duração da requisição concordem sobre "qual é a rota", para navegar entre métrica e trace sem discrepância. +- **Critério:** o nome da operação e a métrica de duração referenciam a mesma rota para a mesma requisição. + +### Métricas de sucesso +- **Cardinalidade:** redução ≥95% no número de nomes de operação distintos do serviço mais afetado (baseline: ~50k → ordem de dezenas). +- **Privacidade:** 0 identificadores pessoais detectáveis em nomes de operação novos após o rollout (amostragem no backend de traces). +- **Sem regressão de análise:** capacidade de filtrar/agrupar por rota mantida (métrica de duração por rota continua funcionando). +- **Adoção:** consumidores conseguem atualizar a biblioteca com um caminho de migração claro para o impacto de mudança de comportamento. + +--- + +## 3. Escopo + +### Dentro do escopo +- Corrigir, na biblioteca, o nome das operações HTTP recebidas para usar o padrão de rota em vez do caminho concreto. +- Garantir o comportamento seguro para requisições sem rota reconhecida (nome degradado, sem poluir séries). +- Garantir que a informação de rota permaneça disponível para análise (não remover capacidade existente). +- Fornecer um caminho de migração/comunicação para consumidores, dado que é **mudança de comportamento observável** (ver Assunções). +- Cobertura de testes que garanta o novo comportamento e previna regressão. + +### Fora do escopo +- **Corrigir o dado pessoal já armazenado** no backend de traces (histórico). Esta mudança previne vazamentos **futuros**; a limpeza do histórico é um esforço separado. +- **Remover a mitigação do pipeline central** existente. Pode ser reavaliada depois que o fix estiver difundido nos consumidores — decisão separada. +- **Mudanças em como cada serviço consumidor registra/ordena seus componentes de middleware.** Tratado como feature separada (repo do consumidor). *Nota: a pesquisa indicou uma dependência de ordem que pode exigir coordenação entre a biblioteca e o consumidor — a resolução técnica dessa dependência é decidida no TRD, não aqui.* +- Escolhas de implementação (como renomear a operação com segurança, se haverá ponto de extensão para customização, alinhamento de convenções internas) — **deferidas ao TRD**. + +### Assunções +- A informação de rota (padrão da URL) **já é conhecida** pela biblioteca no momento adequado (a métrica de duração já a utiliza corretamente hoje) — logo o nome da operação pode ser alinhado à mesma fonte. +- A mudança do formato do nome da operação é **breaking change** do ponto de vista de versionamento: dashboards e buscas de consumidores que agrupam pelo nome antigo (com caminho concreto) serão impactados. O rollout deve comunicar isso e seguir o processo de release da biblioteca (pré-lançamento antes de estável). + +### Dependências de negócio +- Consumidores precisarão atualizar a versão da biblioteca para receber o fix; produtos regulados (fluxo Pix/BACEN) são prioridade pelo componente de privacidade. +- Comunicação com times donos de dashboards que hoje dependem do nome de operação antigo. + +--- + +## 4. Diferenciação / Justificativa +- **Alinha ao padrão de indústria** de nomeação de operações HTTP (método + padrão de rota), que é a prática recomendada consolidada para observabilidade — reduz surpresa para novos consumidores e ferramentas. +- **Corrige na origem** em vez de remediar no pipeline: resolve custo **e** privacidade de uma vez, e para todos os consumidores da biblioteca, eliminando remendos por serviço. +- **ROI:** elimina ~72% de séries desperdiçadas no maior ofensor (custo direto de armazenamento/consulta) e remove um risco regulatório concreto (dado pessoal em telemetria). + +--- + +## Gate 1 — Validação +| Categoria | Status | +|---|---| +| Problema articulado (1-2 frases) | ✅ | +| Impacto quantificado | ✅ (~50k nomes, ~72% séries, CPF/chave Pix expostos) | +| Usuários identificados | ✅ (plataforma, produto, compliance) | +| Features endereçam o problema | ✅ (US-1 privacidade, US-2 cardinalidade, US-3/4 análise) | +| Métricas mensuráveis | ✅ | +| Escopo in/out explícito | ✅ | +| Requisito regulatório documentado | ✅ (LGPD — dado pessoal em telemetria) | +| Decisões técnicas deferidas ao TRD | ✅ (hazard de ordem, ponto de extensão, semconv) | + +**Resultado:** ✅ PASS → Gate 2 (TRD) diff --git a/docs/pre-dev/fix-span-name-cardinality-pii/research.md b/docs/pre-dev/fix-span-name-cardinality-pii/research.md new file mode 100644 index 0000000..e2f038f --- /dev/null +++ b/docs/pre-dev/fix-span-name-cardinality-pii/research.md @@ -0,0 +1,109 @@ +# Gate 0 — Research: Fix span.name high cardinality + PII (Fiber middleware) + +## Metadata +- **Date:** 2026-07-17 +- **Feature:** fix-span-name-cardinality-pii +- **Repo:** lib-observability (Go 1.26.3, Fiber v2.52.13, OTel Go SDK v1.44.0, semconv v1.34.0) +- **Research mode:** modification (extending existing middleware) +- **Track:** Small (4 gates) +- **Agents dispatched:** repo-research-analyst (primary), best-practices-researcher, framework-docs-researcher + +## Executive Summary +O middleware Fiber (`middleware/telemetry.go`) nomeia o span SERVER a partir do **path resolvido** (`c.Path()`, só com UUID mascarado) no momento da criação (linha 213/223), enquanto o `http.route` (template, baixa cardinalidade) só é aplicado como atributo **depois** do `c.Next()`. Isso causa (a) explosão de cardinalidade via spanmetrics connector, que usa `span.name` como dimensão default, e (b) vazamento de PII (chave Pix/CPF crua no nome). O fix canônico do OTel é renomear o span para `{method} {http.route}` após o routing. **Descoberta crítica:** o middleware `EndTracingSpans`, quando registrado antes das rotas (padrão do plugin consumidor), encerra o span ANTES do `applyTelemetrySpanAttributes` rodar → qualquer `SetName`/`SetAttributes` pós-`c.Next()` é silenciosamente descartado. O fix precisa lidar com essa ordem. + +## Research Mode +**modification** — estamos alterando o comportamento de naming de span num middleware existente e testado. Foco primário em codebase (padrões, testes que travam comportamento, interação de componentes). Best practices e framework docs validaram a abordagem e revelaram caveats (sampling, fasthttp buffer, PII). + +--- + +## Codebase Research (file:line) + +### Ciclo de vida do span SERVER (`middleware/telemetry.go`, handler em `WithTelemetry`, começa :159) +| Evento | Linha | Detalhe | +|---|---|---| +| Nome do span computado | `:213` | `method + " " + replaceUUIDWithPlaceholder(c.Path())` — **path cru** (só UUID mascarado) | +| Span criado | `:223` | `tracer.Start(traceCtx, routePathWithMethod, trace.WithSpanKind(trace.SpanKindServer))` | +| `defer endState.End()` | `:226` | fim garantido do próprio WithTelemetry (idempotente via `sync.Once`, :78-93) | +| endState no contexto | `:230-231` | compartilhado — é como o EndTracingSpans acha o mesmo span | +| `c.Next()` | `:238` | handlers downstream rodam | +| status reconciliado | `:243` | `httpStatusCode(c, err)` | +| `applyTelemetrySpanAttributes` | `:245-254` | roda APÓS c.Next() | +| `http.route` setado (SetAttributes) | `:295-297`, `:311` | via `routeAttribute`; template correto | + +### Helper de rota (`middleware/helpers.go`) +- `routeAttribute(c, effectiveStatus) (string,bool)` — `:50-65`. Template de `c.Route().Path` (`:64`). Fallback 404: omite se `status==404 && r.Path=="/" && c.Path()!="/"` (`:60-62`). **Correto por spec OTel.** +- `replaceUUIDWithPlaceholder` — `:222-225`; regex `:16-17` só casa UUID canônico 8-4-4-4-12. **NÃO mascara IDs numéricos (`06881656483`), slugs, CPF/CNPJ.** Origem do vazamento no nome. + +### ⚠️ Interação EndTracingSpans (o ponto arquitetural crítico) +- `EndTracingSpans` — `middleware/telemetry.go:399-422`; encerra span via `state.End()` (`:413`). É middleware **separado**, registrado independentemente pelo consumidor (sem wiring no repo). +- **Hazard confirmado:** se registrado `WithTelemetry` (outer) → `EndTracingSpans` (inner, antes das rotas), o unwind LIFO faz `EndTracingSpans.state.End()` (:413) rodar ANTES do `WithTelemetry.applyTelemetrySpanAttributes` (:245). Span OTel é imutável após `End()` → `SetName`/`SetAttributes`/`SetStatus` viram **no-op silencioso**. Hoje isso já faz o plugin perder `http.route`, `status_code`, `error.type` nas rotas registradas após o EndTracingSpans. gRPC análogo: `EndTracingSpansInterceptor` :506-523. + +### Métrica nativa (modelo do fix — já correto) +- `http.server.request.duration` — const `:31`, record `recordHTTPServerDuration` `:350-381` (record `:380`). Usa `http.route` template via `routeAttribute` (`:367-369`), NÃO span.name. Docstring `:330-332` confirma. **O lado métrica já está certo; o problema é só o span name.** + +### Testes que travam o comportamento atual (precisam mudar) +- `telemetry_test.go:172-187` (`TestWithTelemetry`): span name = `method + " " + expectedPath` (path cru UUID-masked). **Trava naming cru.** +- `telemetry_test.go:284-292` (`TestWithTelemetryExcludedRoutes`): `expectedSpanName := method + " " + replaceUUIDWithPlaceholder(path)`. **Trava naming cru.** +- `telemetry_test.go:378-386`, `:435-438`: semântica de End do EndTracingSpans. +- `telemetry_route_test.go:41`: span criado mesmo em 404. +- `telemetry_metrics_test.go:166-168`: `http.route=="/api/users/:id"` na métrica ("must use route template, never raw path") — **é o modelo do fix**. +- `telemetry_metrics_test.go:679-686`, `:704-711`: http.route ausente em 404 / `"/"` em root real. +- Faltam testes unitários isolados de `routeAttribute` e `replaceUUIDWithPlaceholder` — adicionar. + +### Sem hook público de naming +Não existe `SpanNameFormatter`/`WithSpanNameFormatter`. Naming 100% interno (:213/:223). Consumidor não tem override. Único knob: `excludedRoutes ...string` (:132). Porém o formato do span name é um **contrato comportamental observável** (dashboards/Tempo), relevante p/ versionamento. + +### docs/solutions/ +**Ausente.** Prior art = CHANGELOG v1.1.0 (:5-25): já houve trabalho de baixa cardinalidade no lado métrica ("Omit http.route for unmatched 404", "Normalize http.request.method"). O fix de span name é o próximo passo lógico, mesma abordagem semconv-driven. + +### Convenções (CLAUDE.md:26-41) +- Commit: `git commit -S -m "type(scope): message" --trailer "X-Lerian-Ref: 0x1"` — **GPG-signed obrigatório** + trailer. +- Conventional commits (feat/fix/chore/docs/refactor/test). Este = `fix(middleware): ...`. +- CI: `golangci-lint run ./...`; `go test -tags=unit ./...` (testes carregam `//go:build unit`); mocks via mockgen commitados. +- Release: semantic-release. **develop → beta**, **main → stable**. Mudança de formato do span name é behavior change (semver-relevante). + +--- + +## Best Practices Research (URLs) + +- **Span name HTTP server = `{method} {http.route}` (template)** — https://opentelemetry.io/docs/specs/semconv/http/http-spans/ — spec estável (2023). Exemplo canônico `GET /webshop/articles/:article_id`. "Instrumentation MUST NOT default to using URI path as `{target}`" — path resolvido é **proibido**. +- **http.route omitido em unmatched/404** — mesma spec. Fallback correto: nome degrada p/ só `{method}`, sem http.route. Confirma o helper atual. +- **`span.SetName()` pós-start/pré-End é suportado** (spec `UpdateName`) — https://opentelemetry.io/docs/specs/otel/trace/api/. **Caveat:** decisão de sampling foi tomada na criação com o nome original e NÃO é reavaliada no rename. +- **Nunca embutir ID/path/PII no nome** — https://opentelemetry.io/blog/2025/how-to-name-your-spans/ — IDs vão em atributos, não no nome. +- **PII em telemetria** — https://opentelemetry.io/docs/security/handling-sensitive-data/ — minimização; scrubbing de url.path; hash de CPF é reversível (espaço pequeno) → preferir drop/truncate. Usar template no nome exclui PII estruturalmente. +- **spanmetrics usa span.name como dimensão default** — https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/connector/spanmetricsconnector/README.md — template no nome = cardinalidade limitada. Failure mode documentado: `GET /product/1YMWWN1N4O`. Safety-net collector `set_semconv_span_name()` (complementar, não substituto). + +--- + +## Framework Documentation (versões) + +- **Fiber v2.52.13** — `c.Route().Path` = template; **confiável só APÓS `c.Next()`** (doc avisa explicitamente). `c.Path()` = resolvido. Middleware `app.Use` deve ser registrado ANTES das rotas que envolve; unwind LIFO (código após `c.Next()` = fase de resposta, rota já casada). +- **fasthttp v1.71.0** (indirect) — buffer recycling: strings de request (`c.Method()`, `c.OriginalURL()`...) precisam ser copiadas antes do `c.Next()` (já feito). **`c.Route().Path` NÃO precisa** (string do router, não do buffer) — safe pós-Next. +- **OTel Go v1.44.0** — `trace.Span.SetName(string)` existe e é válido pós-Start/pré-End (no-op se não-recording; guardar com `IsRecording()` opcional). Sem version skew entre otel/sdk/trace/metric. +- **semconv v1.34.0** — keys estáveis `http.route`, `http.request.method`, `http.response.status_code` (helpers `semconv.HTTPRoute(...)` etc.). Lib usa string literal idêntico hoje (drift médio: bump futuro não é pego pelo compilador; poderia migrar de brinde). + +--- + +## Synthesis + +### Padrões a seguir +- Modelar o fix no lado métrica já-correto: `recordHTTPServerDuration` usa `routeAttribute` pós-`c.Next()` (`telemetry.go:367-369`). Renomear o span no mesmo ponto. +- Nome = `{method} {http.route}` quando `routeAttribute` retorna `present`; fallback = nome provisório só-método (NÃO path cru) quando não casou rota. +- Migração opcional dos literais p/ `semconv.*` helpers enquanto edita. + +### Constraints identificadas +1. **EndTracingSpans encerra o span antes do post-Next** (telemetry.go:413 vs :245) → `SetName`/`SetAttributes` pós-End são no-op. **O fix DEVE resolver isso**, não assumir. Opções: (a) aplicar nome/atributos antes de qualquer End compartilhado disparar; (b) coordenar o ciclo de vida do endState; (c) repensar a relação WithTelemetry/EndTracingSpans. **Acopla esta feature ao fix de ordem do plugin.** +2. **fasthttp buffer** — não introduzir leitura de request-string pós-Next sem cópia (c.Route().Path é exceção segura). +3. **Sampling** — rename tardio não reavalia sampling. Se houver sampler por nome, decisão fica no nome original. (Provavelmente não afeta head sampling atual, mas documentar.) +4. **Contrato comportamental** — mudança do formato do span name afeta dashboards/queries Tempo e é semver-relevante. +5. **Testes** — 2 testes travam o naming cru (telemetry_test.go:172-187, :284-292) → reescrever espelhando o teste da métrica. + +### Prior solutions (docs/solutions/) +Ausente. CHANGELOG v1.1.0 mostra trabalho análogo de baixa cardinalidade no lado métrica — mesma filosofia. + +### Open questions para o PRD/TRD +1. **Como resolver o hazard do EndTracingSpans?** Tornar a lib robusta à ordem (aplicar nome/attrs antes do End) OU depender do fix de ordem no plugin? (decisão de design central — afeta se a lib sozinha resolve ou precisa do plugin junto) +2. **Introduzir `SpanNameFormatter` opt-in** agora, ou só corrigir o naming internamente? (escopo) +3. **Migrar literais → semconv helpers** neste PR, ou manter escopo mínimo? +4. **É breaking change?** O formato do span name muda — precisa footer `BREAKING CHANGE`/bump major, ou é `fix` normal? (afeta consumidores com dashboards por span name) +5. **Fallback de nome em 404/unmatched:** só `{method}`, ou `{method} {path-uuid-masked}` como hoje? (spec prefere só método) diff --git a/docs/pre-dev/fix-span-name-cardinality-pii/tasks.md b/docs/pre-dev/fix-span-name-cardinality-pii/tasks.md new file mode 100644 index 0000000..306abc6 --- /dev/null +++ b/docs/pre-dev/fix-span-name-cardinality-pii/tasks.md @@ -0,0 +1,157 @@ +# Tasks — Fix: span.name HTTP com alta cardinalidade e PII + +## Metadata +- **Date:** 2026-07-17 +- **Feature:** fix-span-name-cardinality-pii +- **Gate:** 3 (Task Breakdown — final do Small Track) +- **Inputs:** [research.md](./research.md), [prd.md](./prd.md), [trd.md](./trd.md) +- **Confidence:** 88/100 (design revisado por eng. Go; tasks pequenas, critérios testáveis; risco residual = contrato observável do nome) + +## ⚠️ Alvo: Fiber v3 (branch `fix/span-name-cardinality-pii` a partir de `develop`) +A `develop` já migrou para **Fiber v3.4.0** (a `main` é v2.52.13). O research/TRD foi feito em v2, mas a estrutura do código em v3 é **idêntica** nos pontos do fix. Diferenças de API a usar (confirmadas no código da develop): +- `c.UserContext()` → `c.Context()`; `c.SetUserContext()` → `c.SetContext()` (telemetry.go:215/231). +- `EndTracingSpans(c fiber.Ctx)` / `routeAttribute(c fiber.Ctx, ...)` — `fiber.Ctx` é interface na v3 (sem ponteiro). +- `c.Route().Path`, guard 404 (helpers.go:60), `replaceUUIDWithPlaceholder`, `uuidPattern` — **inalterados**. +O núcleo do fix (span name :213/:223, applyTelemetrySpanAttributes :245, defer :226, EndTracingSpans :399-421) é conceitualmente igual ao TRD. + +## Sequenciamento +``` +T-001 (ownership do ciclo de vida) ──► T-002 (nomeação por template) + │ + └─► ambas cobertas por T-003 (testes/robustez) +T-004 (semconv literais) — opcional, independente +``` +T-001 é foundation (habilita o rename funcionar); T-002 entrega o valor (nome correto); T-003 é o DoD de robustez que valida os dois. Entregar como **um único PR** (mudança coesa e breaking), mas as tasks organizam a implementação e a verificação. + +--- + +## T-001: Span raiz HTTP encerrado de forma determinística (ownership) +**Type:** Foundation + +**Deliverable:** O span raiz criado pelo `WithTelemetry` é finalizado e encerrado por ele mesmo, exatamente uma vez, **independente da ordem** em que o consumidor registra `WithTelemetry`/`EndTracingSpans`. + +**Scope:** +- Inclui: sinal de ownership no `spanEndState`; `WithTelemetry` como único encerrador do seu span raiz; `EndTracingSpans` pula spans owned; preservar fallback de span estrangeiro. +- Exclui: a nomeação em si (T-002); reclassificação de error.type (fora de escopo). + +**Success Criteria:** +- **Funcional:** atributos pós-`c.Next()` (`http.route`, status, error.type) são aplicados ao span em **todas** as ordens de registro — hoje são perdidos quando `EndTracingSpans` está na cadeia. +- **Funcional:** span raiz encerrado exatamente uma vez em todas as 3 permutações posicionais e também **sem** `EndTracingSpans` registrado. +- **Funcional:** span estrangeiro/handler-criado continua encerrado pelo `EndTracingSpans` (fallback intacto — `TestEndTracingSpans_EndsFinalContextSpan` passa). +- **Técnico:** sem double-end, sem race (single-goroutine + flag; `sync.Once` mantido como defesa). +- **Qualidade:** 14 linters + `forbidigo` passam. + +**User/Technical value:** habilita o rename de T-002 a funcionar de fato (sem isso, o rename seria descartado); e já corrige a perda silenciosa de `http.route`/status/error.type que existe hoje. + +**Dependencies:** Blocks T-002. Requires: nenhum. + +**Effort:** S (2-3 pts, ~1-2 dias). + +**Risks:** +- *Mudança de comportamento do `EndTracingSpans` (deixa de encerrar o root do WithTelemetry).* Impacto: médio; Prob: média. Mitigação: no-op escopado só ao branch owned; fallback preservado; testes das 3 permutações. Fallback: reverter para encerramento coordenado (ADR-001 opção b). + +**Testing:** unit (permutações de ordem, encerramento único, fallback estrangeiro). + +**DoD:** revisado, testes passando, linters limpos, sem regressão em testes existentes de `EndTracingSpans`. + +--- + +## T-002: Nome de operação HTTP = `{método} {rota}` (sem PII, baixa cardinalidade) +**Type:** Feature (entrega o valor principal do PRD — US-1, US-2, US-4) + +**Deliverable:** Spans HTTP server são nomeados pelo template de rota (`GET /v1/dict/statistics/keys/:key`), nunca pelo caminho concreto com identificadores; nome livre de PII inclusive no momento da criação. + +**Scope:** +- Inclui: nome na **criação** = só-método (PII-free para o sampler); rename para `{método} {rota}` após roteamento, guardado por `IsRecording()`; fallback só-método quando rota não reconhecida (reusa guard 404 existente). +- Exclui: hook público de formatação (não incluído — ADR-004); mudança na métrica de duração (já correta). + +**Success Criteria:** +- **Funcional:** requisições ao mesmo endpoint com identificadores diferentes compartilham um único nome de span. +- **Funcional/Privacidade:** nenhum identificador concreto (CPF/chave Pix/numérico/slug) aparece no nome — nem no nome de criação, nem no final, nem em rotas não casadas. +- **Funcional:** `http.route` permanece disponível como atributo (US-3); nome e métrica de duração concordam sobre a rota (US-4). +- **Funcional:** rota não reconhecida (404/catch-all) → nome só-método, `http.route` omitido. +- **Qualidade:** convenção OTel `{method} {http.route}` respeitada. + +**User/Technical value:** elimina a explosão de cardinalidade e o vazamento de PII na origem, para todos os consumidores da lib. + +**Dependencies:** Requires T-001 (senão o rename é descartado). + +**Effort:** S (3 pts, ~1-2 dias). + +**Risks:** +- *Breaking change do formato do nome* (dashboards/queries por span name antigo). Impacto: médio-alto; Prob: alta. Mitigação: nota de release BREAKING + comunicação a donos de dashboard; versionamento (beta antes de stable). Fallback: N/A (é o comportamento desejado). +- *Sampling por nome de span* usa nome de criação (agora só-método). Mitigação: documentar na release; nome só-método já é PII-free. + +**Testing:** unit (nome por template; nome PII-free na criação; fallback 404; consistência nome↔`http.route`). + +**DoD:** revisado, testes passando, nota BREAKING preparada, linters limpos. + +--- + +## T-003: Cobertura de testes e robustez (DoD transversal de T-001+T-002) +**Type:** Foundation (garantia de não-regressão) + +**Deliverable:** Suíte de testes que trava o novo comportamento e previne regressão da cardinalidade/PII e do ciclo de vida do span. + +**Scope:** +- Inclui: atualizar testes que travam o nome antigo; testes de robustez (3 permutações + sem EndTracingSpans + span estrangeiro); testes unitários novos de `routeAttribute` e do mascaramento de identificadores; assert de nome PII-free na criação. +- Exclui: testes de integração fora do escopo unit. + +**Success Criteria:** +- **Técnico:** testes que asseravam o nome cru (path) atualizados para asserir o nome por template. +- **Técnico:** existem testes cobrindo as 3 permutações posicionais + "sem EndTracingSpans" + span estrangeiro. +- **Técnico:** `routeAttribute` e o mascaramento têm testes unitários isolados (hoje inexistentes). +- **Qualidade:** tag `//go:build unit` mantida; mocks via mockgen; `go test -tags=unit ./...` verde; cobertura conforme padrão do repo. + +**User/Technical value:** garante que o fix não regrida e documenta o contrato novo via testes. + +**Dependencies:** Requires T-001, T-002. + +**Effort:** S-M (3-5 pts, ~2 dias). + +**Risks:** +- *Testes existentes acoplados ao comportamento antigo além dos 2 mapeados.* Mitigação: rodar a suíte completa; ajustar os que quebrarem por dependerem do nome cru. + +**Testing:** é a própria task. + +**DoD:** suíte verde, cobertura ok, linters limpos. + +--- + +## T-004 (opcional): Alinhar literais de atributo aos helpers canônicos +**Type:** Polish + +**Deliverable:** Atributos HTTP usam os helpers canônicos de convenção em vez de strings literais, se trivial e baixo risco. + +**Scope:** inclui só a troca literal→helper nos atributos já emitidos. Exclui qualquer mudança de valor/semântica; exclui reclassificação de error.type (proibida neste fix). + +**Success Criteria:** valores de atributo idênticos aos atuais (sem mudança observável); compila e testes passam. + +**User/Technical value:** remove drift latente (bump futuro de convenção pego pelo compilador). + +**Dependencies:** Optional; independente de T-001/T-002. + +**Effort:** S (1 pt). **Só fazer se não aumentar risco/escopo do PR.** + +**Risks:** baixo. Mitigação: se qualquer valor divergir, não incluir. + +--- + +## Release (aplicável a todas as tasks — um PR coeso) +- Commit **GPG-signed** + trailer `X-Lerian-Ref`; conventional commit `fix(middleware): ...`. +- **Footer `BREAKING CHANGE`** (formato do span name muda; dashboards por span name antigo afetados). +- Target **`develop`** (beta) → validar → **`main`** (stable). Semantic-release cuida do versionamento. +- Nota de release comunicando: mudança de nome de span (método+rota), impacto em dashboards/queries por nome antigo, e o caveat de sampling por nome. + +## Gate 3 — Validação +| Categoria | Status | +|---|---| +| Todos os componentes do TRD cobertos | ✅ (ADR-001→T-001, ADR-002→T-002, testes→T-003, ADR-004→T-004) | +| Cada task entrega incremento funcional | ✅ | +| Critérios de sucesso testáveis | ✅ | +| Dependências mapeadas | ✅ | +| Nenhuma task > 2 semanas | ✅ (todas S/S-M) | +| Estratégia de teste definida | ✅ | +| Riscos com mitigação | ✅ | + +**Resultado:** ✅ PASS — pre-dev Small Track COMPLETO. Pronto para implementação (dev-cycle). diff --git a/docs/pre-dev/fix-span-name-cardinality-pii/trd.md b/docs/pre-dev/fix-span-name-cardinality-pii/trd.md new file mode 100644 index 0000000..9a4fcce --- /dev/null +++ b/docs/pre-dev/fix-span-name-cardinality-pii/trd.md @@ -0,0 +1,131 @@ +# TRD — Fix: span.name HTTP com alta cardinalidade e PII + +## Metadata +- **Date:** 2026-07-17 +- **Feature:** fix-span-name-cardinality-pii +- **Gate:** 2 (TRD, Small Track) +- **Inputs:** [research.md](./research.md), [prd.md](./prd.md) +- **deployment.model:** N/A (biblioteca compartilhada, não serviço) +- **tech_stack.primary:** Go (backend library) +- **tech_stack.standards_loaded:** golang/bootstrap.md (observability), golang/quality.md (linting/testing) +- **Confidence:** 90/100 (revisado por engenheiro Go; premissa do hazard corrigida, mecânica de ownership estabelecida, escopo do EndTracingSpans delimitado, nome PII-free na criação) +- **Go review:** ✅ Opção (a) validada como SÓLIDA com 4 correções aplicadas (premissa LIFO, flag owned vs sync.Once, nome PII-free na criação, no-op escopado a spans owned) + +--- + +## 1. Análise / NFRs + +| NFR | Alvo | +|---|---| +| **Cardinalidade** | Nome de operação HTTP em baixa cardinalidade (ordem de nº de rotas, não de requisições) | +| **Privacidade** | Zero identificador concreto (PII) no nome da operação, para qualquer formato de parâmetro | +| **Robustez** | Correção funciona **independente da ordem** em que o consumidor registra os middlewares de telemetria | +| **Compatibilidade** | Métrica de duração HTTP existente (que já usa o template de rota) **não** pode regredir; fallback 404 preservado | +| **Não-regressão de análise** | Rota permanece disponível como atributo para filtro/agrupamento | + +### Mapeamento PRD → componentes +- US-1 (privacidade) + US-2 (cardinalidade) → **nomeação do span pelo template de rota** (componente: middleware HTTP de telemetria). +- US-3 (análise por rota) → **atributo de rota preservado** + **fallback seguro** para rota não reconhecida. +- US-4 (consistência) → nome do span e métrica de duração derivam da **mesma fonte de rota**. + +--- + +## 2. Arquitetura da Solução + +### Estilo +Modificação pontual de um **componente de middleware HTTP** dentro de uma biblioteca de observabilidade. Sem novo componente, sem nova fronteira de domínio. Padrão: alinhar o ciclo de vida de nomeação/finalização do span à convenção de indústria (método + template de rota). + +### Componentes envolvidos +| Componente | Responsabilidade | Mudança | +|---|---|---| +| Middleware de telemetria HTTP (`WithTelemetry`) | Cria o span raiz da requisição, captura atributos, finaliza | Passa a **nomear o span pelo template de rota** e a **finalizar nome+atributos antes de qualquer encerramento** | +| Middleware de encerramento (`EndTracingSpans`) | Encerra o span raiz | Passa a ser **idempotente e seguro** — não pode "vencer a corrida" e encerrar o span antes da finalização | +| Helper de rota (`routeAttribute`) | Deriva o template + fallback 404 | **Reutilizado** como fonte única da rota (nome + atributo + métrica) | + +### ADR-001 — Como resolver o hazard de ordem do middleware (o problema central) +> **Revisado após review de engenheiro Go (2026-07-17).** A premissa original ("só ocorre com ordem errada") estava incorreta — ver Context corrigido. + +- **Context (CORRIGIDO):** O span raiz é criado por `WithTelemetry` (`telemetry.go:223`) mas o template de rota só é conhecido após o roteamento (`c.Next()`, `:238`), aplicado em `applyTelemetrySpanAttributes` (`:245`). O encerramento é feito hoje pelo middleware separado `EndTracingSpans` (`:399-422`, `state.End()` em `:413`), que compartilha o mesmo `endState` via contexto (`:230`). **Descoberta do review:** pelo LIFO do Fiber, o middleware registrado por ÚLTIMO é o mais interno e desenrola PRIMEIRO. Logo, com `EndTracingSpans` registrado **por último** (a ordem CORRETA do standard), ele ainda encerra o span **antes** do `WithTelemetry` rodar `applyTelemetrySpanAttributes`. **O hazard existe na ordem correta do standard, não só na errada.** Consequência: `http.route`, `status_code` e `error.type` já são perdidos hoje em qualquer consumidor que use os dois middlewares — é um defeito real, não só misconfiguração. (Spans OTel são imutáveis após `End()` → `SetName`/`SetAttributes` viram no-op silencioso.) +- **Ring Standard (observability):** *"telemetry middleware must be first; EndTracingSpans must be last."* A ordem do standard é necessária mas **não suficiente** — mesmo seguindo-a, o defeito ocorre. A lib precisa se apropriar da finalização; não pode depender da ordem do consumidor. +- **Ordenação interna do `WithTelemetry` já está correta** (confirmado no review): `applyTelemetrySpanAttributes` (`:245`) roda antes do `defer endState.End()` (`:226`), que só dispara no `return` (`:258`). O problema é exclusivamente o `EndTracingSpans` **externo** encerrando antes. +- **Options:** + - **(a) `WithTelemetry` é o ÚNICO encerrador do seu span raiz + `EndTracingSpans` pula spans "owned".** `WithTelemetry` finaliza (nome+atributos+status) em `:245` e encerra via seu `defer` (`:226`); `EndTracingSpans` detecta que o `endState` pertence ao `WithTelemetry` e **não** o encerra. Robusto em todas as ordens. + - **(b) Mover finalização para dentro do encerrador compartilhado** — viável mas força o `EndTracingSpans` a reconstruir estado da requisição (attrs/status/rota) que ele não possui, acoplando-o ao framework HTTP. Mais sujo. + - **(c) Nomear na criação** — REJEITADA: template não confiável pré-`c.Next()`. +- **Decision:** **Opção (a), com a mecânica precisa que o review estabeleceu:** + 1. Adicionar um **sinal de ownership** ao `spanEndState` (ex.: flag `owned bool`, `:78-93`), setado pelo `WithTelemetry`. + 2. `WithTelemetry` é o **único** que encerra seu span raiz (via `defer`, `:226`), depois de finalizar em `:245`. + 3. `EndTracingSpans` (`:412-415`): quando o `endState` é `owned`, **não** chama `End()` (retorna). O fallback para spans estrangeiros (`trace.SpanFromContext(...).End()`, `:417-419`) é **preservado intacto**. + 4. Guardar o rename/attrs com `span.IsRecording()` — documenta a intenção "pular se já encerrado" (torna testável em vez de depender do no-op-após-End). +- **`sync.Once` NÃO resolve o hazard** (correção do review): `Once` só previne double-end; **não** garante encerrar *após* a finalização. A ordem é resolvida pelo flag `owned`, não pelo `Once`. O `Once` permanece como defesa contra double-end e para o caminho de span estrangeiro. +- **Rationale:** Torna o `WithTelemetry` autoritativo sobre o ciclo de vida do span que ele cria; elimina a corrida por design (single-goroutine + flag); mantém a capacidade legítima do `EndTracingSpans` de encerrar spans estrangeiros/handler-criados (há teste que depende — `TestEndTracingSpans_EndsFinalContextSpan`). Não depende da ordem do consumidor. +- **Consequences:** + - `EndTracingSpans` deixa de ser quem encerra o span raiz do `WithTelemetry` (passa a no-op nesse caso) — mudança de comportamento observável interna; testes que asseram "EndTracingSpans encerra o root" precisam de ajuste. + - `WithTelemetry` encerra o span raiz em **todos** os caminhos (normal/erro/panic/streaming/hijack) via seu `defer` — o review confirmou que `EndTracingSpans` não é estritamente necessário para o span raiz em nenhum desses. + - Requer teste das **3 permutações posicionais** + caso "sem EndTracingSpans registrado" para provar robustez. + - Aplicar semântica equivalente ao par gRPC (`WithTelemetryInterceptor`/`EndTracingSpansInterceptor`, `:476`/`:506`) para não divergir — **prioridade menor** (gRPC nomeia de `info.FullMethod`, sem PII/cardinalidade), pode ficar fora do escopo mínimo mas documentado. + +### ADR-002 — Fonte e formato do nome do span + +- **Context:** O nome precisa ser baixa cardinalidade e livre de PII — **inclusive no momento da CRIAÇÃO do span**, não só após o rename (correção do review: o sampler enxerga o nome da criação). +- **Decision:** + - **Nome na criação (`:213/:223`) = só `{método}`** (ex.: `GET`), NÃO `{método} {caminho}`. Isso remove PII/cardinalidade do nome que o sampler vê e do caso de rota não reconhecida. + - **Após `c.Next()`**, quando o roteamento reconheceu uma rota, renomear para `{método} {template de rota}` (via o mesmo helper que a métrica já usa). + - **Fallback (404/catch-all):** mantém o nome só-método da criação; não força rota. (Alternativa: manter mascaramento de UUID como enriquecimento do fallback — decidir na task, mas o padrão é só-método.) +- **Rationale:** Convenção estável de indústria para spans HTTP server; exclui PII estruturalmente; alinha nome e métrica (US-4). **Ponto do review:** criar o span já com nome só-método garante que nem o sampler nem spans descartados nem rotas não-casadas carreguem o caminho cru com identificadores. +- **Consequences:** O mascaramento heurístico de UUID (`replaceUUIDWithPlaceholder`) **deixa de ser a defesa principal** — passa, no máximo, a enriquecimento opcional do fallback. Requisições sem rota reconhecida terão nome só-método (correto e de baixa cardinalidade). +- **Caveat de sampling (documentar na release):** o rename pós-criação NÃO reavalia a decisão de amostragem (feita na criação, com o nome só-método agora). Consumidores que amostram por nome de span devem ser avisados — mas com nome só-método na criação, não há PII exposta a samplers. + +### ADR-003 — Distinção da convenção de nomeação (evitar conflito com o standard) + +- **Context:** A Ring Standard exige spans no padrão `layer.domain.operation` (ex.: `service.tenant.create`). O span aqui é o **span raiz HTTP server** criado pelo middleware, não um span de camada de serviço/repositório. +- **Decision:** Spans **raiz HTTP server** seguem a convenção OTel `{método} {http.route}` (a própria standard descreve o middleware criando "root spans for HTTP endpoints"). O padrão `layer.domain.operation` continua válido para spans **internos** (serviço/repositório) criados pelos handlers — **fora do escopo** desta mudança. +- **Rationale:** As duas convenções não conflitam; aplicam-se a camadas diferentes. Documentar evita interpretação equivocada de "violação de standard". + +### ADR-004 — Escopo adicional (decisões deferidas do PRD) + +- **Ponto de extensão de nomeação (hook público de formatação):** **Não incluir agora.** Não há demanda de consumidor; ampliaria a superfície pública da API sem necessidade comprovada. Reavaliar se surgir caso de uso. +- **Alinhamento de convenções internas de atributo (literais → helpers canônicos):** **Incluir como oportunístico** apenas se trivial e baixo risco (o review endossou). Não bloqueante. +- **Reclassificação de error.type (business vs technical):** **NÃO incluir** (correção do review). O código atual marca status ERROR para qualquer erro de handler e ≥500; a reclassificação business/technical é uma mudança de **contrato observável separada** e não deve ser embutida neste fix cirúrgico. Registrar como to-do próprio se relevante. + +--- + +## 3. Qualidade / Testes + +Conforme golang/quality.md e as convenções do repo (research §9): +- **Atualizar** os testes que travam o nome antigo (caminho concreto) para asserir o **nome baseado em template**, espelhando o teste já existente da métrica. +- **Adicionar** testes unitários isolados para o helper de rota e para o mascaramento de identificadores (hoje inexistentes). +- **Testes-chave de robustez (novos, exigidos pelo review):** + - Nome+atributos aplicados corretamente e span encerrado **exatamente uma vez** nas **3 permutações posicionais**: (i) `WithTelemetry` primeiro + `EndTracingSpans` por último (ordem do standard), (ii) `EndTracingSpans` antes das rotas (ordem do plugin), (iii) `EndTracingSpans` fora/antes do `WithTelemetry`. + - Span raiz encerrado corretamente **sem `EndTracingSpans` registrado** (prova que o `WithTelemetry` é autossuficiente). + - Span **estrangeiro/handler-criado** ainda encerrado pelo `EndTracingSpans` (preserva `TestEndTracingSpans_EndsFinalContextSpan` — não quebrar o fallback). +- **Nome PII-free na criação:** asserir que o nome na criação do span é só-método (não contém caminho), garantindo que o sampler não vê PII. +- **Fallback:** testar rota não reconhecida (nome só-método, `http.route` omitido). +- Manter tag de build de teste unitário e mocks gerados conforme o repo. Linters obrigatórios (14) devem passar; `forbidigo` (sem `fmt.Print`/`panic` em lib) respeitado. + +### Caveats técnicos a documentar (research) +- **Reciclagem de buffer do framework:** strings da requisição precisam ser copiadas antes do roteamento; o template de rota é seguro após o roteamento (não é buffer de requisição). +- **Renomear span após criação é válido** no SDK em uso (no-op se o span não estiver sendo gravado — guardar se necessário). +- **Amostragem (sampling):** a decisão de amostragem é feita na criação com o nome original e **não** é reavaliada no rename. Se algum consumidor usa amostragem por nome de span, deve ser avisado (documentar na nota de release). + +--- + +## 4. Versionamento / Rollout (implicação de arquitetura, mecânica é processo) +- Mudança do formato do nome do span é **breaking change** observável (dashboards/buscas que agrupam pelo nome antigo). Deve ser sinalizada como tal no processo de release da biblioteca (pré-lançamento antes de estável) e comunicada aos consumidores. +- **Dependência com a feature do consumidor (plugin):** o ADR-001 torna a lib robusta mesmo com ordem errada, mas a correção de ordem no consumidor (feature separada) continua recomendada por conformidade com o standard. A lib **não depende** dela para funcionar. + +--- + +## Gate 2 — Validação +| Categoria | Status | +|---|---| +| Todos os requisitos do PRD mapeados | ✅ (US-1..US-4) | +| Fronteiras de componente claras | ✅ (WithTelemetry / EndTracingSpans / routeAttribute) | +| Hazard central resolvido por design | ✅ (ADR-001, opção a, revisada) | +| Review de engenheiro Go do ADR-001 | ✅ SÓLIDA + 4 correções aplicadas | +| Estratégia de qualidade/testes | ✅ (3 permutações de ordem + nome PII-free) | +| Quality attributes atingíveis | ✅ | +| Decisões deferidas do PRD resolvidas | ✅ (ADR-002/003/004) | +| Caveats técnicos documentados | ✅ (buffer, sampling, rename, ownership) | + +**Resultado:** ✅ PASS → Gate 3 (tasks) diff --git a/middleware/helpers.go b/middleware/helpers.go index ded5775..661f40b 100644 --- a/middleware/helpers.go +++ b/middleware/helpers.go @@ -13,9 +13,6 @@ import ( "google.golang.org/grpc/metadata" ) -// uuidPattern matches standard UUID v4 strings (8-4-4-4-12 hex digits). -var uuidPattern = regexp.MustCompile(`[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}`) - // internalServicePattern matches Lerian internal service user-agent strings. var internalServicePattern = regexp.MustCompile(`^[\w-]+/[\d.]+\s+LerianStudio$`) @@ -219,11 +216,6 @@ func isInternalLerianService(userAgent string) bool { return internalServicePattern.MatchString(userAgent) } -// replaceUUIDWithPlaceholder replaces UUIDs with a placeholder in a given path string. -func replaceUUIDWithPlaceholder(path string) string { - return uuidPattern.ReplaceAllString(path, ":id") -} - // isNilOrEmptyString reports whether a string pointer is nil or the trimmed value is empty. // "null" and "nil" are treated as empty to handle JSON null serialization artifacts // where some encoders emit the literal string "null" or "nil" instead of a JSON null. diff --git a/middleware/helpers_test.go b/middleware/helpers_test.go index f80cc87..21a0750 100644 --- a/middleware/helpers_test.go +++ b/middleware/helpers_test.go @@ -64,3 +64,86 @@ func TestIsRouteExcludedFromList(t *testing.T) { }) } } + +// TestRouteAttribute_NilContext verifies the nil-context guard returns absent. +func TestRouteAttribute_NilContext(t *testing.T) { + route, present := routeAttribute(nil, http.StatusOK) + assert.False(t, present, "nil ctx must report the route as absent") + assert.Empty(t, route) +} + +// TestRouteAttribute_MatchedRoute verifies that a matched :param route yields +// the route template (never the concrete path) with present==true. +func TestRouteAttribute_MatchedRoute(t *testing.T) { + app := fiber.New() + + var ( + gotRoute string + gotPresent bool + ) + + app.Get("/api/users/:id", func(c fiber.Ctx) error { + gotRoute, gotPresent = routeAttribute(c, http.StatusOK) + return c.SendStatus(http.StatusOK) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/api/users/42", nil)) + assert.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + assert.True(t, gotPresent, "matched route must be present") + assert.Equal(t, "/api/users/:id", gotRoute, + "must return the route template, never the concrete path") +} + +// TestRouteAttribute_UnmatchedCatchAll404 verifies the Fiber catch-all guard: +// an unmatched request (effective 404, Route().Path=="/", request path != "/") +// reports the route as absent so callers can omit http.route. +func TestRouteAttribute_UnmatchedCatchAll404(t *testing.T) { + app := fiber.New() + + var ( + gotRoute string + gotPresent bool + ) + + // A catch-all middleware runs for the unmatched path so we can inspect the + // context Fiber assembled for the 404. No route is registered for the path. + app.Use(func(c fiber.Ctx) error { + gotRoute, gotPresent = routeAttribute(c, http.StatusNotFound) + return c.Next() + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/definitely/not/registered", nil)) + assert.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, http.StatusNotFound, resp.StatusCode) + + assert.False(t, gotPresent, + "unmatched catch-all 404 must report the route as absent") + assert.Empty(t, gotRoute) +} + +// TestRouteAttribute_RegisteredRoot verifies that a legitimately-registered +// root ("/") handler retains the route (present==true), distinguishing it from +// the catch-all 404 case that also exposes Route().Path=="/". +func TestRouteAttribute_RegisteredRoot(t *testing.T) { + app := fiber.New() + + var ( + gotRoute string + gotPresent bool + ) + + app.Get("/", func(c fiber.Ctx) error { + gotRoute, gotPresent = routeAttribute(c, http.StatusOK) + return c.SendStatus(http.StatusOK) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/", nil)) + assert.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + assert.True(t, gotPresent, "registered root handler must retain the route") + assert.Equal(t, "/", gotRoute) +} diff --git a/middleware/telemetry.go b/middleware/telemetry.go index 0278b13..327b6b8 100644 --- a/middleware/telemetry.go +++ b/middleware/telemetry.go @@ -77,7 +77,20 @@ type spanEndStateKey struct{} type spanEndState struct { span trace.Span + // once guarantees End() is idempotent. It is retained (not superseded by + // owned) because it still protects the paths where owned does not apply: + // the gRPC interceptor pair (which ends via both defer and + // EndTracingSpansInterceptor) and the foreign/handler-created span on the + // HTTP fallback path. owned resolves ordering; once resolves double-end. once sync.Once + // owned marks the span as exclusively finalized/ended by the middleware + // that created it (WithTelemetry). When set, EndTracingSpans must NOT end + // it: the owning middleware ends it via its own deferred End() AFTER + // applying the route template name and post-c.Next() attributes. This + // removes the ordering hazard where a separately-registered EndTracingSpans + // unwinds first (Fiber LIFO) and ends the span before finalization, + // silently discarding http.route/status/error.type and the span rename. + owned bool } func newSpanEndState(span trace.Span) *spanEndState { @@ -210,7 +223,14 @@ func (tm *TelemetryMiddleware) WithTelemetry(tl *tracing.Telemetry, excludedRout userAgent := string([]byte(c.Get(headerUserAgent))) tracer := effectiveTelemetry.TracerProvider.Tracer(effectiveTelemetry.LibraryName) - routePathWithMethod := method + " " + replaceUUIDWithPlaceholder(c.Path()) + // Create the span with a method-only name (e.g. "GET"). The route + // template is not reliably known until after routing (c.Next), and the + // concrete path carries PII / unbounded cardinality (IDs, Pix keys). A + // method-only creation name keeps PII out of the name the sampler sees + // and out of spans that are dropped or never match a route. After + // c.Next, applyTelemetrySpanAttributes renames the span to + // "{method} {route template}" once the route is known. + spanName := method traceCtx := c.Context() // Compatibility note: trace extraction currently trusts the internal-service @@ -220,8 +240,13 @@ func (tm *TelemetryMiddleware) WithTelemetry(tl *tracing.Telemetry, excludedRout traceCtx = tracing.ExtractHTTPContext(traceCtx, c) } - ctx, span := tracer.Start(traceCtx, routePathWithMethod, trace.WithSpanKind(trace.SpanKindServer)) + ctx, span := tracer.Start(traceCtx, spanName, trace.WithSpanKind(trace.SpanKindServer)) endState := newSpanEndState(span) + // WithTelemetry owns this span's lifecycle: it is the sole ender (via the + // defer below, which fires on return AFTER applyTelemetrySpanAttributes + // has renamed and finalized the span). EndTracingSpans, if also + // registered, detects the owned flag and does NOT end it. + endState.owned = true defer endState.End() @@ -294,6 +319,17 @@ func applyTelemetrySpanAttributes( } if routePath, present := routeAttribute(c, statusCode); present { spanAttrs = append(spanAttrs, attribute.String("http.route", routePath)) + + // Rename the span from its method-only creation name to the OTel + // convention "{method} {http.route}" now that routing has resolved the + // low-cardinality, PII-free route template. Guarded by IsRecording so + // the intent ("skip if already ended/not recording") is explicit and + // testable rather than relying on the post-End() no-op. When no route + // matched (routeAttribute present=false, e.g. catch-all 404) the + // method-only name is left intact and http.route is omitted. + if span.IsRecording() { + span.SetName(req.method + " " + routePath) + } } if req.methodReplaced { @@ -410,7 +446,18 @@ func (tm *TelemetryMiddleware) EndTracingSpans(c fiber.Ctx) error { } if state := spanEndStateFromContext(endCtx); state != nil { + // A span owned by WithTelemetry is finalized and ended by WithTelemetry + // itself (after it applies the route template name and post-c.Next + // attributes). Ending it here would be a no-op at best and, due to + // Fiber's LIFO unwinding, could race ahead of that finalization and + // discard the rename/attributes. Return early and let the owning + // middleware end it. + if state.owned { + return err + } + state.End() + return err } @@ -475,6 +522,12 @@ func (tm *TelemetryMiddleware) WithTelemetryInterceptor(tl *tracing.Telemetry) g ctx, span := tracer.Start(traceCtx, methodName, trace.WithSpanKind(trace.SpanKindServer)) endState := newSpanEndState(span) + // Intentionally NOT marked owned (unlike WithTelemetry). The gRPC span is + // named from info.FullMethod (already low-cardinality, no PII) and has no + // post-handler rename to protect, so EndTracingSpansInterceptor may end it; + // once guards the resulting double-end. If a post-return rename is ever + // added to gRPC, set owned=true here to reinstate the ordering guarantee. + // See TRD ADR-001 (gRPC deferred as out of scope). defer endState.End() ctx = observability.ContextWithTracer(ctx, tracer) diff --git a/middleware/telemetry_span_lifecycle_test.go b/middleware/telemetry_span_lifecycle_test.go new file mode 100644 index 0000000..2c3782c --- /dev/null +++ b/middleware/telemetry_span_lifecycle_test.go @@ -0,0 +1,268 @@ +//go:build unit + +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/LerianStudio/lib-observability/v2/tracing" + "github.com/gofiber/fiber/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + oteltrace "go.opentelemetry.io/otel/trace" +) + +// telemetryTestApp wires a fresh Fiber app + TelemetryMiddleware against a +// span recorder, applying the caller-provided route/middleware registration. +// It returns the app (to drive requests) and the span recorder (to assert on +// ended spans). +func telemetryTestApp( + t *testing.T, + register func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry), +) (*fiber.App, *tracetest.SpanRecorder) { + t.Helper() + + tp, spanRecorder := setupTestTracer(t) + t.Cleanup(func() { _ = tp.Shutdown(context.Background()) }) + + oldTP := otel.GetTracerProvider() + otel.SetTracerProvider(tp) + t.Cleanup(func() { otel.SetTracerProvider(oldTP) }) + + tel := &tracing.Telemetry{ + TelemetryConfig: tracing.TelemetryConfig{ + LibraryName: "test-library", + EnableTelemetry: true, + }, + TracerProvider: tp, + } + + mid := NewTelemetryMiddleware(tel) + + app := fiber.New() + register(app, mid, tel) + + return app, spanRecorder +} + +// findSpan returns the ended span with the exact name, plus a found flag. +func findSpan(spans []sdktrace.ReadOnlySpan, name string) (sdktrace.ReadOnlySpan, bool) { + for _, s := range spans { + if s.Name() == name { + return s, true + } + } + + return nil, false +} + +// spanAttrString returns the string value of the named attribute on the span, +// and whether it was present. +func spanAttrString(s sdktrace.ReadOnlySpan, key string) (string, bool) { + for _, kv := range s.Attributes() { + if string(kv.Key) == key { + return kv.Value.AsString(), true + } + } + + return "", false +} + +// spanNames collects ended span names for diagnostic assertion messages. +func spanNames(spans []sdktrace.ReadOnlySpan) []string { + names := make([]string, 0, len(spans)) + for _, s := range spans { + names = append(names, s.Name()) + } + + return names +} + +// assertRootSpanEndedOnce asserts that exactly one span carrying the expected +// template-based name was ended exactly once and that http.route is present. +// This is the shared lifecycle+naming contract across all middleware orderings. +func assertRootSpanEndedOnce( + t *testing.T, + spanRecorder *tracetest.SpanRecorder, + expectedName, expectedRoute string, +) { + t.Helper() + + require.Eventually(t, func() bool { + return len(spanRecorder.Ended()) >= 1 + }, time.Second, 5*time.Millisecond, "expected the root span to be ended") + + spans := spanRecorder.Ended() + + matches := 0 + for _, s := range spans { + if s.Name() == expectedName { + matches++ + } + } + assert.Equal(t, 1, matches, + "expected exactly one span named %q ended exactly once; got %v", + expectedName, spanNames(spans)) + + s, ok := findSpan(spans, expectedName) + require.True(t, ok, "span %q not found among %v", expectedName, spanNames(spans)) + + route, present := spanAttrString(s, "http.route") + assert.True(t, present, "http.route must be present on the finalized span") + assert.Equal(t, expectedRoute, route) +} + +// TestSpanLifecycle_MiddlewareOrderings proves the fix is robust to the order +// in which the consumer registers WithTelemetry / EndTracingSpans: in every +// ordering the span is named by the route template, carries http.route, and is +// ended exactly once (no double-end, no lost finalization). Before the fix, +// EndTracingSpans unwinding first (Fiber LIFO) ended the span before +// applyTelemetrySpanAttributes ran, silently dropping the rename and http.route. +func TestSpanLifecycle_MiddlewareOrderings(t *testing.T) { + const ( + route = "/api/users/:id" + concretePath = "/api/users/42" // numeric ID must never reach the name + wantName = "GET /api/users/:id" + ) + + handler := func(c fiber.Ctx) error { return c.SendStatus(http.StatusOK) } + + t.Run("WithTelemetry first, EndTracingSpans last (standard order)", func(t *testing.T) { + app, rec := telemetryTestApp(t, func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry) { + app.Use(mid.WithTelemetry(tel)) + // EndTracingSpans as the last (innermost) handler unwinds FIRST + // under LIFO — the exact hazard the owned flag neutralizes. + app.Get(route, mid.EndTracingSpans, handler) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, concretePath, nil)) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + assertRootSpanEndedOnce(t, rec, wantName, route) + }) + + t.Run("EndTracingSpans registered before the route (plugin order)", func(t *testing.T) { + app, rec := telemetryTestApp(t, func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry) { + app.Use(mid.WithTelemetry(tel)) + app.Use(mid.EndTracingSpans) + app.Get(route, handler) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, concretePath, nil)) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + assertRootSpanEndedOnce(t, rec, wantName, route) + }) + + t.Run("no EndTracingSpans registered at all", func(t *testing.T) { + app, rec := telemetryTestApp(t, func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry) { + app.Use(mid.WithTelemetry(tel)) + app.Get(route, handler) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, concretePath, nil)) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + // WithTelemetry is self-sufficient: it ends its own span even with no + // EndTracingSpans in the chain. + assertRootSpanEndedOnce(t, rec, wantName, route) + }) +} + +// TestWithTelemetry_SpanNameUsesRouteTemplate is the primary anti-cardinality / +// anti-PII contract: two requests to the same :param route with different +// concrete identifiers MUST share one span name (the template), and the +// concrete identifiers MUST NOT appear anywhere in the span name. +func TestWithTelemetry_SpanNameUsesRouteTemplate(t *testing.T) { + const route = "/v1/dict/statistics/keys/:key" + + app, rec := telemetryTestApp(t, func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry) { + app.Use(mid.WithTelemetry(tel)) + app.Get(route, func(c fiber.Ctx) error { return c.SendStatus(http.StatusOK) }) + }) + + // Two distinct concrete keys, including a numeric one, that must be masked + // by the template. + for _, key := range []string{"12345678901", "a1b2c3d4-e5f6-7890-abcd-ef1234567890"} { + resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/v1/dict/statistics/keys/"+key, nil)) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusOK, resp.StatusCode) + } + + require.Eventually(t, func() bool { + return len(rec.Ended()) == 2 + }, time.Second, 5*time.Millisecond) + + spans := rec.Ended() + require.Len(t, spans, 2) + + for _, s := range spans { + assert.Equal(t, "GET "+route, s.Name(), + "both requests must collapse to the single template name") + assert.NotContains(t, s.Name(), "12345678901", + "numeric identifier must never appear in the span name") + assert.NotContains(t, s.Name(), "a1b2c3d4", + "UUID identifier must never appear in the span name") + + gotRoute, present := spanAttrString(s, "http.route") + assert.True(t, present, "http.route must remain available as an attribute") + assert.Equal(t, route, gotRoute) + } +} + +// TestWithTelemetry_CreationSpanNameIsMethodOnly asserts the PII-free-at-creation +// guarantee (the name the sampler sees): the span is CREATED with a method-only +// name, so a concrete numeric identifier in the path is absent from the name +// even before routing renames it. We observe the creation name via the active +// span inside the handler, which runs after Start() but before the post-c.Next +// rename in applyTelemetrySpanAttributes. +func TestWithTelemetry_CreationSpanNameIsMethodOnly(t *testing.T) { + var creationName string + + app, _ := telemetryTestApp(t, func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry) { + app.Use(mid.WithTelemetry(tel)) + app.Get("/accounts/:id", func(c fiber.Ctx) error { + // The span exists (created by WithTelemetry) but has not yet been + // renamed — the rename happens after this handler returns. A + // ReadWriteSpan exposes its current Name(). + if rw, ok := oteltraceSpanFromCtx(c.Context()); ok { + creationName = rw.Name() + } + + return c.SendStatus(http.StatusOK) + }) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/accounts/987654321", nil)) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + assert.Equal(t, "GET", creationName, + "creation-time span name must be method-only (PII-free for the sampler)") + assert.NotContains(t, creationName, "987654321", + "concrete numeric identifier must never be present at creation time") +} + +// oteltraceSpanFromCtx returns the active span from the context as an SDK +// ReadWriteSpan so tests can read its current (pre-rename) name. +func oteltraceSpanFromCtx(ctx context.Context) (sdktrace.ReadWriteSpan, bool) { + s := oteltrace.SpanFromContext(ctx) + rw, ok := s.(sdktrace.ReadWriteSpan) + + return rw, ok +} diff --git a/middleware/telemetry_test.go b/middleware/telemetry_test.go index 2987f8c..9c68da7 100644 --- a/middleware/telemetry_test.go +++ b/middleware/telemetry_test.go @@ -7,7 +7,6 @@ import ( "errors" "net/http" "net/http/httptest" - "strings" "sync" "testing" "time" @@ -172,19 +171,24 @@ func TestWithTelemetry(t *testing.T) { if tt.expectSpan && !tt.nilTelemetry && !tt.swaggerPath { require.GreaterOrEqual(t, len(spans), 1, "Expected at least one span to be created") - expectedPath := tt.path - if strings.Contains(tt.path, "123e4567-e89b-12d3-a456-426614174000") { - expectedPath = replaceUUIDWithPlaceholder(tt.path) - } + // After the fix the span is named "{method} {route template}". + // These tests register the route with app.All(tt.path, ...), so + // the matched route template IS tt.path verbatim (no :param + // placeholder). The span name therefore equals method+" "+tt.path. + // Note: the concrete path here doubles as the route template only + // because the route is registered literally; a real :param route + // would yield the template, never the concrete value (asserted in + // TestWithTelemetry_SpanNameUsesRouteTemplate). + expectedName := tt.method + " " + tt.path spanFound := false for _, span := range spans { - if span.Name() == tt.method+" "+expectedPath { + if span.Name() == expectedName { spanFound = true break } } - assert.True(t, spanFound, "Expected span with name %s not found", tt.method+" "+expectedPath) + assert.True(t, spanFound, "Expected span with name %s not found", expectedName) } else if tt.swaggerPath || tt.nilTelemetry { assert.Empty(t, spans, "Expected no spans for swagger path or nil telemetry") } @@ -281,7 +285,10 @@ func TestWithTelemetryExcludedRoutes(t *testing.T) { if tt.expectSpan { require.GreaterOrEqual(t, len(spans), 1, "Expected at least one span to be created") - expectedSpanName := tt.method + " " + replaceUUIDWithPlaceholder(tt.path) + // Route registered literally via app.All(tt.path, ...), so the + // matched template equals tt.path and the span is named + // "{method} {template}". + expectedSpanName := tt.method + " " + tt.path spanFound := false for _, span := range spans { if span.Name() == expectedSpanName { From 325d218856109562d5c97c7db166c5e01aaa76de Mon Sep 17 00:00:00 2001 From: Fellipe Benoni Date: Mon, 20 Jul 2026 17:57:08 -0300 Subject: [PATCH 2/3] test(middleware): address CodeRabbit review + extend gRPC span ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) X-Lerian-Ref: 0x1 --- middleware/helpers_test.go | 9 ++++--- middleware/telemetry.go | 29 ++++++++++++++------- middleware/telemetry_span_lifecycle_test.go | 19 ++++++++++++++ 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/middleware/helpers_test.go b/middleware/helpers_test.go index 21a0750..ed88ca8 100644 --- a/middleware/helpers_test.go +++ b/middleware/helpers_test.go @@ -9,6 +9,7 @@ import ( "github.com/gofiber/fiber/v3" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIsRouteExcludedFromList(t *testing.T) { @@ -57,7 +58,7 @@ func TestIsRouteExcludedFromList(t *testing.T) { req := httptest.NewRequest(http.MethodGet, tc.path, nil) resp, err := app.Test(req) - assert.NoError(t, err) + require.NoError(t, err) defer func() { _ = resp.Body.Close() }() assert.Equal(t, tc.want, got, "isRouteExcludedFromList(path=%q, excluded=%v)", tc.path, tc.excluded) @@ -88,7 +89,7 @@ func TestRouteAttribute_MatchedRoute(t *testing.T) { }) resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/api/users/42", nil)) - assert.NoError(t, err) + require.NoError(t, err) defer func() { _ = resp.Body.Close() }() assert.True(t, gotPresent, "matched route must be present") @@ -115,7 +116,7 @@ func TestRouteAttribute_UnmatchedCatchAll404(t *testing.T) { }) resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/definitely/not/registered", nil)) - assert.NoError(t, err) + require.NoError(t, err) defer func() { _ = resp.Body.Close() }() assert.Equal(t, http.StatusNotFound, resp.StatusCode) @@ -141,7 +142,7 @@ func TestRouteAttribute_RegisteredRoot(t *testing.T) { }) resp, err := app.Test(httptest.NewRequest(http.MethodGet, "/", nil)) - assert.NoError(t, err) + require.NoError(t, err) defer func() { _ = resp.Body.Close() }() assert.True(t, gotPresent, "registered root handler must retain the route") diff --git a/middleware/telemetry.go b/middleware/telemetry.go index 327b6b8..4604600 100644 --- a/middleware/telemetry.go +++ b/middleware/telemetry.go @@ -78,10 +78,11 @@ type spanEndStateKey struct{} type spanEndState struct { span trace.Span // once guarantees End() is idempotent. It is retained (not superseded by - // owned) because it still protects the paths where owned does not apply: - // the gRPC interceptor pair (which ends via both defer and - // EndTracingSpansInterceptor) and the foreign/handler-created span on the - // HTTP fallback path. owned resolves ordering; once resolves double-end. + // owned) because it still protects the foreign/handler-created span on the + // HTTP and gRPC fallback paths (where owned==false and the span may be + // ended both by a defer and by the End middleware). owned resolves ordering + // (which middleware ends the span, and that it happens after finalization); + // once resolves double-end. once sync.Once // owned marks the span as exclusively finalized/ended by the middleware // that created it (WithTelemetry). When set, EndTracingSpans must NOT end @@ -521,13 +522,14 @@ func (tm *TelemetryMiddleware) WithTelemetryInterceptor(tl *tracing.Telemetry) g ctx, span := tracer.Start(traceCtx, methodName, trace.WithSpanKind(trace.SpanKindServer)) endState := newSpanEndState(span) + // WithTelemetryInterceptor owns this span's lifecycle: it applies + // rpc.method / rpc.grpc.status_code / handler error status AFTER the + // handler returns (below), then ends the span via the defer. Marking it + // owned makes EndTracingSpansInterceptor skip it, so those post-handler + // attributes can't be dropped by a chain where the end interceptor + // unwinds first — mirroring the HTTP WithTelemetry/EndTracingSpans pair. + endState.owned = true - // Intentionally NOT marked owned (unlike WithTelemetry). The gRPC span is - // named from info.FullMethod (already low-cardinality, no PII) and has no - // post-handler rename to protect, so EndTracingSpansInterceptor may end it; - // once guards the resulting double-end. If a post-return rename is ever - // added to gRPC, set owned=true here to reinstate the ordering guarantee. - // See TRD ADR-001 (gRPC deferred as out of scope). defer endState.End() ctx = observability.ContextWithTracer(ctx, tracer) @@ -565,6 +567,13 @@ func (tm *TelemetryMiddleware) EndTracingSpansInterceptor() grpc.UnaryServerInte ) (any, error) { resp, err := handler(ctx, req) if state := spanEndStateFromContext(ctx); state != nil { + // A span owned by WithTelemetryInterceptor is finalized and ended by + // that interceptor after it records post-handler attributes/status. + // Skip it here (same reasoning as the HTTP EndTracingSpans). + if state.owned { + return resp, err + } + state.End() return resp, err } diff --git a/middleware/telemetry_span_lifecycle_test.go b/middleware/telemetry_span_lifecycle_test.go index 2c3782c..c4ba8b0 100644 --- a/middleware/telemetry_span_lifecycle_test.go +++ b/middleware/telemetry_span_lifecycle_test.go @@ -180,6 +180,25 @@ func TestSpanLifecycle_MiddlewareOrderings(t *testing.T) { // EndTracingSpans in the chain. assertRootSpanEndedOnce(t, rec, wantName, route) }) + + t.Run("EndTracingSpans registered before WithTelemetry (inverted order)", func(t *testing.T) { + app, rec := telemetryTestApp(t, func(app *fiber.App, mid *TelemetryMiddleware, tel *tracing.Telemetry) { + // Pathological order: EndTracingSpans is the OUTERMOST middleware, so + // it unwinds LAST — after WithTelemetry has finalized and ended its + // own owned span. EndTracingSpans must find the owned state and skip + // it (no double-end, no lost finalization). + app.Use(mid.EndTracingSpans) + app.Use(mid.WithTelemetry(tel)) + app.Get(route, handler) + }) + + resp, err := app.Test(httptest.NewRequest(http.MethodGet, concretePath, nil)) + require.NoError(t, err) + defer func() { require.NoError(t, resp.Body.Close()) }() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + assertRootSpanEndedOnce(t, rec, wantName, route) + }) } // TestWithTelemetry_SpanNameUsesRouteTemplate is the primary anti-cardinality / From 4773b1e34abe5aefc110779705010da0858ac961 Mon Sep 17 00:00:00 2001 From: Fellipe Benoni Date: Mon, 20 Jul 2026 18:15:27 -0300 Subject: [PATCH 3/3] style(middleware): satisfy wsl_v5 whitespace before gRPC interceptor 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) X-Lerian-Ref: 0x1 --- middleware/telemetry.go | 1 + 1 file changed, 1 insertion(+) diff --git a/middleware/telemetry.go b/middleware/telemetry.go index 4604600..b239433 100644 --- a/middleware/telemetry.go +++ b/middleware/telemetry.go @@ -575,6 +575,7 @@ func (tm *TelemetryMiddleware) EndTracingSpansInterceptor() grpc.UnaryServerInte } state.End() + return resp, err }