Skip to content

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

Merged
edwinyyyu merged 3 commits into
MemMachine:mainfrom
edwinyyyu:port/qdrant-metrics-wiring-main
Sep 24, 2026
Merged

Hand the Qdrant vector store a metrics factory, so its tracker emits (port of #1532)#1682
edwinyyyu merged 3 commits into
MemMachine:mainfrom
edwinyyyu:port/qdrant-metrics-wiring-main

Conversation

@edwinyyyu

@edwinyyyu edwinyyyu commented Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Port

Copy of #1532, merged into speedkick as b6c90ab40, on main: the same commit cherry-picked, keeping its author, wanghy73, with two differences: the Dockerfile change (an EXTRAS build arg) is left out as unrelated to the wiring, and the metrics_factory_id key is added to the database configuration table in the docs. A second commit rewords that row to name Neo4j alongside Qdrant, since Neo4jConf takes the same key on main (#1678) and has no row of its own. The other three stores #1532 names, Neo4j, episode and session, were wired on main by #1678 (the port of #1523); the Qdrant store is the one left.

Purpose of the change

OperationTracker accepts factory=None and then discards every timing without an error, a warning, or a series. A component that is fully instrumented but never handed a factory looks identical to one that was never instrumented; the only way to notice is to go looking for a metric that should exist and find nothing.

That is what QdrantVectorStore was doing: QdrantVectorStoreParams.metrics_factory defaults to None and nothing set it. QdrantConf gains MetricsFactoryIdMixin so it can resolve one, and database_manager passes it through.

test_qdrant_creates_vector_store pinned the exact QdrantVectorStoreParams kwargs and had to change. It now asserts metrics_factory is not None rather than pinning a value: passing the keyword is not the property worth guarding, since None is accepted and silently discards everything. Removing the wiring fails it.

Stack

23 PRs on main: 5 independent ones, and three consecutive stages numbered on their own: the vector store scale-out, the SQLite store fixes and the vector store contract changes. A stacked PR's diff on GitHub is cumulative until the PRs under it merge.

Independent PRs, each directly on main; review and merge in any order:

# PR change
— #1541 fix(event-backend): make expand_context return timeline-neighbor episodes
— #1661 Overhaul segment store: shared tables with incarnation-scoped tenant keys (port of #1548)
— #1630 Bound every request to a remote vector store by a configured timeout
— #1682 (this PR) Hand the Qdrant vector store a metrics factory, so its tracker emits (port of #1532)
— #1624 Make no memory request create a project

The vector store stages, consecutive: each stacked on the one below. [vector store scale-out 1/5] is directly on main and 2/5 on it alone; they do not depend on the independent PRs. From 3/5 up, the chain's history also carries the independent PRs' commits beneath it, since the later stages were written on top of them ([vector store contract 2/6] on #1624's session policy, for one); so a merge of an independent PR rebases the chain without conflict, and until they merge those PRs' changes show in a stacked PR's diff.

# PR change
Stage 1, the vector store scale-out: what horizontal scalability without sharding requires.
[vector store scale-out 1/5] #1671 Remove custom sharding from the Qdrant store (port of #1654)
[vector store scale-out 2/5] #1631 Mint an incarnation per collection life in a SQL-arbitrated registry, so any process may serve any Qdrant or Milvus collection
[vector store scale-out 3/5] #1670 Remove per-project filterable properties (port of #1606)
[vector store scale-out 4/5] #1702 Keep user properties out of the vector store
[vector store scale-out 5/5] #1627 Make a vector store one collection, with string-keyed partitions
Stage 2, the SQLite store fixes, on [vector store scale-out 5/5]: re-derived on the one-collection store.
[sqlite store fixes 1/7] #1460 Publish vector index files atomically (but not durably) (as merged into speedkick, #1588)
[sqlite store fixes 2/7] #1469 Never reuse a row id in SQLiteVectorStore (as merged into speedkick, #1589)
[sqlite store fixes 3/7] #1672 Own the search engine's concurrency in the store, not in each engine (port of #1612)
[sqlite store fixes 4/7] #1673 Serialize a partition's writes so the engine sees them in order (port of #1607)
[sqlite store fixes 5/7] #1674 Refuse a pending row replay cannot honor, instead of dropping it (port of #1608)
[sqlite store fixes 6/7] #1675 Take SQLite's write lock at BEGIN, not at the first write (port of #1609)
[sqlite store fixes 7/7] #1676 Give every write a fresh row id, so a key names one version (port of #1610)
Stage 3, the vector store contract changes, on [sqlite store fixes 7/7].
[vector store contract 1/6] #1663 Answer with cosine scores and uuids, not vectors and stale properties, and require a vector on the record (port of #1598 and #1603)
[vector store contract 2/6] #1622 Create a session's storage with the session, never on a request
[vector store contract 3/6] #1625 Remove open-or-create and close from both stores
[vector store contract 4/6] #1618 Let a deployment tune a Qdrant collection's HNSW, optimizers and quantization
[vector store contract 5/6] #1628 Make a vector store filter only on the properties it declares
[vector store contract 6/6] #1616 Close the filter union, and make negation the complement on every backend

This PR is two commits, f51702f15 (the port) and 1a0dc145a (the docs-row wording), directly on main, independent of the others: review and merge it in any order among the independent PRs (a later one touching the same file rebases). The stages' history carries the same change from [vector store scale-out 3/5] up, so their diffs include it until it merges.

Verification

At f51702f15, on 2026-09-21: ruff check and ruff format --check clean; ty check clean as CI runs it (uv run --frozen --all-extras ty check --project packages/server); the full server suite without integration tests passes (pytest packages/server/server_tests -m "not integration"), 1914 tests at f51702f15. 1a0dc145a changes one line of docs/open_source/configuration.mdx and nothing else.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ESpWYTmCR7X3bJEpoA8SAn

This was referenced Sep 17, 2026
Merged
Merged
Draft
Open
Merged
Closed
Draft
Draft
Draft
Closed
Draft
Closed
Draft
Open
Open
Draft
Draft
Draft
Draft
Draft
@edwinyyyu
edwinyyyu force-pushed the port/qdrant-metrics-wiring-main branch from 9f5458f to 4f0c836 Compare September 17, 2026 19:32
@edwinyyyu edwinyyyu changed the title [vector store 14/16] Hand the Qdrant vector store a metrics factory, so its tracker emits (port of #1532) Hand the Qdrant vector store a metrics factory, so its tracker emits (port of #1532) Sep 17, 2026
edwinyyyu
edwinyyyu commented Sep 17, 2026
View reviewed changes

@edwinyyyu edwinyyyu left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The commit trailer "(cherry picked from commit b6c90ab4070e2a5f3fb1d6c4fa2b7a7a4fd2e4ad)" names a sha that exists neither locally nor on GitHub; #1532's merge commit is b6c90ab (same 9-character prefix).
  2. The port also adds MetricsFactoryIdMixin to database_conf.py's import block (speedkick had it via #1523); the adaptation list omits it.

@edwinyyyu
edwinyyyu force-pushed the port/qdrant-metrics-wiring-main branch 2 times, most recently from a20d096 to c85f3ff Compare September 18, 2026 19:39
This was referenced Sep 18, 2026
Draft
Draft
@wanghy73 @claude @edwinyyyu
fix(metrics): wire the Qdrant vector store (speedkick) (MemMachine#1532)
f51702f 
QdrantVectorStore was the fourth component built without a metrics factory.
OperationTracker accepts metrics_factory=None and then discards every timing
without an error, so the store looked instrumented and emitted nothing - the
same defect as the Neo4j store, the episode store and the session store, which
is why no Qdrant latency was observable.

QdrantConf gains MetricsFactoryIdMixin so it can resolve one, and
database_manager passes it through.

test_qdrant_creates_vector_store pinned the exact params and had to change.
It now asserts metrics_factory is not None rather than pinning it: passing the
keyword is not the property worth guarding, since None is accepted and silently
discards everything. Removing the wiring fails it.

Ported to main without MemMachine#1532's Dockerfile change (the EXTRAS build arg),
which is unrelated to the wiring; the `metrics_factory_id` key is added to
the database configuration table in the docs.

(cherry picked from commit b6c90ab)

Claude-Session: https://claude.ai/code/session_01Nr9kacmpFVTTfkZRw6esxP

Co-authored-by: Claude Opus 5 <[email protected]>
@edwinyyyu
edwinyyyu force-pushed the port/qdrant-metrics-wiring-main branch from c85f3ff to f51702f Compare September 21, 2026 18:11
@edwinyyyu edwinyyyu mentioned this pull request Sep 21, 2026
Open
@edwinyyyu
edwinyyyu marked this pull request as ready for review September 21, 2026 22:20
@edwinyyyu @claude
docs(config): name Neo4j on the metrics_factory_id row, as it takes t…
1a0dc14 
…he key too

The row read "Qdrant: ...", but on main Neo4jConf carries the same mixin and
the Neo4j store receives the resolved factory the same way (MemMachine#1678), with no
docs row of its own. Named inline, as the table names Milvus on its rows.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
marvinyu-memverge
marvinyu-memverge approved these changes Sep 22, 2026
View reviewed changes

@marvinyu-memverge marvinyu-memverge left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 1a0dc14. No asks; one observation.

Observation, not an ask: MilvusConf carries no MetricsFactoryIdMixin on main or on speedkick, and database_manager passes nothing to MilvusVectorStoreParams.metrics_factory (the field exists), so the Milvus store is the fifth component in the shape #1532 describes. Out of this PR's scope; flagging so it is on the list.

Verified: diff-of-diffs of f51702f against b6c90ab differs only in the stated omission (the Dockerfile ARG EXTRAS block) and the stated addition (the docs row), plus the import line your self-review already named; 1a0dc14 on top is the one docs line naming Neo4j on that row, which matches Neo4jConf on main. The wiring mirrors the Neo4j precedent at database_manager.py:267; QdrantVectorStoreParams.metrics_factory and the OperationTracker(params.metrics_factory, prefix="vector_store_qdrant") at :637 were already on main, so the one kwarg at :611 is the whole fix. get_metrics_factory() resolves the class-level "prometheus" factory every wired component shares, and it dedupes histograms by name, so two Qdrant confs in one process share one vector_store_qdrant_latency_seconds series with no duplicate registration. The test sees the real factory (QdrantVectorStore is patched, so no tracker is built in the unit test) and fails on KeyError if the kwarg is removed. CI green at head, no threads.

@edwinyyyu edwinyyyu mentioned this pull request Sep 22, 2026
Open
@edwinyyyu
edwinyyyu merged commit fa66fb6 into MemMachine:main Sep 24, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malatewang malatewang malatewang approved these changes
@marvinyu-memverge marvinyu-memverge marvinyu-memverge approved these changes
@wanghy73 wanghy73 Awaiting requested review from wanghy73

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@edwinyyyu @malatewang @marvinyu-memverge @wanghy73