fix(metrics): make Prometheus metrics correct with multiple workers, and wire the trackers up by wanghy73 · Pull Request #1523 · MemMachine/MemMachine

wanghy73 · 2026-08-25T22:58:54Z

Two fixes to metrics that are wrong in ways nothing reports. Both were found while
trying to attribute request latency on a capacity run and discovering the numbers
could not be trusted.

1. Metrics are not aggregated across uvicorn workers

/metrics called bare generate_latest(). With MEMMACHINE_WORKERS > 1 uvicorn
forks a worker per process and each keeps its own registry, so a scrape returned
whichever worker happened to answer — about 1/N of the traffic, a different 1/N
each time. Consecutive scrapes look like counter resets, and every rate, histogram
quantile and calls-per-request figure derived from them is wrong in an unbounded
direction.

Build a fresh registry per scrape and attach MultiProcessCollector when
PROMETHEUS_MULTIPROC_DIR is set. The single-worker path keeps the default
registry, so nothing changes for the default deployment.

Measured, on an 8-tenant capacity run over one rate ladder:

workers	counter resets before	after
4	70	0
8	87	0

Five consecutive scrapes of a loaded server returned identical values where they
previously diverged.

This needs PROMETHEUS_MULTIPROC_DIR set and a writable directory mounted at it.
The chart change that does so is separate.

2. OperationTracker silently discards timings when given no factory

OperationTracker accepts metrics_factory=None and then throws away every
timing it takes — no error, no warning, no series. A component that is fully
instrumented but never handed a factory is indistinguishable from one that was
never instrumented at all, and the only way to notice is to go looking for a
metric that should exist and find nothing.

That is not hypothetical: the Neo4j store, the episode store and the session
store each shipped instrumented and unwired, which is why database latency
appeared to be unmeasurable. Every call was being timed and discarded. This wires
the factory through the resource manager, the database manager and the event
backend's params.

On the tests

test_metrics_factory_wiring.py asserts at the call sites, not on the
components. An earlier version tested that each store honours a factory it is
given — which passes whether or not anything passes one, and still passed with
the fix reverted. The tests here fail when the wiring is removed.

Verification

All 9 changed Python files parse; the new test module was written against the
running server.
Deployed to a test cluster as memmachine/memmachine:instrumented-mp and
exercised with 1, 4 and 8 workers under load.

The two changes are kept as separate commits so either can be reverted alone.

edwinyyyu · 2026-08-26T00:02:46Z

Please pass ruff at least. ty is fixed here: #1519

wanghy73 · 2026-08-26T04:32:45Z

Thanks — ruff passes now. Three things were wrong: typing.Any in the _timed wrappers I added (ANN401), import sorting, and I'd missed that the job also runs ruff format --check.

Fixed with Concatenate plus a Protocol-bound TypeVar rather than a noqa, so the annotations stay meaningful and ty stays at its 17 pre-existing diagnostics — the naive ParamSpec version pushed it to 19, since an unbound type variable can't carry _tracker.

Confirmed those 17 are all the nebulagraph_python.client has no member NebulaAsyncClient import, reproduced identically on upstream/speedkick, so #1519 should clear them. speedkick and main are the same commit right now, so it applies to this PR's base directly once merged.

Both commits pass ruff check and ruff format --check independently, so either can still be reverted on its own.

malatewang · 2026-08-26T18:14:59Z

+    # answered the scrape - about 1/N of the traffic, a different 1/N each time.
+    # Consecutive scrapes then look like counter resets and every rate or
+    # histogram derived from them is wrong.
+    multiproc_dir = os.environ.get("PROMETHEUS_MULTIPROC_DIR")


Who should take care of the directory setup?

Good catch — the answer was "nobody", which is a real gap. Fixed in 725e1de.

The /metrics handler is also the wrong place to look for it: prometheus_client mmaps a file per worker when the first metric is registered, long before any scrape arrives. Creating the directory here would already be too late.

So main() now calls _prepare_multiproc_dir() before any metric exists. It creates the directory and clears stale *.db files — without that, a previous run's dead workers get summed into the live counters on every scrape. It's best-effort: a read-only or pre-seeded mount logs a warning rather than refusing to start, since failing to boot over metrics would be the wrong trade.

Verified it creates missing nested directories, clears stale files, and warns without raising on an unwritable path.

malatewang · 2026-08-26T18:16:40Z

+_R = TypeVar("_R")
+
+
+class _HasTracker(Protocol):


The same code duplicates. We can make it common

Agreed, and it was worse than the diff showed — fixed in 00a7e81.

A later push to satisfy ruff's ANN401 replaced the Any annotations with a Protocol-bound TypeVar, and I copied that into both files too. So by the time you looked it was the decorator plus a Protocol and three TypeVars duplicated — about 30 identical lines in two places.

Now extracted to common/metrics_factory/timed.py, next to OperationTracker, and exported as timed. Both stores import the one copy; the only thing that differed between them was the docstring.

With MEMMACHINE_WORKERS > 1 uvicorn forks a worker per process and each keeps its own registry, so /metrics returned whichever worker happened to answer the scrape - about 1/N of the traffic, a different 1/N each time. Consecutive scrapes then look like counter resets, and every rate, histogram quantile and calls-per-request figure derived from them is wrong in an unbounded direction. Build a fresh registry per scrape and attach MultiProcessCollector when PROMETHEUS_MULTIPROC_DIR is set, leaving the single-worker path on the default registry so nothing changes for the default deployment. Measured on an 8-tenant capacity run: counter resets over one ladder went from 70 (4 workers) and 87 (8 workers) to 0, and five consecutive scrapes of a loaded server returned identical values where they previously diverged. Requires PROMETHEUS_MULTIPROC_DIR to be set and a writable directory mounted at it; the chart change that does so is separate.

OperationTracker accepts metrics_factory=None and then silently discards every timing it takes - no error, no warning, no series. A component that is fully instrumented but never given a factory is therefore indistinguishable from one that was never instrumented at all, and the only way to notice is to go looking for a metric that should exist. The Neo4j store, the episode store and the session store each shipped instrumented and unwired, which is why database latency appeared to be unmeasurable: every call was timed and thrown away. Wire the factory through the resource manager, the database manager and the event backend's params so those trackers emit. The tests assert at the call sites rather than on the components. An earlier version tested that each store honours a factory it is given, which passes whether or not anything passes one - it still passed with the fix reverted. These fail when the wiring is removed.

malatewang · 2026-08-26T20:23:19Z

+    Deliberately best-effort. A deployment may mount the directory read-only or
+    pre-seed it, and refusing to start over metrics would be the wrong trade.
+    """
+    path = os.environ.get("PROMETHEUS_MULTIPROC_DIR")


It is better to check the worker number. If the worker number is bigger than 1, setup multiproc dir automatically.

Agreed — done in b89ee3e.

The worker count is the thing that decides whether aggregation is needed, so it now drives the setup: above 1 with nothing configured, the directory defaults to <tempdir>/memmachine-prometheus-multiproc. An explicit PROMETHEUS_MULTIPROC_DIR still wins, which is how two servers on one host keep their metrics apart.

Two details worth flagging:

If the chosen directory can't be created, the variable is removed again. prometheus_client raises in every worker if it can't open the directory it's pointed at, so leaving behind a path only this code picked would turn an unwritable temp dir into a server that refuses to start — the opposite of the best-effort behaviour the rest of the function promises. An operator's own unwritable path still just warns.

Worker-count parsing moved into _worker_count(), shared with start_http(), so the two can't disagree about the number.

Setting the env var in main() reaches the workers because uvicorn spawns them via multiprocessing.get_context("spawn") (uvicorn/_subprocess.py) — each child is a fresh interpreter that inherits os.environ and imports prometheus_client after we've set it.

13 tests added in test_multiproc_dir.py: the default, the explicit override at both 1 and 4 workers, nested creation, stale *.db clearing, and both failure paths.

PROMETHEUS_MULTIPROC_DIR had to be set by whoever deployed the server. Set MEMMACHINE_WORKERS=4 without it and nothing complains: each worker keeps its own registry, a scrape is answered by whichever worker the load balancer picked, and the numbers that come back look plausible. There is no error to notice, so the only way to find out is to compare a counter against a request count and see it come up short by a factor of the worker count. The worker count is the thing that decides whether aggregation is needed, so read it here and default the directory when it is above 1. An explicit setting still wins, which is how two servers on one host keep their metrics apart. If the chosen directory cannot be created the variable is removed again - prometheus_client raises in every worker if it cannot open the directory it is pointed at, and taking the server down over metrics would be the wrong trade. Worker-count parsing moves into _worker_count() so start_http() and the directory setup cannot disagree about it. Verified: uvicorn spawns workers via multiprocessing.get_context("spawn"), so children re-import in a fresh interpreter and inherit the variable set here. 13 new tests cover the default, the explicit override, nested creation, stale file clearing, and both failure paths; 1228 server/common tests pass; ruff check, ruff format --check and ty are clean (17 ty diagnostics, all pre-existing and none in these files). Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Nr9kacmpFVTTfkZRw6esxP

`main` runs `ty check` over the server package, which `speedkick` did not when MemMachine#1523 merged, and it rejects the stub this test installed: manager.get_sql_engine = fake_engine error[invalid-assignment]: Object of type `def fake_engine(_name) -> ...` is not assignable to attribute `get_sql_engine` of type `def get_sql_engine(self, name: str, validate: bool = False) -> ...` Patch the class with monkeypatch instead of assigning to the instance, and give the stub the real method's signature, matching how this test already replaces `get_metrics_factory`. The `cast` is needed because the return is a MagicMock. Test-only; the wiring under test is untouched. Confirmed the test still fails when the segment-store wiring is reverted, so it keeps its teeth. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

…1597) Port of MemMachine#1597 to main: the tree of feat/event-memory-handoff-speedkick at 80f0c71 applied onto port/chain-main4 at e7e4d54, the commit that closes MemMachine#1606's port. The one adaptation is in LongTermMemory's construction of EventMemoryParams: main's EventBackendParams carries no metrics factory (MemMachine#1523 is not ported) and MemMachine#1597 removed the reranker from the memory, so neither is passed. MemMachine#1597 keeps its review history on speedkick; this commit is the same content, squashed. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01YBbQgZiCqeoLu83EkbEFHE

…and wire the trackers up (MemMachine#1523) * fix(metrics): aggregate Prometheus metrics across uvicorn workers With MEMMACHINE_WORKERS > 1 uvicorn forks a worker per process and each keeps its own registry, so /metrics returned whichever worker happened to answer the scrape - about 1/N of the traffic, a different 1/N each time. Consecutive scrapes then look like counter resets, and every rate, histogram quantile and calls-per-request figure derived from them is wrong in an unbounded direction. Build a fresh registry per scrape and attach MultiProcessCollector when PROMETHEUS_MULTIPROC_DIR is set, leaving the single-worker path on the default registry so nothing changes for the default deployment. Measured on an 8-tenant capacity run: counter resets over one ladder went from 70 (4 workers) and 87 (8 workers) to 0, and five consecutive scrapes of a loaded server returned identical values where they previously diverged. Requires PROMETHEUS_MULTIPROC_DIR to be set and a writable directory mounted at it; the chart change that does so is separate. * fix(metrics): hand every OperationTracker a metrics factory OperationTracker accepts metrics_factory=None and then silently discards every timing it takes - no error, no warning, no series. A component that is fully instrumented but never given a factory is therefore indistinguishable from one that was never instrumented at all, and the only way to notice is to go looking for a metric that should exist. The Neo4j store, the episode store and the session store each shipped instrumented and unwired, which is why database latency appeared to be unmeasurable: every call was timed and thrown away. Wire the factory through the resource manager, the database manager and the event backend's params so those trackers emit. The tests assert at the call sites rather than on the components. An earlier version tested that each store honours a factory it is given, which passes whether or not anything passes one - it still passed with the fix reverted. These fail when the wiring is removed. * fix(metrics): choose a multiprocess directory when workers > 1 PROMETHEUS_MULTIPROC_DIR had to be set by whoever deployed the server. Set MEMMACHINE_WORKERS=4 without it and nothing complains: each worker keeps its own registry, a scrape is answered by whichever worker the load balancer picked, and the numbers that come back look plausible. There is no error to notice, so the only way to find out is to compare a counter against a request count and see it come up short by a factor of the worker count. The worker count is the thing that decides whether aggregation is needed, so read it here and default the directory when it is above 1. An explicit setting still wins, which is how two servers on one host keep their metrics apart. If the chosen directory cannot be created the variable is removed again - prometheus_client raises in every worker if it cannot open the directory it is pointed at, and taking the server down over metrics would be the wrong trade. Worker-count parsing moves into _worker_count() so start_http() and the directory setup cannot disagree about it. Verified: uvicorn spawns workers via multiprocessing.get_context("spawn"), so children re-import in a fresh interpreter and inherit the variable set here. 13 new tests cover the default, the explicit override, nested creation, stale file clearing, and both failure paths; 1228 server/common tests pass; ruff check, ruff format --check and ty are clean (17 ty diagnostics, all pre-existing and none in these files). Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Nr9kacmpFVTTfkZRw6esxP --------- Co-authored-by: Claude Opus 5 <[email protected]> (cherry picked from commit ace7752) Signed-off-by: Haiyan Wang <[email protected]>

`main` runs `ty check` over the server package, which `speedkick` did not when MemMachine#1523 merged, and it rejects the stub this test installed: manager.get_sql_engine = fake_engine error[invalid-assignment]: Object of type `def fake_engine(_name) -> ...` is not assignable to attribute `get_sql_engine` of type `def get_sql_engine(self, name: str, validate: bool = False) -> ...` Patch the class with monkeypatch instead of assigning to the instance, and give the stub the real method's signature, matching how this test already replaces `get_metrics_factory`. The `cast` is needed because the return is a MagicMock. Test-only; the wiring under test is untouched. Confirmed the test still fails when the segment-store wiring is reverted, so it keeps its teeth. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Haiyan Wang <[email protected]>

…and wire the trackers up (port of #1523 to main) (#1678) * fix(metrics): make Prometheus metrics correct with multiple workers, and wire the trackers up (#1523) * fix(metrics): aggregate Prometheus metrics across uvicorn workers With MEMMACHINE_WORKERS > 1 uvicorn forks a worker per process and each keeps its own registry, so /metrics returned whichever worker happened to answer the scrape - about 1/N of the traffic, a different 1/N each time. Consecutive scrapes then look like counter resets, and every rate, histogram quantile and calls-per-request figure derived from them is wrong in an unbounded direction. Build a fresh registry per scrape and attach MultiProcessCollector when PROMETHEUS_MULTIPROC_DIR is set, leaving the single-worker path on the default registry so nothing changes for the default deployment. Measured on an 8-tenant capacity run: counter resets over one ladder went from 70 (4 workers) and 87 (8 workers) to 0, and five consecutive scrapes of a loaded server returned identical values where they previously diverged. Requires PROMETHEUS_MULTIPROC_DIR to be set and a writable directory mounted at it; the chart change that does so is separate. * fix(metrics): hand every OperationTracker a metrics factory OperationTracker accepts metrics_factory=None and then silently discards every timing it takes - no error, no warning, no series. A component that is fully instrumented but never given a factory is therefore indistinguishable from one that was never instrumented at all, and the only way to notice is to go looking for a metric that should exist. The Neo4j store, the episode store and the session store each shipped instrumented and unwired, which is why database latency appeared to be unmeasurable: every call was timed and thrown away. Wire the factory through the resource manager, the database manager and the event backend's params so those trackers emit. The tests assert at the call sites rather than on the components. An earlier version tested that each store honours a factory it is given, which passes whether or not anything passes one - it still passed with the fix reverted. These fail when the wiring is removed. * fix(metrics): choose a multiprocess directory when workers > 1 PROMETHEUS_MULTIPROC_DIR had to be set by whoever deployed the server. Set MEMMACHINE_WORKERS=4 without it and nothing complains: each worker keeps its own registry, a scrape is answered by whichever worker the load balancer picked, and the numbers that come back look plausible. There is no error to notice, so the only way to find out is to compare a counter against a request count and see it come up short by a factor of the worker count. The worker count is the thing that decides whether aggregation is needed, so read it here and default the directory when it is above 1. An explicit setting still wins, which is how two servers on one host keep their metrics apart. If the chosen directory cannot be created the variable is removed again - prometheus_client raises in every worker if it cannot open the directory it is pointed at, and taking the server down over metrics would be the wrong trade. Worker-count parsing moves into _worker_count() so start_http() and the directory setup cannot disagree about it. Verified: uvicorn spawns workers via multiprocessing.get_context("spawn"), so children re-import in a fresh interpreter and inherit the variable set here. 13 new tests cover the default, the explicit override, nested creation, stale file clearing, and both failure paths; 1228 server/common tests pass; ruff check, ruff format --check and ty are clean (17 ty diagnostics, all pre-existing and none in these files). Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Nr9kacmpFVTTfkZRw6esxP --------- Co-authored-by: Claude Opus 5 <[email protected]> (cherry picked from commit ace7752) Signed-off-by: Haiyan Wang <[email protected]> * test(metrics): patch get_sql_engine on the class so ty accepts the stub `main` runs `ty check` over the server package, which `speedkick` did not when #1523 merged, and it rejects the stub this test installed: manager.get_sql_engine = fake_engine error[invalid-assignment]: Object of type `def fake_engine(_name) -> ...` is not assignable to attribute `get_sql_engine` of type `def get_sql_engine(self, name: str, validate: bool = False) -> ...` Patch the class with monkeypatch instead of assigning to the instance, and give the stub the real method's signature, matching how this test already replaces `get_metrics_factory`. The `cast` is needed because the return is a MagicMock. Test-only; the wiring under test is untouched. Confirmed the test still fails when the segment-store wiring is reverted, so it keeps its teeth. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Signed-off-by: Haiyan Wang <[email protected]> * docs(metrics): record why the worker-count check is nested Review follow-up on #1678. Shu asked whether the worker-count check should move out of the `if not path` block, since a single worker has no need of the directory. It cannot: when PROMETHEUS_MULTIPROC_DIR is set by the operator, prometheus_client switches to its multiprocess value class on the strength of the environment variable alone and mmaps a file into the directory on the first metric, whatever the worker count. Hoisting the check would skip the mkdir for a one-worker deployment that sets the variable, and the first metric would raise FileNotFoundError deep in the worker - the failure the docstring already warns about. Verified against prometheus_client directly: with the variable pointing at a missing directory, a single-process Counter().inc() raises FileNotFoundError on <dir>/counter_<pid>.db. test_an_explicit_directory_is_honoured_for_one_worker already pins this, so the comment records the reasoning rather than changing behaviour. Signed-off-by: Haiyan Wang <[email protected]> --------- Signed-off-by: Haiyan Wang <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]>

…1597) Port of MemMachine#1597 to main: the tree of feat/event-memory-handoff-speedkick at 80f0c71 applied onto port/chain-main4 at e7e4d54, the commit that closes MemMachine#1606's port. The one adaptation is in LongTermMemory's construction of EventMemoryParams: main's EventBackendParams carries no metrics factory (MemMachine#1523 is not ported) and MemMachine#1597 removed the reranker from the memory, so neither is passed. MemMachine#1597 keeps its review history on speedkick; this commit is the same content, squashed. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_01YBbQgZiCqeoLu83EkbEFHE

wanghy73 requested review from edwinyyyu, leomem and malatewang August 25, 2026 23:10

wanghy73 force-pushed the fix/metrics-correctness branch 2 times, most recently from 00a4a41 to a83cd21 Compare August 26, 2026 04:18

malatewang reviewed Aug 26, 2026

View reviewed changes

wanghy73 added 2 commits August 26, 2026 19:12

wanghy73 force-pushed the fix/metrics-correctness branch from a83cd21 to 00a7e81 Compare August 26, 2026 19:13

malatewang reviewed Aug 26, 2026

View reviewed changes

malatewang approved these changes Aug 26, 2026

View reviewed changes

wanghy73 merged commit ace7752 into MemMachine:speedkick Aug 26, 2026
35 of 39 checks passed

wanghy73 deleted the fix/metrics-correctness branch August 26, 2026 21:43

wanghy73 mentioned this pull request Aug 27, 2026

fix(metrics): wire the Qdrant vector store, and let a build include it #1532

Merged

This was referenced Sep 16, 2026

[speedkick port 2/17] Make create session idempotent (port of #1539) #1660

Closed

[user properties 2/2] Remove per-project filterable properties (port of #1606) #1670

Open

wanghy73 mentioned this pull request Sep 17, 2026

fix(metrics): make Prometheus metrics correct with multiple workers, and wire the trackers up (port of #1523 to main) #1678

Merged

edwinyyyu mentioned this pull request Sep 17, 2026

[event memory 1/7] Add time bounds, sources and expansion to EventMemory (port of #1597, without session) #1684

Draft

This was referenced Sep 17, 2026

Hand the Qdrant vector store a metrics factory, so its tracker emits (port of #1532) #1682

Merged

Overhaul segment store: shared tables with incarnation-scoped tenant keys (port of #1548) #1661

Merged

edwinyyyu mentioned this pull request Sep 22, 2026

Resolve the metrics factory once and hand it to every component #1709

Open

Navigation Menu

Repository navigation

fix(metrics): make Prometheus metrics correct with multiple workers, and wire the trackers up - #1523

fix(metrics): make Prometheus metrics correct with multiple workers, and wire the trackers up#1523
wanghy73 merged 3 commits into
MemMachine:speedkickfrom
wanghy73:fix/metrics-correctness

wanghy73 commented Aug 25, 2026 •

edited

Loading

Uh oh!

edwinyyyu commented Aug 26, 2026

Uh oh!

wanghy73 commented Aug 26, 2026

Uh oh!

malatewang Aug 26, 2026

Uh oh!

wanghy73 Aug 26, 2026

Uh oh!

malatewang Aug 26, 2026

Uh oh!

wanghy73 Aug 26, 2026

Uh oh!

malatewang Aug 26, 2026

Uh oh!

wanghy73 Aug 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

wanghy73 commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Metrics are not aggregated across uvicorn workers

2. OperationTracker silently discards timings when given no factory

On the tests

Verification

Uh oh!

edwinyyyu commented Aug 26, 2026

Uh oh!

wanghy73 commented Aug 26, 2026

Uh oh!

malatewang Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

wanghy73 Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

malatewang Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

wanghy73 Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

malatewang Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

wanghy73 Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wanghy73 commented Aug 25, 2026 •

edited

Loading