fix(warehouse): read renamed OTel attributes under either semconv spelling - #560
Merged
Merged
Conversation
…v key
OpenTelemetry renamed the resource attribute: the registry lists
`deployment.environment.name` as stable and plain `deployment.environment`
as deprecated ("Replaced by `deployment.environment.name`"). We dual-emit
both on the write side, but every read was pinned to exactly one spelling —
traces, logs, errors, metrics and the service map read the legacy key, infra
read `.name`, and all 14 MV bodies pre-extracted `DeploymentEnv` from the
legacy key alone.
So a service instrumented with an OTel SDK new enough to have adopted the
rename materialized an EMPTY environment into every rollup: no environment
facet, no environment filter, on exactly the dashboards the rollups back.
Our own SDKs dual-emit, which is why this never showed on our telemetry.
One canonical expression now backs both sides (`DEPLOYMENT_ENV_SQL` /
`deploymentEnvExpr`, mirroring the existing `db.system.name` fragment):
coalesce(nullIf(ResourceAttributes['deployment.environment.name'], ''),
ResourceAttributes['deployment.environment'])
- write: every MV body, ClickHouse migration 0020 (DROP+CREATE for the 15
affected views, requiredForIngest false), and the local-store mirror —
schema v10 snapshot, history identity, v9 -> v10 edge.
- read: query-helpers, logs, errors, metrics, service-map(-rollup) and infra
(including its facets) go through the shared expression. Generic attribute
filters coalesce via a new RESOURCE_SEMCONV_ALIASES table, so a filter
typed as either spelling matches either stored key.
- tests: a gate that fails if any MV extracts DeploymentEnv another way, a
byte-identity check between the SQL constant and the DSL expression, and
both-spellings filter coverage. SQL baseline diff is env lines plus
fingerprints only.
Forward-only: rows already materialized keep the empty DeploymentEnv the old
bodies wrote and age out with the target TTL. Backfilling would mean
rewriting every rollup from `traces`.
The legacy emit stays — not for our rollups any more, but for pre-0020 rows
and for BYO-ClickHouse orgs still on a pre-0020 schema. Docs and the
telemetry-conventions skill now say that instead of "keep until the MVs
coalesce".
Deploy needs the manual prod ClickHouse migration and a Tinybird deploy.
The probe asserts the promoted store's marker equals the CURRENT head identity, so it fails by construction on a schema bump — which is what it is for. Move it from v9/2516215f22b41a63 to v10/c10dd07518c57e5d.
…ey too Audited every rename in the repo's own dictionary (`ATTRIBUTE_RENAMES`, 30 pairs) against the read paths. The HTTP method/status aliases, the whole `db.*` family, `http.target`/`url.path`, `http.host`/`net.peer.name` → `server.address` and `enduser.id` → `user.id` all already coalesce. One did not: `messaging.destination` → `messaging.destination.name`. `service_external_edges_hourly_mv` and its raw-`traces` branch key `TargetName` off the deprecated spelling alone, so a producer span from current instrumentation fell through to the messaging *system* — every topic and queue collapsing into a single `kafka` / `sqs` node instead of a per-destination edge. Our own `VcsSyncQueue` spans emit `messaging.destination.name`, so this was visible on Maple's own service map. Same treatment as the environment key, and folded into the same migration rather than a second one: MV body, raw branch, migration 0020 (now `0020_semconv_key_renames`), local schema v10, one Tinybird deploy for both. `deployment-env-sql.ts` becomes `semconv-renames.ts` — one module for the coalescing fragments of keys we key on, which is the shape this should have had from the start now that there are two. Renames whose keys are only ever surfaced stay in the recommendation dictionary. Also: the client SDK emitted the deprecated `browser.user_agent`; it now emits `user_agent.original`. BREAKING for any customer query or dashboard keyed on `browser.user_agent` — nothing in our own read path reads it (session replay reads a `UserAgent` column, `inspect-trace` already lists the semconv key).
Makisuo
changed the title
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
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
1 participant
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.
What
Two OpenTelemetry attribute renames were being read on the deprecated spelling alone, inside materialized views — where the loss is invisible and permanent for the rows already written.
deployment.environment→deployment.environment.nameDeploymentEnv(infra queries read only.name— the mirror image)messaging.destination→messaging.destination.nameservice_external_edges_hourly_mv+ its raw branchTargetNamefell through to the messaging system, so every topic/queue collapsed into onekafka/sqsnode instead of per-destination edgesBoth were self-demonstrating: our own SDKs and ingest gateway dual-emit the environment (which is why it never showed on our telemetry), and our own
VcsSyncQueueproducer spans emitmessaging.destination.name(which is why that one was visible on our own service map).How
One module —
packages/domain/src/tinybird/semconv-renames.ts— holds the coalescing fragment for every renamed key Maple actually keys on, in the shapedb-query-shape-sql.tsalready established:DROP VIEW+CREATE MATERIALIZED VIEWfor the 15 affected views,requiredForIngest: false); local-store mirror: schema v10 snapshot, history identity, v9 → v10 edge.RESOURCE_SEMCONV_ALIASEStable intraces-shared.ts, so a filter typed as either spelling matches either stored key.Audit
I checked all 30 pairs in the repo's own
ATTRIBUTE_RENAMESdictionary against the read paths, plus resource-level renames that dictionary deliberately excludes. Already correct: HTTP method/status, the wholedb.*family,http.target→url.path,http.host/net.peer.name→server.address,enduser.id→user.id. Absent entirely:telemetry.auto.version,faas.id,container.labels,otel.library.*. Not audited: metric names/units, log-record semconv, gen-ai attributes.Known and not fixed here: the query-builder and autocomplete dictionaries still advertise
http.method/http.status_code/deployment.environmentas suggested keys. Filters coalesce so they work, but we're teaching users deprecated spellings.⚠ Breaking
The client SDK emitted the deprecated
browser.user_agent; it now emitsuser_agent.original. Nothing in our read path reads the old key (session replay reads aUserAgentcolumn;inspect-tracealready lists the semconv key), but a customer query or dashboard keyed onbrowser.user_agentwill stop matching on the next SDK release.Reviewer notes
tracesfor values that were empty or mislabeled on those rows anyway.tracesbranch, and they only merge if both sides compute the same grouping key (asserted by test).Deploy
Needs the manual prod ClickHouse migration and
bun run --cwd apps/api tinybird:deploy. Both renames ride one migration and one deploy.Verification
bun typecheck— 40/40@maple/domain555 ✓ ·@maple/query-engine1227 ✓ ·@maple/effect-sdkclient 32 ✓ · CLI migration tests ✓ · targetedapps/api+apps/web✓clickhouse:schema:check,tinybird:manifest:check, local schema manifest gate — clean