test(metrics): assert the two usage aggregates agree - #167
Open
lizhuojunx86 wants to merge 1 commit into
Open
Conversation
/metrics/summary reports the same total twice, computed two ways. Since bdd164a they are meant to agree and nothing checked that they do. Adds a unit test over a hand-built log and an integration test that drives DefaultStrategy with a mock model, so both the read path and the two write paths are covered. Refs sandbaseai#77
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.
What changed
Two test files, no source changes.
/metrics/summaryreturns the same total twice, computed two ways (runtime.ts:137-149):Since
bdd164a6dthose are supposed to be equal. Nothing checked that they are, so drift on either path ships silently. These tests check it.tests/unit/usage-accounting.test.tsbuilds a log by hand and asserts the identity through the real route, which is mounted on a database alone because the/metrics/summaryhandler only reaches fordeps.db. Seven cases, 0.6s. Three of them assert the identity fails where it should, so the check has teeth rather than passing by construction.tests/integration/usage-accounting.test.tsrunsDefaultStrategyagainst a mock model reporting known usage, following the pattern instrategy-tool-schema.test.ts. Four cases, 1.9s. This one covers the write paths atdefault-strategy.ts:190and:199rather than simulating them.No ground truth, no vendor account, no network.
Why the identity is worth pinning
It catches drift on either of the two paths the fix created, and it costs nothing to run. I checked that it catches both by breaking them:
WHERE type = 'span.model_request_end'fromeventUsageexpected 360 to be 120eventLog.recordUsage(...)atdefault-strategy.ts:199expected 120 to be +0The first failure message names the double count directly, which is the behaviour the filter was added to prevent.
It also makes one existing silent failure visible.
recordUsageis a bareUPDATE ... WHERE id = ?and the changes count fromrun()is not checked, so a request against a session row that isn't there increments nothing and raises nothing. The current test pins that behaviour as not-throwing (event-logger.test.ts:120-122); with the identity in place the divergence it produces is caught instead of absorbed. That case is in the unit file.What these tests do not claim
The identity is necessary, not sufficient. Both aggregates can be equally wrong and it still holds. Two known cases, both of which you flagged as deferred in #77:
context-compactor.ts:77destructuresconst { text } = await generateText(...), sousageis never bound.outcome-evaluator.ts:58keeps the whole response and reads onlyresponse.text(:70). Neither path writes usage anywhere, so the two sides agree while both are short by the same amount.InMemoryEventLog.recordUsageis a no-op (in-memory-event-log.ts:43-45) and the class writes nothing to the database, so delegated runs miss both columns equally. The comment there already says this is intended for ephemeral delegation, which answers the question I left open in Usage accounting: one model request lands on 2..N+2 events, and sessions.usage_tokens_* is never written #77. Treat that one as withdrawn.Neither is in scope here. I mention them so the green check isn't read as covering more than it does.
Validation
npm run typechecknpm testnpm run buildRun against
a634eb431on Node 22.23.2 (82 files, 623 passed, 15 skipped) and onNode 26.5.1 (82 files, 624 passed, 14 skipped). The skip counts differ because the
Docker and Kubernetes sandbox suites skip on backend availability, not because of
anything here.
Offered, not included
If you would rather the SQL live in one place than be re-stated in the tests, I can extract the two aggregates into a small helper that
runtime.tsand both tests call. I left it out to keep this focused, and because it touches code you just wrote. Say the word and I'll push it here.Refs #77