feat: add client telemetry support - #3770
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xiaofan-luan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ff66688 to
5b05f65
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3770 +/- ##
==========================================
- Coverage 94.22% 94.16% -0.07%
==========================================
Files 77 78 +1
Lines 16204 17813 +1609
==========================================
+ Hits 15268 16773 +1505
- Misses 936 1040 +104 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5b05f65 to
6f60fe4
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f60fe4601
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b3b64a8 to
e3281fe
Compare
pymilvus-bot
left a comment
pymilvus-bot
left a comment
There was a problem hiding this comment.
Requesting changes for the telemetry correctness and lifecycle issues called out inline. The most serious issue is that persistent configs are passed through the one-time command watermark, so a newly relevant config can be acknowledged and hashed without ever being applied. Iterator-internal RPCs and sync Future parsing also violate the stated one-final-outcome contract, and sync shutdown can return while its worker is still running. These were verified against the current head with focused reproductions.
e3281fe to
c874b2e
Compare
c874b2e to
0c846be
Compare
pymilvus-bot
left a comment
pymilvus-bot
left a comment
There was a problem hiding this comment.
Inline review comments for the current head. The existing config-hash discussion already covers the acknowledged protocol gap, so it is not duplicated here.
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
0c846be to
a9e449e
Compare
issue: #53099 ## What problem does this PR solve? After the last persistent telemetry config matching a client is deleted, the client continues to report its old non-empty config hash. The heartbeat response contains only commands, so an empty command list cannot distinguish an already-matching non-empty config set from an authoritative empty set. Existing SDKs therefore preserve the old hash and never converge. Related design: https://github.com/milvus-io/milvus/blob/master/docs/design-docs/design_docs/20260131-client_side_telemetry.md PyMilvus discussion: milvus-io/pymilvus#3770 (comment) ## What is changed and how does it work? - Synthesize a stable persistent no-op push_config sentinel only when the effective config set is empty and the client reports another non-empty hash. - Accept both the original empty hash and the sentinel hash as converged empty states, so fresh clients receive no extra command. - Keep the sentinel out of etcd and command List/Delete APIs, and ignore its ACK in ordinary reply history. - Document the compatibility behavior and its cross-SDK hash vector. This is a server-only rolling-upgrade fix. It requires no protobuf or SDK change because all telemetry SDKs already accept an empty push_config object as a no-op and calculate the same persistent-command hash. ## Verification - go test ./internal/rootcoord/telemetry - go test -race ./internal/rootcoord/telemetry - go vet ./internal/rootcoord/telemetry - git diff --check A broader local go test ./internal/rootcoord/... reaches an unrelated existing Loon CGo ABI mismatch in internal/storagev2/packed; the telemetry package and race suite pass, and PR CI will provide the clean broader build signal. Signed-off-by: xiaofanluan <xf@hjjaq.com> Co-authored-by: xiaofanluan <xf@hjjaq.com>
| if self._database_bound | ||
| else self._database_provider() or "" | ||
| ) | ||
| if database: |
There was a problem hiding this comment.
[P1] Report the implicit default database in heartbeats
MilvusClient and AsyncMilvusClient default db_name to "", and this branch omits reserved["db_name"] when that value is empty. RootCoord only adds non-empty database names to AccessedDatabases, while database:default command matching consults that set. Consequently, a normal MilvusClient() or AsyncMilvusClient() never receives commands or persistent configs scoped to database:default, even though its operations run in that database. Please normalize the logical empty name to "default" in telemetry identity (or handle the convention server-side) and cover database:default in an end-to-end test.
|
|
||
| def _queue_reply(self, reply: CommandReply) -> None: | ||
| with self._pending_lock: | ||
| self._pending_replies.append( |
There was a problem hiding this comment.
[P1] Enforce the built-in reply limit at a common boundary
The linked design caps built-in reply payloads at 1 MiB, but this queue accepts every payload unchanged. Both the no-payload collection_metrics response and get_config serialize the unbounded enabled_collections set, which can grow across repeated commands and exceed the limit. If an oversized reply then makes the heartbeat fail, it remains in _pending_replies and can poison every later heartbeat. Please enforce the limit for all built-in handlers before enqueueing and replace an oversized result with a bounded failure reply.
3 participants
Summary
Related work
Verification