Skip to content

fix(metrics): stop double-counting upsert-creates and counting no-ops as engram writes - #887

Open
johanneshauer wants to merge 1 commit into
scrypster:developfrom
johanneshauer:fix/engine-writes-total-accounting
Open

fix(metrics): stop double-counting upsert-creates and counting no-ops as engram writes#887
johanneshauer wants to merge 1 commit into
scrypster:developfrom
johanneshauer:fix/engine-writes-total-accounting

Conversation

@johanneshauer

Copy link
Copy Markdown
Contributor

fix(metrics): stop double-counting upsert-creates and counting no-ops as engram writes

muninndb_engine_writes_total is documented as "Total number of engrams written".
Two paths break that, both introduced together in #556 when the UPSERT write mode
was added — the new branches were instrumented without noticing that the path they
delegate to already instruments itself.

1. Upsert-create is counted twice

upsertCreate does not write anything itself. It delegates:

resp, err := e.Write(withSkipContentDedup(ctx), &createReq)

Write then increments EngineWritesTotal, records the latency sample and observes
WriteDuration. upsertCreate repeats all three at the end of the same call, so
one upsert-create produces two increments, two latency samples and two duration
observations for a single write.

Fixed by removing the duplicated block; the delegated Write remains the single
point of instrumentation for that write.

2. The identical-content no-op is counted as a write

The branch whose own comment reads:

Identical content: no-op. Cheap fast path — no write, no evolve, no index churn.

increments EngineWritesTotal before returning the existing id. No engram is
written, so the counter moves for something that by its own description is not a
write.

Fixed by removing only the increment. The latency observations there are kept
deliberately
— a call was served, and that is the only place this fast path is
measured. The two changes differ for that reason: at upsertCreate the latency data
is a duplicate of the same work, whereas on the no-op path it is the sole
observation of a distinct operation.

What is deliberately left alone

  • The upsert-evolve branch keeps its increment. evolveAtInternal does not
    increment, so that site is the only counter for an evolve. A test guards it.
  • RememberTree still does not count. It builds nodes with
    e.store.NewBatch() + batch.WriteEngram(...) and never routes through
    Write/WriteBatch, so tree nodes are absent from this counter entirely. Out of
    scope here, but worth knowing — the help text reads as though it covers them.
    Happy to fold them in, or narrow the help text, in a follow-up if you have a
    preference.

If you disagree on the no-op

Counting a served no-op as request volume is a defensible thing to want; it is
just not what this counter's name and help text promise. If you would rather keep
that visibility, say so and I will drop that hunk and add a separate
muninndb_engine_upsert_noop_total instead — the double-count fix stands on its own
either way.

Tests

internal/engine/writes_total_accounting_test.go, five tests, all deltas (the
counter is process-global):

  • plain write → 1
  • upsert-create → 1 — RED-checked: reverting the fix reports delta = 2
  • upsert no-op → 0 — RED-checked: reverting the fix reports delta = 1
  • upsert-evolve → 1 (guards that the fix did not silence the evolve path)
  • 3-engram batch → 3

Verified against develop at 34b505f: gofmt clean, go vet clean, full module
go test ./... 51 ok / 0 FAIL, go test -race ./internal/engine/ clean with 0
data races. The three non-bug tests pass both before and after the fix, which is
what shows the change is narrow.

… as engram writes

muninndb_engine_writes_total is documented as "Total number of engrams
written". Two paths broke that, both introduced together in scrypster#556 when the
UPSERT write mode was added: the new branches were instrumented without
noticing that the path they delegate to already instruments itself.

upsertCreate does not write anything itself -- it delegates to e.Write(),
which increments EngineWritesTotal, records the latency sample and observes
WriteDuration. upsertCreate repeated all three, so one upsert-create produced
two increments, two latency samples and two duration observations for a
single write. The duplicated block is removed; the delegated Write remains
the single point of instrumentation.

The identical-content branch is a documented no-op ("no write, no evolve, no
index churn") yet incremented the counter before returning the existing id.
Only the increment is removed there -- the latency observations are kept
deliberately, because a call was served and that is the only place this fast
path is measured. The two hunks differ for that reason: at upsertCreate the
latency data duplicates the same work, whereas on the no-op path it is the
sole observation of a distinct operation.

The upsert-evolve branch keeps its increment: evolveAtInternal does not
increment, so that site is the only counter for an evolve. A test guards it.

Tests: five delta-based tests over the write paths. Two of them fail against
the unfixed code (delta = 2 for upsert-create, delta = 1 for the no-op); the
other three pass both before and after, which shows the change is narrow.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@johanneshauer

Copy link
Copy Markdown
Contributor Author

CI note: the one red job (Vulnerability scan) is pre-existing on develop and not from this change — #885 documents that develop @ 34b505f and every open PR against it fail that job on the 6 reachable stdlib advisories against the pinned go 1.26.5, and bumps the directive to clear it. Every other job is green here (Go build & test, Windows, CLI integration, Playwright E2E, Python + Node SDK, API spec, shellcheck, web console). This branch will go fully green once #885 lands; happy to rebase then if you would rather see it clean before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant