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]