feat: add client telemetry support - #2040
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: xiaofan-luan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
60ce683 to
816a6a9
Compare
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Sampling kept a counter modulo 10000 and recorded an operation while the remainder was under rate*10000, so the sampled operations formed one contiguous run per cycle. A cycle is ten thousand operations while a heartbeat window is tens or hundreds, so every window came out wholly sampled or wholly dropped: at 3 QPS a rate of 0.25 gave fourteen minutes of full metrics followed by forty-one reporting nothing from a client that never stopped working. The long-run ratio was right; the ratio inside a window, which is the only unit the telemetry API reports, never was. Accumulate the rate instead and sample on the operation that carries the accumulator across a whole unit: 0.25 samples every fourth operation, and any stretch of operations holds the configured ratio. A rate too small to represent now still samples rarely rather than never -- the old threshold truncated anything below 1e-4 to zero and then sampled nothing, so a configured rate silently meant "off". Matches the Go client, milvus-io/milvus#52615. Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
The heartbeat interval is also the metrics window: each heartbeat carries the operations since the last one, and the coordinator answers a telemetry query from the window before the newest, so a caller reads data between one and two intervals old. At the previous default of 30 seconds that is up to a minute behind, which reads as a stalled client rather than as a lagging view. Ten seconds puts the worst case at twenty. The cost is the coordinator heartbeat rate, which scales with the number of connected clients rather than with traffic; a deployment with enough clients for that to matter can raise the interval, per client or fleet-wide, with a persistent push_config. Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
816a6a9 to
c7fb80e
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: c7fb80ec23
ℹ️ 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".
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
Signed-off-by: xiaofanluan <xiaofan.luan@zilliz.com>
| return ""; | ||
| } | ||
| String requestId = connectConfig.getClientRequestId().get(); | ||
| return ClientRequestInterceptor.isValidClientRequestId(requestId) ? requestId : ""; |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/MilvusClientV2.java line:334
Medium ---- captureClientRequestId() now applies the strict OTel trace-ID check (exactly 32 lowercase hex, non-zero) before the value is set as CLIENT_REQUEST_ID_OPTION on the future stub, so on queryAsync/searchAsync/hybridSearchAsync/getAsync an arbitrary caller-supplied client_request_id becomes "" and is dropped from the wire metadata, while sync V2 and legacy V1 still transmit it. Before this PR the raw thread-local value reached the wire on the async path too, so callers using ConnectConfig.clientRequestId with a non-OTel correlation ID silently lose access-log correlation on async calls. The strict check is already applied exactly where telemetry needs it (TelemetryInterceptor.requestId), so the raw value can be kept in the option and filtered only when recording error details. This is a follow-up to the resolved thread on ClientRequestInterceptor.java:68 -- that fix restored wire preservation at the interceptor level, but the async path never reaches the interceptor with the raw id, so the drop persists for async V2.
| try (TelemetryInterceptor.LogicalOperationScope ignored = | ||
| TelemetryInterceptor.beginLogicalOperation()) { | ||
| T result = supplier.get(); | ||
| manager.recordOperation(operation, collection, startNanos, "", requestId); |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/MilvusClientV2.java line:348
Medium ---- The happy-path manager.recordOperation(...) here is not wrapped in a best-effort guard, unlike the V1 legacy path: MilvusServiceClient.recordLogicalResult calls telemetry.recordOperation inside try { ... } catch (RuntimeException ignored) { // Telemetry is best-effort and must never replace the operation result. }. Here, if recordOperation throws on the success path, control jumps to the catch (RuntimeException | Error) below, which calls recordOperation again (error path) and then rethrows — so a telemetry-side failure would turn an otherwise successful Milvus operation into an exception for the caller. Consider wrapping the success-path recording in the same try/catch the V1 path uses so the telemetry contract is identical across both client surfaces.
| "Insert", "Delete", "Upsert", "Search", "HybridSearch", "Query", "RunAnalyzer")); | ||
| public static final CallOptions.Key<Boolean> LOGICAL_OPERATION_OPTION = | ||
| CallOptions.Key.create("milvus-telemetry-logical-operation"); | ||
| private static final ThreadLocal<Integer> LOGICAL_OPERATION_DEPTH = |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/telemetry/TelemetryInterceptor.java line:45
Low ---- LOGICAL_OPERATION_DEPTH is a static ThreadLocal, and MilvusServiceClient keeps a second, separate static ThreadLocal of the same name (MilvusServiceClient.java line:94). Because both are process-global per thread, any two clients running interleaved or nested logical operations on the same thread share the same depth counter (e.g., a user callback issued inside client A's operation that triggers client B's search). The scopes are balanced by try-with-resources, so normal nesting is fine, but the shared static state means one client's scope can suppress or mis-record telemetry for an unrelated client's operation on the same thread. Consider instance-scoping the depth counters to the owning client.
| SearchResp search(SearchReq request, String clusterId) { | ||
| return rpcUtils.retry(() -> vectorService.search(this.getRpcStub(), request, clusterId)); | ||
| String collection = request == null ? "" : request.getCollectionName(); | ||
| return recordLogicalOperation("Search", collection, |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/MilvusClientV2.java line:1094
Medium ---- Iterators are the one V2 DQL path that is not wrapped in a logical-operation scope: searchIteratorV2/queryIteratorV2 build their stub via createIteratorRpcStub and issue search/query RPCs per page-fetch without any recordLogicalOperation wrapper. Those RPCs still traverse the TelemetryInterceptor, so each page becomes its own per-attempt Search/Query metric. The PR summary states the goal is "report one outcome per logical operation across legacy/V1, V2, async, retry, validation, and result processing paths"; an iterator over N pages will instead report N Search/Query entries. Please confirm whether per-page recording for iterators is intended, or whether iterator construction should be wrapped so one logical iteration produces one metric.
| } | ||
| } | ||
|
|
||
| public static final class MetricSnapshot { |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/telemetry/ClientTelemetryManager.java line:1317
Low ---- These three snapshot value classes each store the same value twice: public snake_case fields (request_count, end_time, collection_metrics) and private camelCase copies (requestCount, endTime, collections), kept in sync by hand in the constructors. Any future metric field has to be added and assigned in two places, and a one-sided edit would silently diverge the JSON replies (show_errors / show_latency_history build their payloads from the camelCase copies) from what external callers of getMetricsSnapshots()/snapshotRuntimeState() read via the snake_case fields, with no test catching it unless the duplicate is asserted. Consider a single field set with a Gson field-naming strategy (@SerializedName or FieldNamingPolicy) so there is one source of truth to keep in sync.
Summary
Related work
Verification