[REG-15] W0.10 - Collector deployment, sampling policy, and cost controls - #173
Merged
Conversation
…ubbing
A place for traces to actually go, and a sampling policy that keeps the
bill and the storage bounded.
- New deploy/otel-collector/otel-collector-config.yaml: the collector
pipeline - redaction/pii (defense in depth allow-list + email-pattern
masking) -> tail_sampling (keep 100% of errors, incident-creating,
and paging traces; probabilistic 2% of the rest) -> batch -> export
to Tempo.
- New deploy/otel-collector/tempo.yaml: single-binary Tempo config,
7-day retention default (compactor.compaction.block_retention).
Pinned to grafana/tempo:2.6.1, not :latest - verified directly that
:latest resolves to Tempo v3.0.0, whose config schema has moved
since the docs/examples this file was written against.
- docker-compose.dev.yml: otel-collector + tempo added under a new
"observability" compose profile (off by default - verified via
`docker compose config --services` that neither appears without
--profile observability). The app's OTEL_* env vars default to empty
either way, matching InitTracer's existing "off unless explicitly
configured" design.
- Helm chart: new tracing.* values block and templates/otel-collector.yaml
(ConfigMap + Deployment + Service, gated by tracing.collector.enabled,
off by default - verified via `helm template` that none of it renders
otherwise). Deliberately a plain set of resources in this chart, not
the full upstream open-telemetry-collector chart - see the values.yaml
comment for when to reach for that one instead.
- OTEL_TRACES_SAMPLER / OTEL_TRACES_SAMPLER_ARG: no code change needed -
reading go.opentelemetry.io/otel/sdk/trace's own source confirmed
NewTracerProvider already applies env-based sampler config before any
explicit option, and InitTracer never passes WithSampler. Added three
tests proving this explicitly rather than taking the SDK's internals
on faith, and fixed a real latent bug the new tests surfaced:
clearOTelEnv's cleanup only restored vars that existed before a test,
never unset ones a test itself set - meaning OTEL_TRACES_SAMPLER=
traceidratio leaked into every subsequent test in the binary and
silently broke unrelated worker_test.go assertions.
- New incident.id / incident.created span attributes
(internal/observability/incident_span.go), wired into the incident
HTTP handlers (GetIncident/CreateIncident/UpdateIncident) and the
alert-triggered creation path (CreateIncidentFromAlert(WithGrouping)).
Without this, the runbook's own "find a trace from an incident ID"
promise would have been false for most incident traces - only
REG-12's LLM spans carried it before this.
- New notification.paged span attribute on SendEscalationDM (its own
root span, same shape as StartWorkerTick - it has no ctx to thread
one through, per REG-13's documented reasoning) - the signal the
tail-sampling policy needs to keep 100% of traces that send a page.
- docs/OBSERVABILITY.md: Tempo vs SigNoz recommendation (Tempo, for the
same "smallest footprint" reasoning behind this project's other
infra choices), the sampling policy and why tail-based sampling can
only happen at a collector, retention rationale tied to W13's DSAR
work, and the runbook for finding a trace from an incident ID or a
customer complaint.
Live-verified against a real otel-collector + Tempo: fired traces
through the actual pipeline and found two real bugs in the process,
both fixed here -
1. Tempo :latest (v3.0.0) rejected the config's compactor section
entirely - a schema change from the v2.x docs this was written
against. Pinned to 2.6.1, config verified with -config.verify=true.
2. The tail-sampling policies for incident.created/notification.paged
used type: string_attribute against real OTel bool attributes -
which never matches a bool-typed value (confirmed live: traces
with these attributes were dropped by "sample-the-rest" instead of
kept). Fixed by switching to type: boolean_attribute.
After both fixes: an error trace, an incident.created trace, a
notification.paged trace, an attribute outside the redaction allow-list
(confirmed stripped entirely), and an email address inside an allowed
key (confirmed masked to asterisks) all behaved exactly as configured;
~2.5% of 2000 "boring" control traces survived the probabilistic
policy, consistent with the configured 2%.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes REG-15
Summary
A place for traces to actually go, and a sampling policy that keeps the bill and the storage bounded.
What changed
deploy/otel-collector/otel-collector-config.yaml(new): the collector pipeline —redaction/pii(defense-in-depth allow-list + email-pattern masking) →tail_sampling(keep 100% of errors, incident-creating, and paging traces; probabilistic 2% of the rest) →batch→ export to Tempo.deploy/otel-collector/tempo.yaml(new): single-binary Tempo config, 7-day retention default. Pinned tografana/tempo:2.6.1, not:latest— verified directly that:latestresolves to Tempo v3.0.0, whose config schema has moved since the docs/examples this was written against (see below).docker-compose.dev.yml:otel-collector+tempoadded under a new opt-inobservabilityprofile — verified viadocker compose config --servicesthat neither appears without--profile observability.tracing.*values block +templates/otel-collector.yaml(ConfigMap + Deployment + Service, gated bytracing.collector.enabled, off by default — verified viahelm template). Deliberately plain resources in this chart rather than the full upstreamopen-telemetry-collectorchart; the values.yaml comment explains when to reach for that one instead.OTEL_TRACES_SAMPLER/OTEL_TRACES_SAMPLER_ARG: no code change needed — confirmed via the SDK's own source thatNewTracerProvideralready applies env-based sampler config before any explicit option, andInitTracernever overrides it. Added 3 tests proving this rather than trusting the SDK's internals on faith.incident.id/incident.createdspan attributes (newinternal/observability/incident_span.go), wired into the incident HTTP handlers and the alert-triggered creation path. Without this the runbook's own "find a trace from an incident ID" claim would have been false for most incident traces.notification.pagedspan attribute onSendEscalationDM(its own root span — no ctx to thread one through, per REG-13's documented reasoning) — the signal the tail-sampling policy needs to keep 100% of paging traces.docs/OBSERVABILITY.md(new): Tempo vs SigNoz recommendation, the sampling policy and why tail-based sampling can only happen at a collector, retention rationale tied to W13's DSAR work, and the runbook for finding a trace from an incident ID or a customer complaint.Bugs found and fixed via a real test-isolation bug + two live-verification failures
clearOTelEnv's cleanup only restored env vars that existed before a test — it never unset ones the test itself set.OTEL_TRACES_SAMPLER=traceidratiofrom one test leaked into every later test in the binary, silently breakingworker_test.go's unrelated assertions. Fixed the helper.:latest(v3.0.0) rejected the config'scompactorsection entirely — a schema change from the docs this was written against. Confirmed by running-config.verify=truedirectly against candidate versions; pinned to2.6.1.incident.created/notification.pagedusedtype: string_attributeagainst real OTel bool attributes — which never matches a bool-typed value (string_attributeonly reads the OTLPstring_valuefield). Confirmed live: traces carrying these attributes were being dropped bysample-the-restinstead of kept at 100%. Fixed by switching totype: boolean_attribute.Acceptance criteria
docker-compose.dev.yml(opt-in profile) and the Helm chart (optional, gated resources)OTEL_TRACES_SAMPLER/OTEL_TRACES_SAMPLER_ARGconfigurable — already worked, now tested and documentedVerification
go build ./...,go vet ./...,gofmt -lclean on all touched files-shuffle=onon all touched packages (multiple runs, confirming the env-leak fix holds)golangci-lint run ./...: 0 issuesdocker compose configvalidates the compose file and confirms profile gatinghelm lint/helm templatevalidate the chart, confirm the collector resources are absent by default and correct when enabledincident.createdtrace, anotification.pagedtrace, a trace with an attribute outside the redaction allow-list, and a trace with an email address inside an allowed key — all five behaved exactly as configured (kept at 100%, allow-list attribute stripped, email masked to asterisks) — plus 2000 "boring" control traces, of which ~2.5% survived the probabilistic policy, consistent with the configured 2%