From e4c7a69750c9e69214742c5cd1fa815baafb693b Mon Sep 17 00:00:00 2001 From: Joana Carvalho Date: Sat, 3 Oct 2026 14:41:14 +0000 Subject: [PATCH] fix(collector): apply the cardinality guardrail to metric data points, not just spans (#172) The agent config's transform processor dropped user.id / session.id / request.id / order.id from span attributes only. semconv-discipline ranks a metric dimension as the worst position for an identifier (one retained series per value, untouched by sampling, persisting in the backend after the code is fixed), so the guardrail covered the least damaging case and missed the most damaging one. The repo-specific keys from derived.highCardinalityAttributes (#37) had the same gap: they were only ever emitted as trace_statements. - collector-topology template and the three agent goldens: a metric_statements list (context: datapoint) with the same drop-list, and transform in the metrics pipeline between memory_limiter and batch. The generated file says that dropping a dimension merges the series that differed only by it. - /otel-collector adds each derived.highCardinalityAttributes key to both lists. - semconv-discipline said the Collector guardrail "operates on spans only" and "cannot reach a metric dimension", and every subagent that reads the skill was told the case must be fixed in code. Corrected: it covers spans and metric data points for the keys on its list, as a backstop that does not replace fixing the instrumentation. Log records are deliberately NOT guarded, which departs from the issue's proposal. semconv-discipline ranks a log-record attribute as the prescribed home for a per-request identifier, a log record is not a time series, and deleting user.id there would remove debugging data with no cardinality benefit. The template says so and when a log_statements list would be justified. Verified at runtime against otelcol-contrib 0.161.0 using the golden's own processor block: a metric data point and a span carrying user.id lose it and keep http.route, while a log record keeps both. The goldens still validate (tests/collector-validate.sh). collector-cardinality-contract.test.sh gains 17 checks: per golden, the two drop-lists are identical, the datapoint context is used, and traces and metrics run memory_limiter, transform, batch while logs stay unguarded. Bumps to 0.8.20. --- .claude-plugin/plugin.json | 2 +- ROADMAP.md | 2 +- commands/otel-collector.md | 3 +- skills/collector-topology/SKILL.md | 33 ++++++++++- skills/semconv-discipline/SKILL.md | 13 +++-- .../collector-cardinality-contract.test.sh | 58 +++++++++++++++++++ .../collector/otelcol-agent-headers.yaml.snap | 12 +++- .../collector/otelcol-agent-public.yaml.snap | 12 +++- .../collector/otelcol-agent.yaml.snap | 12 +++- 9 files changed, 133 insertions(+), 14 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 226a113..af54de5 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "otel-as-code", - "version": "0.8.19", + "version": "0.8.20", "description": "OpenTelemetry instrumentation and observability-as-code for any stack", "author": { "name": "radra.gwent", diff --git a/ROADMAP.md b/ROADMAP.md index af4ed1d..3b24622 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -8,7 +8,7 @@ > of the same shape, and every concrete line item that isn't done yet has a linked tracking issue — > comment there rather than only in this file if you want to weigh in. -Current release: **0.8.19**. Node.js, Python, Java, .NET, Go, Ruby, and Rust (axum) instrumentation, Terraform +Current release: **0.8.20**. Node.js, Python, Java, .NET, Go, Ruby, and Rust (axum) instrumentation, Terraform for Grafana, Datadog, New Relic, and Dash0, semconv pinned at 1.44.0. Since 0.2.0 the command set has grown past generation: `/otel-uninstrument` rolls instrumentation back behind an ownership marker, `/otel-remediate` proposes a deployment-config diff for services that have no application diff --git a/commands/otel-collector.md b/commands/otel-collector.md index af63085..8ff2dae 100644 --- a/commands/otel-collector.md +++ b/commands/otel-collector.md @@ -124,7 +124,8 @@ For `agent` mode: `collector-topology` template, add one `delete_key` for each attribute name in `context.services[i].derived.highCardinalityAttributes`, unioned across all services in the cache and de-duplicated. That field is a typed list of attribute keys the scanner/auditor found - to be high-cardinality — read it directly. Do NOT filter `conformanceIssues` on a + to be high-cardinality — read it directly, and add each one to BOTH the `trace_statements` and + the `metric_statements` lists (they must stay identical; #172). Do NOT filter `conformanceIssues` on a `severity: "cardinality"` value: no producer emits that severity (they emit info/warning/error), and `conformanceIssues` lives under `derived`, not `existingOtel` — so the old filter matched nothing and only the template's four generic identifiers were ever protected (#37). diff --git a/skills/collector-topology/SKILL.md b/skills/collector-topology/SKILL.md index 51803d3..a9a43e3 100644 --- a/skills/collector-topology/SKILL.md +++ b/skills/collector-topology/SKILL.md @@ -197,8 +197,8 @@ auth explanation above) rather than leaving it as an unexplained value only the ## Cardinality Guardrails (add to agent config) -Always include a `transform` processor to drop high-cardinality span attributes -that are common mistakes: +Always include a `transform` processor to drop high-cardinality identifiers that are common +mistakes. It runs on **spans and on metric data points**, with the same drop-list in both: ```yaml processors: @@ -214,10 +214,37 @@ processors: - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) + metric_statements: + - context: datapoint + statements: + # The SAME list. A metric dimension is the worst position for an identifier (one + # retained series per value; sampling cannot help), so this is the more valuable half. + # Dropping the key merges the series that differed only by it, which is the intended + # outcome: say so in the generated file so nobody is surprised. + - delete_key(attributes, "user.id") where IsString(attributes["user.id"]) + - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) + - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) + - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) + +service: + pipelines: + traces: { processors: [memory_limiter, transform, batch] } + metrics: { processors: [memory_limiter, transform, batch] } + logs: { processors: [memory_limiter, batch] } ``` When generating config, check the context JSON for any attributes marked high-cardinality -by the `brownfield-auditor` and add matching `delete_key` statements. +by the `brownfield-auditor` and add matching `delete_key` statements **to both lists**, so they +cannot drift apart (#172; before this, repo-specific keys were only ever added for spans). + +**Logs are deliberately not guarded.** A log record is not a time series, and +`semconv-discipline` ranks a log-record attribute as the prescribed home for a per-request +identifier, so deleting `user.id` there would remove debugging data with no cardinality benefit. +Add a `log_statements` list only if a specific backend turns log attributes into indexed labels, +and then drop only the keys it indexes. + +This guardrail is a backstop, not the fix: the source of truth is still the instrumentation +(`semconv-lint` catches the identifier in code). It only covers the keys on its list. ## Required exporter configuration diff --git a/skills/semconv-discipline/SKILL.md b/skills/semconv-discipline/SKILL.md index eb1c9fe..0823fec 100644 --- a/skills/semconv-discipline/SKILL.md +++ b/skills/semconv-discipline/SKILL.md @@ -174,8 +174,12 @@ position is the one the old one-line rule never mentioned. Tier findings by posi So the fix differs by position: for a **metric dimension** it is "remove the tag" (irreversible damage otherwise); for a **span attribute** it is "consider moving to a span event or log". Note -the Collector `transform` `delete_key` guardrail operates on **spans only** — it cannot clean up -an identifier that already reached a metric dimension, so that case must be caught in code. +the Collector `transform` `delete_key` guardrail covers **spans and metric data points** with the +same drop-list (#172), so a listed identifier is stripped from a metric dimension before it reaches +the backend, at the cost of merging the series that differed only by it. It is a backstop for the +keys on its list, not a substitute for fixing the code: an identifier it does not list still +creates a series per value, and series already created in the backend persist. Log records are +deliberately not guarded, since they are the prescribed home for a per-request identifier. The one-line form, kept because it is still true for the common case: high-cardinality identifiers do not belong as span attributes — use events or logs instead — but a metric @@ -185,9 +189,8 @@ dimension is worse, not exempt. hook (at the source level across all six supported languages, matching both the dotted `user.id` form and the common camelCase/snake_case spellings — `userId` / `user_id` — as a span attribute, AND as a metric dimension, which the hook treats as `error` tier per the ranking above, not -`warning`) and by the Collector `transform` drop-list in `collector-topology` (at the -span-attribute level only — see the cardinality-position table above for why the Collector -guardrail cannot reach a metric dimension). `semconv-lint` reads this exact list at runtime +`warning`) and by the Collector `transform` drop-list in `collector-topology` (on span attributes +and metric data points; not log records — see the cardinality-position table above). `semconv-lint` reads this exact list at runtime (`hooks/semconv-lint.sh` greps the line below) rather than hardcoding a second copy — keep it in sync with the Collector drop-list when adding an identifier, and do not reformat this line (the hook's extraction depends on its shape: backtick-wrapped dotted identifiers, comma-separated, diff --git a/tests/hooks/collector-cardinality-contract.test.sh b/tests/hooks/collector-cardinality-contract.test.sh index b204e75..d3c2a11 100644 --- a/tests/hooks/collector-cardinality-contract.test.sh +++ b/tests/hooks/collector-cardinality-contract.test.sh @@ -28,6 +28,64 @@ fi # read, so check 1 already catches that regression — no brittle "severity=cardinality" grep here, # which would false-fire on the docs' own do-NOT guidance.) +# --- #172: the guardrail covers metric data points, not just spans --------------------------- +# A metric dimension is the worst position for an identifier (semconv-discipline's own ranking), +# and the template used to clean spans only. Logs are deliberately NOT guarded: a log record is +# the prescribed home for a per-request identifier. +SKILL="skills/collector-topology/SKILL.md" +SEMCONV="skills/semconv-discipline/SKILL.md" + +# delete_key keys under one statements list (trace_statements / metric_statements) of a golden. +stmt_keys() { # file list-name + awk -v L="$2:" '$1==L{f=1;next} f&&/^ [a-z_]+:/{f=0} f&&/delete_key/{match($0,/"[^"]+"/);print substr($0,RSTART,RLENGTH)}' "$1" | sort | tr '\n' ' ' +} +pipeline_procs() { # file pipeline + awk -v P=" $2:" '$0==P{f=1;next} f&&/processors:/{print;exit}' "$1" +} + +for snap in otelcol-agent otelcol-agent-public otelcol-agent-headers; do + f="tests/snapshots/collector/$snap.yaml.snap" + tk=$(stmt_keys "$f" trace_statements); mk=$(stmt_keys "$f" metric_statements) + if [ -n "$tk" ] && [ "$tk" = "$mk" ]; then + echo "PASS: $snap: metric_statements drop exactly the span drop-list ($tk)"; pass=$((pass+1)) + else + echo "FAIL: $snap: metric_statements ('$mk') differ from trace_statements ('$tk') (#172)"; fail=$((fail+1)) + fi + if grep -qF "context: datapoint" "$f"; then + echo "PASS: $snap: metric statements use the datapoint context"; pass=$((pass+1)) + else + echo "FAIL: $snap: no datapoint-context statements (#172)"; fail=$((fail+1)) + fi + for pl in traces metrics; do + if pipeline_procs "$f" $pl | grep -qF "[memory_limiter, transform, batch]"; then + echo "PASS: $snap: $pl pipeline runs memory_limiter, transform, batch"; pass=$((pass+1)) + else + echo "FAIL: $snap: $pl pipeline does not run transform between memory_limiter and batch (#172)"; fail=$((fail+1)) + fi + done + if pipeline_procs "$f" logs | grep -qF "[memory_limiter, batch]"; then + echo "PASS: $snap: logs pipeline is deliberately unguarded"; pass=$((pass+1)) + else + echo "FAIL: $snap: logs pipeline changed; log records are the prescribed home for identifiers"; fail=$((fail+1)) + fi +done + +if grep -qF "metric_statements:" "$SKILL" && grep -qF "context: datapoint" "$SKILL" && grep -qF "**Logs are deliberately not guarded.**" "$SKILL"; then + echo "PASS: collector-topology template has the metric guardrail and explains why logs are skipped"; pass=$((pass+1)) +else + echo "FAIL: collector-topology template lacks metric_statements, or the log-record rationale (#172)"; fail=$((fail+1)) +fi +if grep -qF "BOTH the \`trace_statements\` and" "$COLLECTOR"; then + echo "PASS: /otel-collector adds derived.highCardinalityAttributes to both lists"; pass=$((pass+1)) +else + echo "FAIL: /otel-collector adds repo-specific keys to spans only (#172)"; fail=$((fail+1)) +fi +if ! grep -qF "spans only" "$SEMCONV" && ! grep -qF "cannot reach a metric dimension" "$SEMCONV" && grep -qF "spans and metric data points" "$SEMCONV"; then + echo "PASS: semconv-discipline no longer says the Collector guardrail cannot reach metrics"; pass=$((pass+1)) +else + echo "FAIL: semconv-discipline still tells every subagent the Collector cannot clean a metric dimension (#172)"; fail=$((fail+1)) +fi + echo "" echo "Results: $pass passed, $fail failed" [ "$fail" -eq 0 ] diff --git a/tests/snapshots/collector/otelcol-agent-headers.yaml.snap b/tests/snapshots/collector/otelcol-agent-headers.yaml.snap index 5cc46ce..d65e843 100644 --- a/tests/snapshots/collector/otelcol-agent-headers.yaml.snap +++ b/tests/snapshots/collector/otelcol-agent-headers.yaml.snap @@ -22,6 +22,16 @@ processors: - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) + # Same drop-list on metric data points: a metric dimension is the worst place for an + # identifier (one retained series per value; sampling cannot help). Dropping it merges + # the series that differed only by that key, which is the intended outcome. + metric_statements: + - context: datapoint + statements: + - delete_key(attributes, "user.id") where IsString(attributes["user.id"]) + - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) + - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) + - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) batch: timeout: 1s send_batch_size: 1024 @@ -53,7 +63,7 @@ service: exporters: [otlp] metrics: receivers: [otlp] - processors: [memory_limiter, batch] + processors: [memory_limiter, transform, batch] exporters: [otlp] logs: receivers: [otlp] diff --git a/tests/snapshots/collector/otelcol-agent-public.yaml.snap b/tests/snapshots/collector/otelcol-agent-public.yaml.snap index c5fed85..9a9ba08 100644 --- a/tests/snapshots/collector/otelcol-agent-public.yaml.snap +++ b/tests/snapshots/collector/otelcol-agent-public.yaml.snap @@ -34,6 +34,16 @@ processors: - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) + # Same drop-list on metric data points: a metric dimension is the worst place for an + # identifier (one retained series per value; sampling cannot help). Dropping it merges + # the series that differed only by that key, which is the intended outcome. + metric_statements: + - context: datapoint + statements: + - delete_key(attributes, "user.id") where IsString(attributes["user.id"]) + - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) + - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) + - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) batch: timeout: 1s send_batch_size: 1024 @@ -75,7 +85,7 @@ service: exporters: [otlp] metrics: receivers: [otlp] - processors: [memory_limiter, batch] + processors: [memory_limiter, transform, batch] exporters: [otlp] logs: receivers: [otlp] diff --git a/tests/snapshots/collector/otelcol-agent.yaml.snap b/tests/snapshots/collector/otelcol-agent.yaml.snap index 10f6283..2fdf4fa 100644 --- a/tests/snapshots/collector/otelcol-agent.yaml.snap +++ b/tests/snapshots/collector/otelcol-agent.yaml.snap @@ -22,6 +22,16 @@ processors: - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) + # Same drop-list on metric data points: a metric dimension is the worst place for an + # identifier (one retained series per value; sampling cannot help). Dropping it merges + # the series that differed only by that key, which is the intended outcome. + metric_statements: + - context: datapoint + statements: + - delete_key(attributes, "user.id") where IsString(attributes["user.id"]) + - delete_key(attributes, "session.id") where IsString(attributes["session.id"]) + - delete_key(attributes, "request.id") where IsString(attributes["request.id"]) + - delete_key(attributes, "order.id") where IsString(attributes["order.id"]) batch: timeout: 1s send_batch_size: 1024 @@ -50,7 +60,7 @@ service: exporters: [otlp] metrics: receivers: [otlp] - processors: [memory_limiter, batch] + processors: [memory_limiter, transform, batch] exporters: [otlp] logs: receivers: [otlp]