Use OpContext trace span for Cardinal tracing by jamesgao-jpg · Pull Request #1684 · zilliztech/knowhere

jamesgao-jpg · 2026-06-17T11:53:37Z

Summary

keep Cardinal v2 pinned to the existing released version v3.0.2; this PR does not advance Knowhere main to the tracing Cardinal branch
prefer the active milvus::OpContext trace span in Knowhere search paths, while keeping the legacy trace_id/span_id/trace_flags config fields as a compatibility fallback for callers that do not yet seed OpContext::trace_span
use the shared milvus-common NestedSpanGuard helper to temporarily expose the Knowhere search span to downstream Cardinal calls
include shared trace helpers from common/Tracer.h and construct spans through the existing tracer utilities instead of going through the removed StartScopedSpan wrapper

Dependency

PR https://github.com/zilliztech/cardinal/pull/865 introduces the Cardinal tracing implementation that a future Cardinal version bump can expose through Knowhere. This PR only prepares Knowhere to propagate the OpContext trace span and keeps the current Cardinal v2 version pin unchanged.

This branch pins the published helper package milvus-common/1.0.0-ae43d5c@milvus/dev#fe24230e26e129f6bf303a8342ba8980, produced from milvus-common commit ae43d5cf11852185ad5d5b48808cb7b58cc4ccd4.

Verification

Not run locally. This branch is intended to be validated through upstream CI and the downstream Milvus integration branch.

Follow-up issue: #1693 tracks the future Cardinal v2 version bump after Cardinal tracing is released.

mergify · 2026-06-17T11:54:19Z

@jamesgao-jpg 🔍 Important: PR Classification Needed!

For efficient project management and a seamless review process, it's essential to classify your PR correctly. Here's how:

If you're fixing a bug, label it as kind/bug.
For small tweaks (less than 20 lines without altering any functionality), please use kind/improvement.
Significant changes that don't modify existing functionalities should be tagged as kind/enhancement.
Adjusting APIs or changing functionality? Go with kind/feature.

For any PR outside the kind/improvement category, ensure you link to the associated issue using the format: “issue: #”.

Thanks for your efforts and contribution to the community!.

mergify · 2026-06-17T15:04:43Z

@jamesgao-jpg e2e jenkins job failed, comment /run-e2e can trigger the job again.

mergify · 2026-06-17T18:18:32Z

@jamesgao-jpg e2e jenkins job failed, comment /run-e2e can trigger the job again.

mergify · 2026-06-18T06:05:52Z

@jamesgao-jpg e2e jenkins job failed, comment /run-e2e can trigger the job again.

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

liliu-z · 2026-06-29T10:47:35Z

-            span->SetAttribute(meta::ROWS, Count());
-            span->SetAttribute(meta::DIM, Dim());
-            span->SetAttribute(meta::NQ, dataset->GetRows());
+        auto span = milvus::tracer::ScopedSpan("knowhere search", milvus::OpContext::GetTraceSpan(op_context));


index.cc:165 derives the search span from op_context->GetTraceSpan(), but no released Milvus populates op_context->trace_span — a grep for trace_span = across internal/core/src finds nothing — so GetTraceSpan() returns nullptr and every Knowhere search/brute-force span (here and at brute_force.cc:512) is silently dropped. The trace_id/span_id/trace_flags JSON fields still advertised in config.h are now dead: a caller that sets them gets a silent no-op instead of a trace. Built against anything but the unmerged integration branch all Knowhere tracing disappears with no error, so gate this on the upstream change that writes trace_span, or remove the dead config fields until it lands.

Thanks, this was valid. Fixed in 199c60d: Knowhere now starts spans through StartMilvusSpanFromOpContextOrConfig, which prefers OpContext::trace_span but falls back to the legacy trace_id/span_id/trace_flags config fields when the OpContext span is absent. This keeps tracing working for Milvus versions that still pass trace context through JSON config, and applies to both index.cc search/range search and the brute-force span sites.

liliu-z · 2026-06-29T10:47:36Z

+            span_ptr->SetAttribute(meta::DIM, Dim());
+            span_ptr->SetAttribute(meta::NQ, dataset->GetRows());
        }
+        auto op_context_trace_span_guard =


The NestedSpanGuard at index.cc:174-175 writes span_ptr into op_context->trace_span before fanning out to parallel workers and restores it on scope exit. It is safe today because Milvus builds a fresh stack-local OpContext per search (segment_c.cpp:399) and OpContext deletes its copy/move constructors, so no two concurrent top-level searches share one. But the guard mutating caller-owned state is a latent contract violation: a future caller that shares one OpContext across concurrent Index::Search calls would hit an unsynchronized shared_ptr write/read race. Document the single-owner-per-search requirement, or carry the span on a per-call local instead of mutating op_context.

This is the same ownership concern raised and addressed in milvus-common PR zilliztech/milvus-common#96 (comment).

We fixed it in milvus-common commit 75fd9691a82f89caab715907e3ff4d2c7b5e25c4 and replied here: zilliztech/milvus-common#96 (comment). The shared OpContext::trace_span API now documents the contract directly: it is a single-threaded parent handoff slot, not a concurrent current-span stack, and code entering parallel work must copy the parent into per-worker state before nesting spans.

Knowhere follows that contract here: the guard publishes the current Knowhere search span at the search-wrapper level so downstream code can snapshot it as a parent. The shared API contract is intentionally documented in milvus-common, where the OpContext field and NestedSpanGuard live, instead of duplicating a second contract in every downstream caller.

liliu-z · 2026-06-29T10:47:37Z

        self.requires("abseil/20250127.0#481edcc75deb0efb16500f511f0f0a1c")
        self.requires("boost/1.83.0#4e8a94ac1b88312af95eded83cd81ca8")
-        self.requires("milvus-common/1.0.0-ac18852@milvus/dev#d9aa7497425662eb17749b4871700200")
+        self.requires("milvus-common/1.0.0-45bff01@milvus/dev#97948822f1dacdeafed7e8d5251df493")


The Cardinal pin at cmake/libs/cardinal/v2/CMakeLists.txt:6 changes from the immutable tag v3.0.2 to a mutable branch (origin/codex/cardinal-master-config-carrier-tracing) that is git-checked-out at build time, so builds track a moving branch tip and are not reproducible. The milvus-common reference at conanfile.py:119 pins commit 45bff01, which is not an ancestor of milvus-common master (a side commit, 'move scoped trace helpers into Tracer'); its conan recipe revision hash does keep that package itself reproducible, but it likewise points at unmerged upstream work. Gate the merge on these upstream changes landing and repin Cardinal to an immutable tag/commit on mainline.

Agreed. This is a dependency-settlement issue rather than a Knowhere code issue.

This PR is intentionally staged on top of unmerged tracing dependencies: Cardinal PR https://github.com/zilliztech/cardinal/pull/865 and milvus-common PR zilliztech/milvus-common#96. We should keep this Knowhere PR gated until those land. Before merging Knowhere, I will repin Cardinal from the mutable tracing branch to an immutable mainline tag/commit and update the milvus-common reference to the corresponding merged/published package revision.

liliu-z · 2026-06-29T10:47:38Z


 #if defined(NOT_COMPILE_FOR_SWIG) && !defined(KNOWHERE_WITH_LIGHT)
-        const BaseConfig& b_cfg = static_cast<const BaseConfig&>(*cfg);
+        BaseConfig& b_cfg = static_cast<BaseConfig&>(*cfg);


b_cfg is needlessly non-const at index.cc:163 and :257 — it was const BaseConfig& before this PR and every remaining use is a read (e.g. b_cfg.metric_type.value() in the SetAttribute calls), and BaseConfig's optional fields expose a const .value() overload. Revert both casts to const BaseConfig&.

Fixed in 199c60d. The two b_cfg casts in index.cc are now const BaseConfig& again; the remaining uses only read config fields for span attributes and fallback trace context.

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

alexanderguzhva · 2026-07-01T15:47:45Z

/lgtm
/approve

sre-ci-robot · 2026-07-01T15:47:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexanderguzhva, jamesgao-jpg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [alexanderguzhva]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

## Summary - keep the upstream Knowhere pin from Milvus master: `zilliztech/knowhere.git` at `v3.0.5` - pin `milvus-common/1.0.0-a3299ca@milvus/dev#eea0e22b8d5e36ff6f0a5ffbed14703e` - seed the SegCore search span into the `milvus::OpContext` used by `ExecPlanNodeVisitor` - include trace span types from `common/Tracer.h`, matching the latest milvus-common helper API shape - keep the trace parent carrier in Milvus local to each search execution path before Knowhere/Cardinal consume it - propagate the same `milvus::OpContext` into indexed vector iterator paths so group-by, iterative-filter, and iterator-v2 search modes can preserve the trace parent into Knowhere ## Dependencies Knowhere PR zilliztech/knowhere#1684 prepared Knowhere to consume the OpContext trace span. This Milvus branch no longer pins a temporary Knowhere branch; it keeps the upstream `v3.0.5` pin from Milvus master. The downstream source changes target the published helper package `milvus-common/1.0.0-a3299ca@milvus/dev#eea0e22b8d5e36ff6f0a5ffbed14703e`, produced from milvus-common commit `a3299cac82bb85868d2e6cd9233e92202fd5ae30` and published through conanfiles PR milvus-io/conanfiles#155. The branch has been rebased on current Milvus `master`; FileManager stream path work from PR #50455 is inherited from upstream `master` and is no longer carried in this PR branch. ## Verification - Not run locally. Milvus validation is intentionally left for the downstream image/CI validation path. --------- Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

sre-ci-robot requested review from foxspy and marcelo-cjl June 17, 2026 11:53

sre-ci-robot added the size/L label Jun 17, 2026

mergify Bot added the needs-dco label Jun 17, 2026

mergify Bot added the do-not-merge/missing-related-issue label Jun 17, 2026

jamesgao-jpg mentioned this pull request Jun 17, 2026

enhance: seed search OpContext trace span milvus-io/milvus#50606

Merged

jamesgao-jpg force-pushed the codex/knowhere-master-cardinal-config-carrier-tracing branch from 85d6c4b to 8e8431d Compare June 17, 2026 12:14

mergify Bot added dco-passed and removed needs-dco labels Jun 17, 2026

jamesgao-jpg force-pushed the codex/knowhere-master-cardinal-config-carrier-tracing branch from 7773d06 to 22b4b59 Compare June 17, 2026 13:07

jamesgao-jpg force-pushed the codex/knowhere-master-cardinal-config-carrier-tracing branch from bbcff0b to 90dbd1a Compare June 17, 2026 16:42

jamesgao-jpg mentioned this pull request Jun 17, 2026

Add OpContext trace span helpers zilliztech/milvus-common#96

Merged

jamesgao-jpg force-pushed the codex/knowhere-master-cardinal-config-carrier-tracing branch from 0f84502 to 4cbd6cb Compare June 18, 2026 03:52

jamesgao-jpg mentioned this pull request Jun 18, 2026

Add milvus-common 1.0.0-45bff01 milvus-io/conanfiles#137

Closed

jamesgao-jpg force-pushed the codex/knowhere-master-cardinal-config-carrier-tracing branch from 56811f8 to c26b842 Compare June 18, 2026 09:17

jamesgao-jpg added 9 commits June 29, 2026 14:15

Pin Cardinal config-carrier tracing branch

6627e09

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

Use OpContext trace span in Knowhere tracing

2a1dd73

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

Pin milvus-common trace helper package

3b987f4

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

refactor trace span helper usage

eb764e4

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

include Tracer for scoped trace helpers

b51ceed

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

Pin milvus-common trace helper package

75dfca2

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

Fix tracing clang-format

0f5cd52

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

Define Cardinal graph candidate limit metric

11332bb

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

Force Cardinal branch checkout

b0eb536

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

jamesgao-jpg force-pushed the codex/knowhere-master-cardinal-config-carrier-tracing branch from 5172343 to b0eb536 Compare June 29, 2026 06:27

mergify Bot added the ci-passed label Jun 29, 2026

liliu-z reviewed Jun 29, 2026

View reviewed changes

Restore config trace fallback

199c60d

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

mergify Bot removed the ci-passed label Jun 30, 2026

Fix tracing clang-format

c6f4419

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

mergify Bot added the ci-passed label Jun 30, 2026

Pin merged milvus-common tracing package

eb02f9f

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

mergify Bot removed the ci-passed label Jun 30, 2026

Keep Cardinal v2 on released version

4ba10f5

Signed-off-by: jamesgao-jpg <james.gao@zilliz.com>

jamesgao-jpg mentioned this pull request Jun 30, 2026

Track Cardinal v2 version bump for OpContext tracing #1693

Open

jamesgao-jpg removed the do-not-merge/missing-related-issue label Jun 30, 2026

mergify Bot added the ci-passed label Jun 30, 2026

sre-ci-robot assigned alexanderguzhva Jul 1, 2026

sre-ci-robot added the lgtm label Jul 1, 2026

sre-ci-robot added the approved label Jul 1, 2026

sre-ci-robot merged commit 65d38da into zilliztech:main Jul 1, 2026
13 checks passed

Uh oh!

Conversation

jamesgao-jpg commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependency

Verification

Uh oh!

mergify Bot commented Jun 17, 2026

Uh oh!

mergify Bot commented Jun 17, 2026

Uh oh!

mergify Bot commented Jun 17, 2026

Uh oh!

mergify Bot commented Jun 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderguzhva commented Jul 1, 2026

Uh oh!

sre-ci-robot commented Jul 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamesgao-jpg commented Jun 17, 2026 •

edited

Loading