feat(metrics)!: drop session.id and project.id from metric labels - #124
Open
ShawnZhang31 wants to merge 2 commits into
Open
ShawnZhang31 wants to merge 2 commits into
ShawnZhang31 wants to merge 2 commits into
Conversation
…sion end Remove the `session.id` attribute from all 15 metric instruments. It was the dominant source of Prometheus series growth: every label combination is a separate time series and a histogram multiplies it by its bucket count (20 at the SDK's default boundaries), the SDK caps a metric at 2000 attribute sets and silently collapses the rest into `otel.metric.overflow`, and cumulative aggregation never evicts — so a long-lived process re-exports every session it has ever seen on every export, indefinitely. `session.id` is unchanged on spans and OTLP log events, where per-session drill-down belongs and high cardinality is acceptable. Replace the `lines_of_code.total` Gauge with a `session.lines_of_code.total` Histogram. A Gauge cannot carry a per-session dimension: with LastValue aggregation the SDK collapses every session into one attribute set and exports whichever session wrote last, so simply dropping `session.id` from the Gauge would have made it silently wrong. The histogram is recorded once when the session ends (`session.deleted`, or `session.error`), not on `session.idle`. `session.idle` fires once per *turn* while opencode's `session.diff` is cumulative for the whole session, so recording there added the running session total once per turn — a four-turn session reported its LOC four times over. This adds handling for the `session.deleted` event, which the plugin previously ignored. Fixing that also fixes the same defect in the gross counter: the per-session diff baseline is no longer discarded on every idle, so a later turn now emits the true delta instead of treating the whole session cumulative as new churn. The net values are also attached to the run and session spans as `session.total_lines_added` / `session.total_lines_removed`, on both the idle and error paths, so the information survives with traces enabled. Tests: add a cardinality guard asserting no metric data point carries `session.id` and that every handler-reachable log event still does, a non-vacuity check that fails if an instrument is missing from the test double, a multi-turn regression test for the over-count, and a test pinning the instrument names and kinds, which are the disable-metrics keys. BREAKING CHANGE: `session.id` is no longer present on any metric data point. Dashboards and alerts that group or filter metrics by `session.id` must move to traces or logs, or to the `project.id` / `model` / `agent` attributes that remain. The `opencode.lines_of_code.total` Gauge is removed and replaced by the `opencode.session.lines_of_code.total` Histogram, which exports as `opencode_session_lines_of_code_total_bucket` / `_sum` / `_count` (adjusting for `OPENCODE_METRIC_PREFIX`). Its `OPENCODE_DISABLE_METRICS` suffix is now `session.lines_of_code.total`; the old suffix `lines_of_code.total` no longer matches anything and is silently ignored. The new histogram is emitted only when a session ends, so sessions that are never deleted or errored report no line totals at all. Co-Authored-By: Claude Code <noreply@anthropic.com>
`project.id` was spread into every metric data point through `commonAttrs`. It is derived from the project directory, so it stays small for interactive use, but it has the same two properties that made `session.id` a problem: it is unbounded in CI, where every checkout can be a fresh path, and cumulative aggregation never evicts, so a long-lived process keeps re-exporting every project it has ever seen on every export. Split the shared attribute set in two. `commonAttrs` keeps `project.id` and is used by spans and log events, where per-project drill-down belongs. The new `metricAttrs` carries only the configured `OPENCODE_SPAN_ATTRIBUTES` pairs and is what every metric call site now spreads. With both identifiers off metrics, the remaining labels are bounded dimensions only: `model`, `provider`, `agent`, `agent.type`, `type`, `tool_name`, `success`, `is_subagent`. `project.id` is unchanged on spans and OTLP log events. Tests: the cardinality guard now asserts `project.id` is absent from every metric data point as well as `session.id`, and asserts the reverse for logs and spans so the split cannot silently drop the attribute from both sides. Two existing tests that pinned `project.id` onto counters are inverted to pin it off. Mutation-verified: re-injecting `project.id` into a single counter fails the guard with `instrument "commit" leaked project.id`. BREAKING CHANGE: `project.id` is no longer present on any metric data point. Dashboards and alerts that group metrics by project must move to traces or log events, where `project.id` remains, or use the `model` / `agent` attributes that stay on metrics. Cost and token attribution per project is no longer possible from metrics alone. Co-Authored-By: Claude Code <noreply@anthropic.com>
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.
Description
Two identifiers currently sit on every metric data point:
session.id(on all 15 instruments) andproject.id(spread into each one via the shared attribute set). Both are unbounded as Prometheus labels, and the failure mode is quiet:_sum+_count+_min+_max). At 8 tools × 2successvalues,tool.durationalone reaches the SDK's 2000 attribute-set ceiling after ~125 sessions, i.e. ~40,000 series.otel.metric.overflowseries, so the metric degrades with no error anywhere.project.idis derived from the project directory, so it looks bounded interactively but grows without bound in CI, where every checkout can be a fresh path.After this change the remaining metric labels are bounded dimensions only:
model,provider,agent,agent.type,type,tool_name,success,is_subagent, plus whatever the user adds viaOPENCODE_SPAN_ATTRIBUTES.Both identifiers are unchanged on spans and OTLP log events, where per-session and per-project drill-down belongs and high cardinality is acceptable.
The Gauge could not simply lose its label
lines_of_code.totalwas a Gauge carryingsession.id. Dropping the label from a Gauge does not produce a low-cardinality metric — with LastValue aggregation the SDK collapses every session into one attribute set and exports whichever session wrote last, so it would have become silently wrong rather than merely coarse.It is replaced by an
opencode.session.lines_of_code.totalHistogram. A histogram of a per-session value also cannot be recorded onsession.idle: idle fires once per turn, not once per session, while opencode'ssession.diffis cumulative for the whole session — so recording there added the running session total once per turn. A two-turn session reaching a cumulative of 25 reported_sum35 across two observations. The histogram is therefore recorded once when the session ends, via thesession.deletedevent, which the plugin previously ignored.Fixing that also fixes the same defect in the gross
lines_of_code.countcounter, which discarded its per-session diff baseline on every idle and so treated the whole session cumulative as new churn on each later turn.Net values are additionally attached to the run and session spans as
session.total_lines_added/session.total_lines_removed, on both the idle and error paths.project.idis removed by splitting the shared attribute set:commonAttrskeeps it for spans and log events, and a newmetricAttrs— the configured span attributes only — is what every metric call site now spreads. Addingproject.idexplicitly at each span/log site was the alternative, but that means editing 14 logic-heavy sites instead of 22 mechanical{ ...ctx.metricAttrs, ... }spreads.Type of change
Checklist
bun run lintpasses with no errorsbun run check:jsdoc-coveragepasses with no errorsbun run typecheckpasses with no errorsbun testpasses with no errorsRelated issues
None — no related issue was referenced in the repository.
Additional context
Verified against the real SDK, not only the unit-test doubles. Driving a full session lifecycle through a real
MeterProviderwith anInMemoryMetricExporteryields 15 instruments and 0 data points carryingsession.idorproject.id. The complete set of metric label keys is now:session.idandproject.idboth remain on the emitted log events and spans.The regression guards are mutation-verified.
tests/handlers/metric-cardinality.test.tsasserts that no metric data point carries either identifier, that every handler-reachable log event and every span still does, and — importantly — that each instrument recorded something, so the guard cannot pass by observing nothing. Injectingsession.idorproject.idback into a single counter fails it withinstrument "commit" leaked project.id. Two pre-existing tests that pinnedproject.idonto counters are inverted to pin it off.The instrument names and kinds are now pinned by a test. These names are also the
OPENCODE_DISABLE_METRICSkeys and the dashboards' contract, and had no coverage at all.createInstrumentsgained an optionalmeterparameter to make this testable — the OTel API accepts only one global provider registration per process, so a recording meter cannot be injected through the global.Known gap.
user_promptis emitted from the plugin'schat.messagehook insrc/index.ts, which needs a full plugin harness to reach. It is the oneemitLogsite not covered by the log-side guard, and the test names that exclusion explicitly rather than implying full coverage.BREAKING CHANGE
session.idandproject.idare no longer present on any metric data point. Dashboards and alerts that group or filter metrics by either must move to traces or log events, where both remain, or use themodel/agentattributes that stay on metrics.opencode.lines_of_code.totalGauge is removed and replaced by theopencode.session.lines_of_code.totalHistogram, which exports asopencode_session_lines_of_code_total_bucket/_sum/_count(adjusting forOPENCODE_METRIC_PREFIX).OPENCODE_DISABLE_METRICSsuffix is nowsession.lines_of_code.total. The old suffixlines_of_code.totalno longer matches anything and is silently ignored.🤖 Generated with Claude Code