fix(otel): one enduser.id, on the log record, and a real service.version - #101
suibianwanwank wants to merge 3 commits into
Conversation
…sion
Identity was on the log Resource. Resource is defined as an immutable description of
the entity PRODUCING telemetry and is bound to the provider for its lifetime, so
`enduser.id` / `instance.name` / `workspace.name` never belonged there:
- every (user, instance, workspace) triple read as a separate service instance
downstream, multiplying the cardinality of what should be one service;
- the trace path already put the same four attributes on the span, so the two
signals disagreed about where identity lives — a join across them had to know
which layer to read per signal;
- a profile switch mid-process cannot be expressed on an immutable resource, and
the CLI is growing long-lived paths (`agent`, `serve`) where that now happens.
The key is now emitted exactly once, on the log record, matching the span path. The
test asserts the count, not just the presence, and fails if the resource entry comes
back.
setup.ts hand-rolled `attrs["enduser.id"] = String(opts.userId)`, a second spelling of
a key that connection/telemetry.ts already owned — which is how one copy could sit on
the wrong layer while the other was right. Both paths go through identityAttributes()
now, and IdentityAttributes makes a fifth key a type error rather than a silent extra
attribute.
Separately, the trace resource's service.version reported InstallationVersion —
opencode's version, which is "local" for every build that is not a published opencode
release — while service.name says cz-agent. So cz-agent traces carried no usable
cz-cli version and could not be segmented by release at all. It now stamps VERSION,
with opencode's own version kept as opencode.version.
That needed build.ts: runtime assets are bundled separately from the binary and never
saw its `define` block, so version.ts fell back to 0.0.0-dev+<ts> inside the plugin —
the same drop that once broke `cz-cli --version`. CLICKZETTA_VERSION is now stamped
into runtime assets too.
Warehouse follow-up, not in this repo: the semantic view czcli.public.czcli_otel_analytics
reads resourceattributes['enduser.id'] and must move to logattributes for new rows.
ai_agent_user_daily already COALESCEs both spellings and needs no change. Rows written
before this commit keep identity on the resource.
Verified: `bun run typecheck` clean; 95 tests across the telemetry/otel/setup files pass;
reverting the resource line turns the new count assertion red; a probe build of the
plugin runtime asset shows opencode.version present and no 0.0.0-dev fallback.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "service.name": process.env.OPENCODE_SERVICE_NAME || "opencode", | ||
| "service.version": InstallationVersion, | ||
| "service.version": VERSION, | ||
| "opencode.version": InstallationVersion, |
There was a problem hiding this comment.
MEDIUM (confidence: high) — opencode.version will be the literal string "local" in every released build, so this key does not preserve what the comment says it preserves.
"opencode.version": InstallationVersion,InstallationVersion is typeof OPENCODE_VERSION === "string" ? OPENCODE_VERSION : "local" (packages/core/src/installation/version.ts:6). This module ships inside clickzetta-opencode-plugin.js, built by buildRuntimeAsset (script/build.ts:322) — and that is exactly the bundle whose missing define block this PR is fixing. The PR adds only CLICKZETTA_VERSION there (script/build.ts:54-56); OPENCODE_VERSION is still absent, so InstallationVersion falls through to "local". That is the same defect being fixed for service.version, relocated to a new key.
Note the other direction is no better: in the binary's own define block, OPENCODE_VERSION: '${Script.version}' (script/build.ts:273) is the cz-cli version, so if this module were ever bundled into the binary graph the attribute would just duplicate service.version. Neither path ever yields 1.17.11.
The actual baseline version is already in scope at build time — script/build.ts:32 does import pkg from "../../opencode/package.json", whose version is 1.17.11. So the smaller correct change is either to drop this key, or to stamp a separate define (e.g. CLICKZETTA_UPSTREAM_VERSION: pkg.version) into buildRuntimeAsset and read that instead of InstallationVersion.
No test covers this attribute — otel-config.test.ts only asserts env var names, and nothing exercises OtelPlugin's resourceAttrs.
There was a problem hiding this comment.
Fixed — key dropped in c594ec7. Confirmed your reading: InstallationVersion falls through to "local" in the plugin runtime asset, and the binary's own OPENCODE_VERSION define is fed Script.version, so neither path could ever yield 1.17.11. I chose dropping over a CLICKZETTA_UPSTREAM_VERSION define — the baseline version is a build-time fact that belongs in the release ledger, not on every span. This also corrects the first commit's stated cause: the value was "local" because the runtime asset never saw the binary's defines, not because the build was an unpublished opencode release. The resource now comes from a pure otelResourceAttributes() in otel/resource.ts with 4 tests, one of which asserts opencode.version is absent.
| const resourceAttrs: Record<string, string> = { | ||
| "service.name": process.env.OPENCODE_SERVICE_NAME || "opencode", | ||
| "service.version": InstallationVersion, | ||
| "service.version": VERSION, |
There was a problem hiding this comment.
LOW / regression note (confidence: high) — please confirm the intent: this changes service.version on three signals, not just otel_traces as the PR's table states.
"service.version": VERSION,resourceAttrs is passed to initOtelSdk, which builds one resource and hands the same object to the LoggerProvider, the BasicTracerProvider and the MeterProvider (otel/setup.ts:45-65). So every cz-agent log record emitted through handlers.ts's _logger and every metric from PeriodicExportingMetricReader also flips service.version from "local" to the cz-cli version.
That is almost certainly the behavior you want, but the PR's before/after table lists only otel_traces for the version change, and the warehouse follow-up section audits only enduser.id. Anything downstream that filters or groups otel_logs / otel_metrics rows on resourceattributes['service.version'] = 'local' (a value that has been constant for the whole life of the agent path) will stop matching new rows. Worth the same 12-object sweep you did for enduser.id.
Nothing in the suite covers the plugin's resource, so this is unverified in tests either way.
There was a problem hiding this comment.
Intent confirmed, and the blast radius is measured: the flip is meant to cover logs and metrics too — one wrong version on a shared resource is exactly why it should move everywhere at once.
Ran your suggested 14-object sweep over czcli.public. Only czcli_otel_analytics references service.version at all, and it reads it from otel_logs (logs.service_version = resourceattributes['service.version']) — the CLI path, which already reported the real cz-cli version and is untouched here. No object filters on the literal 'local'. So nothing downstream depends on the old agent-path value. PR body updated with this.
Test coverage added: otel/resource.ts is now a pure builder with 4 tests (defaults, override precedence, no identity on the resource, no bare opencode version).
| attributes: commandAttributes(event), | ||
| attributes: [ | ||
| ...commandAttributes(event), | ||
| ...Object.entries(identity).map(([k, v]) => ({ key: k, value: { stringValue: v } })), |
There was a problem hiding this comment.
MEDIUM (confidence: high on the mechanism, this is a sequencing call not a code defect) — this is a hard cutover with no overlap window, and the compensating change lives in another repo.
...Object.entries(identity).map(([k, v]) => ({ key: k, value: { stringValue: v } })),The PR body already records that czcli.public.czcli_otel_analytics reads resourceattributes['enduser.id'] and "must move to logattributes for new rows". Since identity is removed from the resource in the same commit, that view returns NULL identity for every row written between this merge and the warehouse edit — and the binary carrying the change ships on its own schedule, so the gap is however long that takes. ai_agent_user_daily is fine because it already COALESCEs both layers.
The cheap way to close the gap is to write both layers for one release (record attribute and resource attribute), then drop the resource copy once the view is migrated. The cost is that the cardinality problem the PR is fixing persists for that release, and the new exactly once assertion in profile-telemetry.test.ts:100 would have to become "at least once" — which is why I'm raising this as a sequencing question rather than asserting the code is wrong. If the plan is instead "edit the view first, then merge", saying so in the PR body would make it reviewable.
Separately and minor: IdentityAttributes is Partial<Record<…, string>>, so v is string | undefined here. identityAttributes() never produces an undefined value, but a hand-built event.identityAttributes (as in telemetry.test.ts:38) could, and it would serialize as "value":{}. Not reachable from current callers.
There was a problem hiding this comment.
Sequencing call, and you are right that the PR body did not state it. The plan is view-first, not dual-write: czcli_otel_analytics migrates to COALESCE(ELEMENT_AT(logattributes,'enduser.id'), ELEMENT_AT(resourceattributes,'enduser.id')) — the shape ai_agent_user_daily already uses — BEFORE a binary carrying this change ships.
That closes the gap without dual-writing: reading both layers costs nothing and keeps history queryable, while the emitter stays at exactly one key, so the cardinality fix lands immediately and the exactly once assertion stays as-is. Dual-writing would have inverted that trade. The view edit is warehouse-side and is not done yet — it is awaiting the owner's go-ahead, and I have flagged it as the merge blocker in the PR body rather than leaving it as a follow-up.
Second point taken: undefined values are now filtered before serialisation, so a hand-built identityAttributes cannot emit "value":{}.
There was a problem hiding this comment.
Done — view migrated 2026-09-14, so this is no longer a blocker. CREATE OR REPLACE SEMANTIC VIEW, previous DDL backed up first.
All five consumers of a moved key now COALESCE both layers, not four: the sweep had missed logs.unique_user_count, which reads enduser.id inside count(DISTINCT …) as a metric rather than as a dimension. Verified on live data that identity resolves for both the old resource-shaped rows and the new record-shaped ones.
The view also turned out to be stale for reasons predating this PR — nine dimensions/metrics read an OpenInference/Arize vocabulary (llm.model_name, llm.token_count.*, session.id, agent.name) this project has never emitted, so both token metrics had returned 0 for their entire life. Repointed at gen_ai.* / opencode.session.id with names, synonyms and comments unchanged, so no existing phrasing breaks; they now report real figures. Details in the PR body.
| export type IdentityAttributes = Partial<Record<(typeof FIELDS)[number][1], string>> | ||
|
|
||
| /** Map a profile row (or a login's not-yet-persisted equivalent) onto the attributes. */ | ||
| export function identityAttributes(row: Record<string, unknown> | undefined): IdentityAttributes { |
There was a problem hiding this comment.
LOW (confidence: high) — the "only one spelling" guarantee covers the output key but not the input field, so the one caller this refactor exists for can still silently drop enduser.id.
export function identityAttributes(row: Record<string, unknown> | undefined): IdentityAttributes {Record<string, unknown> accepts any key. setup.ts:47-52 hand-builds the row, so user_id: opts.userId written as userId: opts.userId compiles fine, FIELDS finds nothing at row["user_id"], and the attribute vanishes — no compile error, and no test asserts setup's identity output (telemetry.test.ts:38 passes identityAttributes pre-built, bypassing this function). That is the same class of silent divergence the doc comment above says this function exists to prevent; it's just moved from the attribute name to the field name.
The input side can carry the same guarantee the output side does:
| export function identityAttributes(row: Record<string, unknown> | undefined): IdentityAttributes { | |
| export type IdentityRow = Partial<Record<(typeof FIELDS)[number][0], unknown>> | |
| /** Map a profile row (or a login's not-yet-persisted equivalent) onto the attributes. */ | |
| export function identityAttributes(row: IdentityRow | undefined): IdentityAttributes { |
readProfileEntry's return type still needs to be assignable — if ProfileEntry is an index-signature type it will be; if not, row: IdentityRow | Record<string, unknown> | undefined gets the compile-time check for the literal call site in setup.ts while leaving the profile-store path untouched.
There was a problem hiding this comment.
Fixed — IdentityRow added in c594ec7, using your suggestion. Mutation-checked: renaming the literal's user_id to userId now fails with TS2561 ("Did you mean to write 'user_id'?"). ProfileEntry is Record<string, unknown> so the profile-store path still assigns without a second overload.
Review summaryA. Upstream invasiveness — no issues foundAll seven changed files are under Worth noting for the ledger's benefit: B. Clean fix or hole drilled around the problem — mostly the clean fix, one exceptionThe The exception is the One smaller point on the "only one spelling" claim: the guarantee holds for the OTel attribute name but not for the profile field name on the input side, so C. Regression riskBehavioral changes I can identify, and their test coverage:
No tests were deleted, skipped, or loosened. I have not run anything, so nothing above is a claim that the suite passes. |
Review found the new `opencode.version` key could never hold opencode's version.
`InstallationVersion` is `typeof OPENCODE_VERSION === "string" ? OPENCODE_VERSION :
"local"` (core/installation/version.ts:6), and this module ships in the plugin runtime
asset — the bundle whose missing `define` block this branch is fixing. Only
CLICKZETTA_VERSION was added there, so the attribute was the constant "local": the
exact defect being fixed, relocated to a new key. The other direction is no better —
the binary's own OPENCODE_VERSION define is fed `Script.version`, the cz-cli version,
so it would just duplicate service.version. Key dropped; there is no separate upstream
version worth reporting from here.
That also corrects the previous commit's reasoning: service.version read "local"
because the runtime asset never saw the binary's defines, NOT because the build was an
unpublished opencode release.
The resource is now built by a pure `otelResourceAttributes()` in otel/resource.ts, so
it can be asserted without booting the plugin. It had no test at all, and setup.ts
hands one resource object to the logger, the tracer AND the meter — so a wrong value
there lands on all three signals with nothing to catch it.
`identityAttributes()` took `Record<string, unknown>`, so the guarantee covered the
attribute name but not the profile field name: setup.ts's hand-built row could spell
`userId` for `user_id` and silently drop enduser.id — the same divergence one level
down. `IdentityRow` makes it TS2561, verified by mutating the literal. ProfileEntry is
an index-signature type, so the profile-store path still assigns.
Also filter undefined out of the identity entries: IdentityAttributes is Partial, and
an explicit undefined serialised as `"value":{}` instead of being omitted. Not
reachable from current callers.
Measured for the reviewer's second point: nothing downstream depends on the agent
path's old service.version. Across all 14 derived objects in czcli.public, only
czcli_otel_analytics reads service.version at all, and it reads it from otel_logs —
the CLI path, which already reported the real version. No object filters on 'local'.
Verified: typecheck clean, 99 tests pass across 9 otel/telemetry/setup files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| resource: { | ||
| attributes: [ | ||
| { key: "service.name", value: { stringValue: "cz-cli" } }, | ||
| { key: "service.version", value: { stringValue: VERSION } }, | ||
| ...Object.entries(resourceAttrs).map(([k, v]) => ({ key: k, value: { stringValue: v } })), | ||
| ], | ||
| }, |
There was a problem hiding this comment.
MEDIUM — regression risk on the consumer side (high confidence).
resource: {
attributes: [
{ key: "service.name", value: { stringValue: "cz-cli" } },
{ key: "service.version", value: { stringValue: VERSION } },
],
},The reasoning for the move is sound (a Resource is immutable per provider, and enduser.id/instance.name/workspace.name/service.url are per-event facts), and it makes the log path agree with the trace path in otel/handlers.ts. What I want to flag is the blast radius outside this repo, which nothing in the PR records:
- Most OTLP backends store resource attributes in a separate column/index from record attributes (ClickHouse exporter:
ResourceAttributesvsLogAttributes; Loki/Grafana promote resource attrs to stream labels). Any existing query, dashboard, saved filter, or collectorattributes/transformprocessor that reads these four keys off the resource returns nothing after this ships, silently — no error, just empty panels. - During rollout the two shapes coexist: an installed 2.0.x binary writes identity on the resource while a new one writes it on the record, to the same collector. Anything aggregating over the transition window sees each user twice or not at all depending on which side it reads.
packages/cz-cli/test/profile-telemetry.test.ts and test/telemetry.test.ts pin the new payload shape, so the producer side is covered. Nothing in this repo covers the consumer side, and I couldn't find a collector config or dashboard definition checked in here to update (grep -rn enduser over *.md/*.yaml/*.json finds nothing outside packages/cz-cli), so the migration is presumably external. Worth a note in the PR body or release notes naming the four keys that moved, so whoever owns the dashboards knows to update them.
No change requested to the code — this is a heads-up about coordination.
There was a problem hiding this comment.
Agreed on coordination, and the PR body now names all four keys that moved plus the rollout-window double-count. The four are enduser.id, instance.name, workspace.name, service.url.
For the record on scope: I swept czcli.public and czcli_otel_analytics is the only checked-in-or-otherwise consumer that reads any of them off the log resource; ai_agent_user_daily already COALESCEs. Nothing else in the repo defines a collector config or dashboard, matching your grep. The view migration is the stated merge blocker.
| // cz_change: runtime assets are bundled SEPARATELY from the binary, so the binary's | ||
| // `define` block never reached them and anything here importing version.ts read the | ||
| // 0.0.0-dev+<ts> fallback — the same drop that once broke `cz-cli --version`. The otel | ||
| // plugin now stamps this into the trace resource's service.version, so it has to be | ||
| // the real release version. | ||
| define: { | ||
| CLICKZETTA_VERSION: `'${Script.version}'`, | ||
| }, |
There was a problem hiding this comment.
MEDIUM — the fix is one key short of the root cause (high confidence on the fact, judgement call on the fix).
define: {
CLICKZETTA_VERSION: `'${Script.version}'`,
},The diagnosis in the comment is right, and I verified it: buildRuntimeAsset had no define at all, so every build-time global in an asset's module graph resolved to its dev fallback. But CLICKZETTA_VERSION is not the only one reachable from these entrypoints.
src/opencode-plugin/server.ts → otel/index.ts → ./handlers → ../../telemetry.js (for isSensitiveKey/isSensitiveValue, handlers.ts:16) → ./otel-defaults.js, whose module-level initialiser is:
export const OTEL_DEFAULTS = {
endpoint: typeof CLICKZETTA_OTEL_ENDPOINT === "string" ? CLICKZETTA_OTEL_ENDPOINT : "",
headers: typeof CLICKZETTA_OTEL_HEADERS === "string" ? CLICKZETTA_OTEL_HEADERS : "",
}So OTEL_DEFAULTS.endpoint is "" inside the plugin asset in a released build, exactly the way VERSION was 0.0.0-dev+…. Nothing in the asset calls trackCommand or applyDefaultOtelEnv today, so there is no live symptom — but it is the same latent trap this PR is fixing, one import away, and the failure mode is the same silent one (trackCommand early-returns on an empty endpoint and reports nothing).
The smaller correct change is to make the two builds share one source of truth for the CLICKZETTA_* injections rather than list one key here and three at line 271. Something like:
const CZ_DEFINES = {
CLICKZETTA_VERSION: `'${Script.version}'`,
CLICKZETTA_OTEL_ENDPOINT: JSON.stringify(process.env.CLICKZETTA_OTEL_ENDPOINT ?? ""),
CLICKZETTA_OTEL_HEADERS: JSON.stringify(process.env.CLICKZETTA_OTEL_HEADERS ?? ""),
}spread into both define blocks. That also removes the need for a future reader to work out which of the two bundles a given global reaches.
Note this cuts the other way for OPENCODE_VERSION: @opencode-ai/core/flag/flag is the only upstream value-import in this graph and it imports nothing but effect, so InstallationVersion is not currently reachable from an asset bundle and does not need a define — which is consistent with resource.ts moving off it.
There was a problem hiding this comment.
Fact confirmed and it is a real latent trap — server.ts → otel/index.ts → handlers → telemetry.js → otel-defaults.js does reach it, and OTEL_DEFAULTS.endpoint is "" in that bundle.
Declining the injection, with the reasoning recorded in the define block itself. The plugin never reads OTEL_DEFAULTS: it takes the collector from OPENCODE_OTLP_ENDPOINT/_HEADERS (index.ts:36-37), which applyDefaultOtelEnv() fills from those same defines in the parent process. So injecting them here would change no behavior while shipping the collector credentials in a second artifact — a trade I would rather not make silently for a latent case. What is missing is the knowledge, not the value, so the comment says what to do if anything in an asset ever calls trackCommand directly: add them, and share one define block with the binary build.
Happy to switch to the shared CZ_DEFINES block if you would rather have the symmetry now — it is a one-line change once the credential question is settled by whoever owns the release pipeline.
| const resourceAttrs = otelResourceAttributes({ | ||
| serviceName: process.env.OPENCODE_SERVICE_NAME, | ||
| version: VERSION, | ||
| client: Flag.OPENCODE_CLIENT, |
There was a problem hiding this comment.
LOW — service.version becomes unbounded-cardinality in unreleased builds (medium confidence).
version: VERSION,VERSION falls back to a timestamp when the define is absent (src/version.ts:4-5):
const ts = new Date().toISOString().slice(0, 16).replace(/[-:T]/g, "")
export const VERSION = typeof CLICKZETTA_VERSION === "string" ? CLICKZETTA_VERSION : `0.0.0-dev+${ts}`Released builds now get the real version, which is the point of the PR. But this value lands on the Resource, which keys the resource/time-series in most backends, and in a source run it changes every minute — 0.0.0-dev+202609141503, …1504, and so on, one distinct value per process. Previously it was InstallationVersion, i.e. the single constant "local", so dev traffic aggregated into one series.
This is reachable: OPENCODE_OTLP_ENDPOINT is a plain env var read at index.ts:37, so anyone running bun run against the internal collector (or a local one) produces it. OTEL_DEFAULTS.endpoint being empty in a source build only stops the CLI trackCommand path, not this one.
Small enough that it may not be worth acting on, but if you want the dev case to stay aggregatable, either pass a stable dev sentinel here:
version: VERSION.startsWith("0.0.0-dev+") ? "local" : VERSION,or drop the timestamp from version.ts's fallback. The timestamp appears to exist for cz-cli --version legibility, so the first option keeps that intact.
There was a problem hiding this comment.
Fixed in ded3a3e, using your first option but in the pure builder so it is testable:
"service.version": input.version.startsWith("0.0.0-dev+") ? "local" : input.version,Dev traffic aggregates under the same constant it always did, --version keeps its timestamp, and released builds are unaffected. Test asserts two versions a minute apart produce one series.
| if (!OTEL_DEFAULTS.endpoint) return Promise.resolve() | ||
| try { | ||
| const resourceAttrs = event.resourceAttributes ?? profileTelemetryAttributes() | ||
| const identity = event.identityAttributes ?? profileTelemetryAttributes() |
There was a problem hiding this comment.
LOW — the new doc comment describes a fallback that ?? does not provide (high confidence, pre-existing behavior).
const identity = event.identityAttributes ?? profileTelemetryAttributes()The field's docstring above reads "Identity for this event, when the active profile cannot supply it". ?? only falls back on undefined, and trackSetup (commands/setup.ts:47-52,74) always passes the object identityAttributes() returns — which is {} whenever opts.userId and opts.collected are all absent. So on a setup run that fails early, the empty object wins and profileTelemetryAttributes() is never consulted, even on a machine that already has a default profile with a user_id.
Several trackSetup call sites are on that path — setup.ts:479 and 1037 fire before the credentials/instance steps complete — so an early-failure setup event carries no identity at all. Those are exactly the failures you'd most want attributed.
This is unchanged from before the PR (resourceAttributes: attrs had the same shape, so ?? never fired then either), so it is not a regression. But the new comment now asserts the opposite, and the one-line fix is right here:
const identity = event.identityAttributes && Object.keys(event.identityAttributes).length
? event.identityAttributes
: profileTelemetryAttributes()Alternatively have trackSetup pass undefined when the row is empty. Either way, worth making the code and the comment agree.
There was a problem hiding this comment.
Correct, and fixed in ded3a3e — the comment was describing behavior the ?? never had. Empty now falls back:
const override = event.identityAttributes
const identity = override && Object.keys(override).length > 0 ? override : profileTelemetryAttributes()Chose this over having trackSetup pass undefined so the invariant holds for any future caller, not just that one. New test asserts a setup failure with identityAttributes: {} still carries the active profile's id; reverting to ?? turns it red.
| const attrs = identityAttributes({ | ||
| user_id: opts.userId, | ||
| instance: opts.collected?.instance, | ||
| workspace: opts.collected?.workspace, | ||
| service: opts.collected?.service, | ||
| }) |
There was a problem hiding this comment.
LOW — confirm intent: userId === 0 now reports where it used to be dropped (medium confidence).
const attrs = identityAttributes({
user_id: opts.userId,The old code guarded on truthiness:
if (opts.userId) attrs["enduser.id"] = String(opts.userId)identityAttributes guards on typeof value === "number" && Number.isFinite(value) (connection/telemetry.ts:40), which accepts 0. So a setup run whose token resolves userId: 0 now emits enduser.id: "0" instead of omitting the key.
That is almost certainly the better behavior — 0 is a real id if the API ever returns one, and it matches what profileTelemetryAttributes() has always done for a profile row with user_id = 0, so the change actually removes a divergence between the two paths rather than creating one. Flagging only because it is a silent semantic shift that the PR description doesn't mention, and because if 0 is a sentinel for "unknown user" anywhere in the ClickZetta API it would start showing up as a real cohort in analytics. Confirm which it is.
The three string fields are equivalent to before (old truthiness check dropped "", the new one drops "" and whitespace-only).
There was a problem hiding this comment.
Answered by the repo, and the answer is the opposite of the guess: 0 is the sentinel, so this was a regression and is fixed in ded3a3e.
login-browser.ts:452passesuserId: 0for a login that keeps a token it could not attribute.connection/token-source.ts:69defaults toids?.userId ?? 0.profile-store.ts:531already refuses to persist it:if (typeof fields.userId === "number" && fields.userId > 0).
So enduser.id = "0" would have collapsed every unattributable session into one large fake cohort. Guard is > 0 now, placed in identityAttributes so setup's literal and the profile path share one rule. Mutation-checked.
Related, out of scope: patchProfileUserId has no such guard and its user_id != null early-return means a persisted 0 would never be corrected — but both live callers (tui-quota-data.ts:337, context.ts:218) guard on truthiness, so it is unreachable today. Left alone deliberately.
Review summaryFive inline findings, none blocking: one MEDIUM on the build wiring, one MEDIUM on consumer-side regression risk, three LOW. Verdict per section below. I did not run the test suite, so nothing here claims anything passes. A. Upstream invasiveness — no issues foundAll nine changed files are under Worth noting in the other direction: the PR reduces upstream coupling. The B. Clean fix or hole drilled around it — mostly the clean fix, one findingGenuinely the right shape, and worth saying so:
One finding: C. Regression riskBehavioral changes I found, with test coverage:
New exported surface, all additive: No tests deleted or skipped, and no assertions loosened. 🤖 Generated with Claude Code |
Second review pass. Three of five findings were real; all three are fixed here.
`identityAttributes` accepted `user_id: 0` because the guard was `Number.isFinite`,
where the code it replaced guarded on truthiness. 0 is this codebase's "unknown user"
sentinel, not an id: login-browser.ts:452 hands it out when a login keeps a token it
could not attribute, token-source.ts:69 defaults to it, and profile-store.ts:531
already refuses to persist it. Emitting `enduser.id = "0"` would have turned every
unattributable session into one large fake cohort. The guard is `> 0` now, at the shared
seam, so setup's hand-built row and the profile path are covered by one rule. (The
review guessed this was the better behavior; the persist site's own guard says
otherwise.) patchProfileUserId has no such guard, but both live callers check
truthiness, so a 0 cannot reach the profile today — left alone.
`event.identityAttributes ?? profileTelemetryAttributes()` never fell back for
trackSetup, which always passes a row: that row is `{}` on a setup that failed before it
learned anything (setup.ts:479, :1037), so the empty object won and exactly the failures
most worth attributing went out anonymous — on machines whose default profile knew the
user all along. Empty now falls back too. Pre-existing, but the docstring this branch
added claimed the opposite.
`service.version` on the resource took VERSION verbatim, and a source build has no
CLICKZETTA_VERSION define, so it carried a per-minute timestamp — one new time series
per dev process, where InstallationVersion used to give the single constant "local".
Dev collapses back to that sentinel; released builds report the real version.
Both new guards are mutation-checked: reverting either turns its test red.
Declined, with the reasoning in the build.ts comment: injecting
CLICKZETTA_OTEL_ENDPOINT/HEADERS into the runtime assets. otel-defaults.ts is reachable
from that graph, so the latent trap is real, but the plugin takes the collector from
OPENCODE_OTLP_* env — applyDefaultOtelEnv() fills those in the parent — so injecting
would ship the credentials in a second artifact and change nothing. Noted in place for
whoever adds the first direct trackCommand call from an asset.
Verified: typecheck clean, 102 tests pass across 9 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| const override = event.identityAttributes | ||
| const identity = override && Object.keys(override).length > 0 ? override : profileTelemetryAttributes() |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the length > 0 threshold only rescues the fully empty row, and the setup failure paths almost never produce one.
const override = event.identityAttributes
const identity = override && Object.keys(override).length > 0 ? override : profileTelemetryAttributes()The comment says the empty-object case is "exactly the failures you most want attributed anonymous, on a machine whose default profile knew the user all along". But trackSetup builds its row from four independent sources (setup.ts:47-52), and on the failure paths userId is always absent while collected usually is not:
setup.ts:1750passescollected: { username, instance, workspace, service }read straight offrawArgv, nouserId.setup.ts:1792passescollected: ttyCollected, populated as the wizard walks, nouserId.- The success paths at
setup.ts:479andsetup.ts:1037setuserId = token.userId || undefinedinside atry {} catch {}— so agetTokenfailure there also yields a row withinstance/workspace/serviceand noenduser.id.
In every one of those, Object.keys(override).length is ≥ 1, override wins, and the event goes out with instance.name but no enduser.id — anonymous, on a machine whose default profile knew the user. That is the same defect at a different threshold.
The root cause looks like identityAttributes conflating one identity attribute (enduser.id) with three context attributes (instance.name, workspace.name, service.url): "did the caller supply identity?" is being answered by asking whether it supplied anything. Keying the decision on the identity attribute itself, and letting the caller's context win over the profile's, covers all of these:
| const override = event.identityAttributes | |
| const identity = override && Object.keys(override).length > 0 ? override : profileTelemetryAttributes() | |
| // Fall back per-CONCERN, not on the row as a whole: trackSetup's row carries three | |
| // CONTEXT attributes (instance/workspace/service, what the run was trying to set up) | |
| // independently of the one IDENTITY attribute, and on a failed setup it has the former | |
| // and not the latter. Keying on `Object.keys(...).length` let that partial row suppress | |
| // the profile lookup, leaving exactly the failures you most want attributed anonymous | |
| // on a machine whose default profile knew the user all along. | |
| const override = event.identityAttributes | |
| const identity = override?.["enduser.id"] ? override : { ...profileTelemetryAttributes(), ...override } |
The new test an empty identity override falls back to the active profile (test/profile-telemetry.test.ts:161) pins only the {} case; nothing covers a partially populated row, which is the shape the live failure paths actually send.
| ...Object.entries(identity).flatMap(([k, v]) => | ||
| v === undefined ? [] : [{ key: k, value: { stringValue: v } }], | ||
| ), |
There was a problem hiding this comment.
MEDIUM (confidence: high on the code, cannot verify the warehouse) — four attributes moved off the Resource, not one, and the stated migration only covers one of them.
...Object.entries(identity).flatMap(([k, v]) =>
v === undefined ? [] : [{ key: k, value: { stringValue: v } }],
),identity is the full return of identityAttributes, which is all four of FIELDS (connection/telemetry.ts:13-18): enduser.id, instance.name, workspace.name, service.url. Before this PR all four were spread into resource.attributes by the deleted line. So every one of them changes layer for CLI-path logs, and test/profile-telemetry.test.ts:117-120 asserts exactly that — all four now on the log record.
The PR description's before/after table lists only enduser.id, and the merge-blocking warehouse edit is scoped to it:
COALESCE(ELEMENT_AT(logattributes, 'enduser.id'), ELEMENT_AT(resourceattributes, 'enduser.id'))The sweep is described as "czcli_otel_analytics and ai_agent_user_daily are the only two that touch the key" — singular. Could you confirm the same sweep of the 14 derived objects was run for resourceattributes['instance.name'], ['workspace.name'] and ['service.url']? Any of those three read off the resource returns NULL for every row written after this ships, with the same permanent split at the cutover and the same COALESCE fix. There is no SQL in this repo (I grepped resourceattributes/logattributes across the tree — only packages/core/src/observability/otlp.ts and these tests), so this is not checkable from here.
Separately worth a sanity check downstream: service.url now sits on the log record while service.name and service.version stay on the resource. Any consumer that reads the service.* group from one map will only find two of the three.
| resource: { | ||
| attributes: [ | ||
| { key: "service.name", value: { stringValue: "cz-cli" } }, | ||
| { key: "service.version", value: { stringValue: VERSION } }, |
There was a problem hiding this comment.
MEDIUM (confidence: high) — the new "don't put a per-minute version on a Resource" rule is applied to one of the two places that write a service.version resource attribute.
{ key: "service.version", value: { stringValue: VERSION } },resource.ts:26-30 states the rule and collapses the dev version to a stable sentinel because "on a RESOURCE that is a new time series every minute". This line writes VERSION raw to a Resource on the CLI path, from the same src/version.ts module, so it carries 0.0.0-dev+<YYYYMMDDHHmm> on every source run and (per my comment on resource.ts:30) 0.0.0-<branch>-<YYYYMMDDHHmm> on every non-release build. Same hazard, same table (czcli.public.otel_logs), untouched.
This is pre-existing rather than introduced here, so it is a judgement call whether it belongs in this PR — but the PR is what establishes the rule, and leaving the second call site on the old behavior means dev traffic aggregates under local for agent-path rows and fans out per minute for CLI-path rows, so a query grouping by service.version across the table sees two different spellings of "not a release". If a shared helper is the answer, otelResourceAttributes is already a pure builder and this path could call the same normalizer for its one field.
test/profile-telemetry.test.ts:107 pins this as expect.any(String) and test/telemetry.test.ts:76-79 pins only the key list, so neither would notice either way.
| if (typeof value === "string" && value.trim()) return [[attribute, value]] | ||
| // `> 0`, not `isFinite`: 0 is this codebase's "unknown user" sentinel, not an id — | ||
| // login-browser.ts:452 hands it out when a login cannot learn the user, and | ||
| // profile-store.ts:531 already refuses to persist it. Reporting `enduser.id = "0"` | ||
| // would invent a cohort out of every unattributable session. | ||
| if (typeof value === "number" && Number.isFinite(value) && value > 0) return [[attribute, String(value)]] |
There was a problem hiding this comment.
LOW (confidence: medium) — the new > 0 sentinel guard is applied on the wrong axis: it covers every field but only the number branch.
if (typeof value === "string" && value.trim()) return [[attribute, value]]
// `> 0`, not `isFinite`: 0 is this codebase's "unknown user" sentinel, not an id —
...
if (typeof value === "number" && Number.isFinite(value) && value > 0) return [[attribute, String(value)]]Two mismatches between the rule and the docstring:
-
The string branch runs first and has no guard.
readProfileEntryreturns raw parsed TOML (profile-store.ts:183-190,ProfileEntry = Record<string, unknown>), so aprofiles.tomlwithuser_id = "0"— hand-edited, or written by any older/other tool — takes the string branch and emitsenduser.id = "0", which is exactly the cohort the comment says must not exist.profile-store.tsguards its write path withtypeof fields.userId === "number" && fields.userId > 0, and this is the read side of the same field, so the two disagree about what a sentinel is. Routing the numeric coercion throughnumericField(already exported fromprofile-store.ts:76for exactly this "callers stop growing their own copies" reason) and then applying> 0once would close it. -
> 0applies toinstance/workspace/servicetoo. Those are strings in practice so nothing changes today, but the rule is documented as being about the user-id sentinel and is enforced on all four fields. A numericinstanceof0in a profile would now be silently dropped rather than reported.
Also a nit on the comment itself: it cites login-browser.ts:452, and there is no login-browser.ts anywhere in the tree (fd login-browser finds nothing). Both file:line citations here will drift; the profile-store.ts:531 one is already off by a few lines. Naming the function (patchProfileUserId / the login helper) rather than the line would survive edits.
Note this is a behavior change beyond the sentinel: profileTelemetryAttributes previously accepted any finite number, so a negative user_id used to be reported and now is not. Covered by the new user_id = 0 is not an identity test at test/profile-telemetry.test.ts:141; the string-"0" and negative cases are not.
|
Review summary A. Upstream invasiveness — no issues found All nine changed files are under B. Clean fix vs. hole drilled around the problem — mostly clean, two exceptions Genuinely the right fix, and worth saying so: collapsing the two copies of the Two places treat the symptom:
No dead code, no leftover debug logging, no unrelated drive-bys. The C. Regression risk Behavioral changes I could identify, and their coverage:
No tests deleted or skipped. I could not run the test suite or a build, so nothing above is a claim that anything passes. |
| // timestamp. On a RESOURCE that is a new time series every minute — dev traffic used to | ||
| // aggregate under the single constant "local", and this keeps that, since a dev build | ||
| // has no release to segment by anyway. Released builds report the real version. | ||
| "service.version": input.version.startsWith("0.0.0-dev+") ? "local" : input.version, |
There was a problem hiding this comment.
HIGH (confidence: high) — the dev-collapse guard misses the format that non-release builds actually produce, so this change introduces the per-minute resource series it is written to prevent.
"service.version": input.version.startsWith("0.0.0-dev+") ? "local" : input.version,0.0.0-dev+<ts> is only the fallback in src/version.ts:5, reached when CLICKZETTA_VERSION is undefined. But this PR's other half adds that define to buildRuntimeAsset (script/build.ts:60-62), so a built asset now carries Script.version — and for any build that is not a release, packages/script/src/index.ts:36-38 gives:
if (IS_PREVIEW) return `0.0.0-${CHANNEL}-${new Date().toISOString().slice(0, 16).replace(/[-:T]/g, "")}`
CHANNEL there is the git branch name, and the timestamp is sliced to the minute. So a local bun run build:local, or any preview-channel build, stamps something like 0.0.0-fix/otel-enduser-id-placement-202609141503. That does not start with 0.0.0-dev+, so it flows straight through to service.version on the Resource — a fresh time series per branch per build-minute, on all three signals.
This is a regression against the pre-PR behavior specifically: before this PR the asset had no define at all, so InstallationVersion resolved to the constant "local" for every non-release build. The comment above the line ("dev traffic used to aggregate under the single constant local, and this keeps that") describes the intent correctly; the predicate is just too narrow to deliver it now that the define exists.
Both non-release shapes start with 0.0.0-, and no real release version does (Script.version releases are major.minor.patch), so matching on that prefix covers both:
| "service.version": input.version.startsWith("0.0.0-dev+") ? "local" : input.version, | |
| "service.version": input.version.startsWith("0.0.0-") ? "local" : input.version, |
Worth noting either way that "0.0.0-dev+" is a literal duplicated across version.ts, resource.ts and test/otel-resource.test.ts with nothing tying them together — if the fallback format in version.ts is ever changed, the collapse silently stops firing and no test goes red. A single exported constant (or a isDevVersion() helper next to VERSION) would make that coupling checkable.
test/otel-resource.test.ts covers 0.0.0-dev+1 and 0.0.0-dev+<ts>; nothing covers the 0.0.0-<branch>-<ts> shape.
1 participant
What
enduser.idwas emitted twice in two different layers depending on the signal:otel_tracesspanattributes['enduser.id']otel_logsresourceattributes['enduser.id']logattributes['enduser.id']Measured on
czcli.public.otel_traces/otel_logsbefore the fix: traces 6/6 on span attributes, logs 27/27 on resource attributes.Four keys move on the log path —
enduser.id,instance.name,workspace.name,service.url. Anything reading them off the log resource (dashboard, saved filter, collectortransformprocessor, Loki stream label) goes silently empty on new rows, and during rollout old and new binaries write both shapes to the same collector, so aggregations spanning the transition can double-count or miss a user depending on which layer they read. See the merge blocker below.One semantic fix rides along:
user_id = 0is no longer reported as an identity. It is this codebase's "unknown user" sentinel —login-browser.ts:452hands it out for a login that could not attribute its token, andprofile-store.ts:531already refuses to persist it — so emittingenduser.id = "0"would have collapsed every unattributable session into one fake cohort. The pre-PR truthiness check dropped it; the refactor briefly did not.Why the resource was wrong
The OTel resource spec defines a Resource as "an immutable representation of the observed entity for which telemetry is being produced", bound to the provider at creation — "that association cannot be changed later". An end user is not the producing entity, and in
cz-cliidentity is not even constant per process:connection/telemetry.tsdeliberately re-reads the active profile on every operation, because a profile can be switched mid-process and becauseuser_idmay be written by a subprocess after the session starts.Concretely it cost us:
(user, instance, workspace)triple looked like a distinct service instance downstream;agent,serve) would pin its first profile onto every later log record.enduser.iditself isDevelopment-stability and PII-flagged in the registry; this PR does not change what we record, only where.Only one spelling
commands/setup.tshand-wroteattrs["enduser.id"] = String(opts.userId)— a second copy of a mappingconnection/telemetry.tsalready owned, which is how one copy could sit on the wrong layer while the other was right. Both paths now go throughidentityAttributes().The guarantee is compile-enforced on both sides:
IdentityAttributesmakes inventing a fifth attribute a type error, andIdentityRowdoes the same for the profile field names — without it,setup.ts's hand-built row could spelluserIdwhereFIELDSreadsuser_idand silently drop the attribute.Also: service.version on the agent path was never real
The trace resource said
service.name: cz-agentbutservice.version: InstallationVersion. That istypeof OPENCODE_VERSION === "string" ? OPENCODE_VERSION : "local"— and the plugin ships in a runtime asset, bundled separately from the binary, which never saw the binary'sdefineblock. So the value was the constant"local"in every build, released or not, and cz-agent telemetry carried no version to segment a release by; the live table showslocaleven for a freshly installed 2.0.5 binary. It isVERSIONnow.That required
script/build.ts: same missing-defineroot cause, soversion.tswas also falling back to0.0.0-dev+<ts>inside the plugin — the drop that once brokecz-cli --version.CLICKZETTA_VERSIONis now stamped into runtime assets too.The resource reaches the LoggerProvider, TracerProvider and MeterProvider as one object (
otel/setup.ts:45-65), so this flipsservice.versionon agent-path logs and metrics as well as traces — intended, and swept: of the 14 derived objects inczcli.publiconlyczcli_otel_analyticsreadsservice.version, and it reads it fromotel_logs(the CLI path, which already reported the real version). Nothing filters on the literal'local'.An earlier revision of this branch also added
opencode.version: InstallationVersion. Dropped — in this bundle it is that same"local"constant, and in the binaryOPENCODE_VERSIONis itself fedScript.version, so it could only ever duplicateservice.versionor repeat the bug under a new name.otel/resource.tsis now a pure builder so the resource is testable without booting the plugin; it previously had no coverage at all.Warehouse side: done, view-first (no longer a blocker)
czcli.public.czcli_otel_analyticsreadsresourceattributes['enduser.id']and would return NULL identity for every row written after a binary with this change ships. Sequencing is view-first, not dual-write: migrate that view to— the shape
ai_agent_user_dailyalready uses — before shipping. Reading both layers costs nothing and keeps history queryable, while the emitter stays at exactly one key, so the cardinality fix lands immediately instead of being deferred a release.Applied 2026-09-14 (
CREATE OR REPLACE SEMANTIC VIEW, previous DDL backed up). All five consumers of a moved key now COALESCE both layers: dimensionslogs.user_id,logs.service_url,logs.instance_name,logs.workspace, and the metriclogs.unique_user_count— that last one is a fifth consumer the earlier sweep missed, since it reads the key insidecount(DISTINCT …)rather than as a dimension. Verified against live data: identity resolves for both the old resource-shaped rows and the new record-shaped ones.While in there, the same view turned out to be largely stale for reasons predating this PR, and was repointed at the vocabulary we actually emit (names, synonyms and comments left unchanged, so no existing phrasing breaks):
traces.model_namellm.model_namegen_ai.request.modeltraces.llm_providerllm.providergen_ai.provider.nametraces.agent_nameagent.namegen_ai.agent.nametraces.session_idsession.idopencode.session.idtraces.usernameresourceattributes['user.name']spanattributes['enduser.id']traces.instance_name/workspacetotal_tokens/input_tokens/output_tokens/cache_read_tokensllm.token_count.*gen_ai.usage.*Those nine read an OpenInference/Arize vocabulary this project has never emitted, so both token metrics had been returning 0 for their whole life; they now report real numbers (129M tokens on the top model over 30 days). Added
traces.tool_name,traces.cache_creation_tokens,traces.reasoning_tokens,traces.unique_session_countfrom keys already in the data.traces.project_idandtraces.total_costhave no source at all and are annotated as such rather than deleted;logs.log_level/log_subsystemreadopenclaw.*keys and were left alone.Swept all 14 derived objects in that schema:
czcli_otel_analyticsandai_agent_user_dailyare the only two that touch the key, and the latter needed no change. Rows written before this ships keep identity on the resource, permanently — nothing backfills.Verification
bun run typecheckclean.telemetry,profile-telemetry,telemetry-identity,otel-resource,otel-span-build,otel-event-contract,otel-config,setup*./"enduser\.id"/g→ exactly 1) rather than checking presence; re-adding the resource line turns it red — mutation-checked.user_idtouserIdinsetup.tsfails with TS2561 rather than silently dropping the attribute.toMatchObjectsubstitutes the expected matchers into the received object, so a string assertion placed after it reads the expectation back. The payload is serialized before the matcher runs, with a comment saying why.0.0.0-devfallback remains.Not verified: no released binary has been built and run end to end with this change; the live-table measurements above come from the pre-fix 2.0.5 binary.
🤖 Generated with Claude Code