fix(collector): apply the cardinality guardrail to metric data points, not just spans (#172)

.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",

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

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).

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
 

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,

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 ]

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]

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]

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]