fix(qdrant): restore search instrumentation on qdrant-client >= 1.12 - #4470
basil-k-aji-dev wants to merge 1 commit into
Conversation
|
|
6c4a486 to
cda44a6
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⛔ Files ignored due to path filters (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Qdrant instrumentation adds sync and async query methods, updates ChangesQdrant method coverage
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The PR adds tracing for modern Qdrant query methods and validates method coverage without an identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 2 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AsyncQdrantClient emitted no search spans at all on qdrant-client 1.12+. Every search method in its wrapped-method list (search, search_batch, search_groups, query, query_batch, discover, discover_batch, recommend, recommend_batch, recommend_groups) was removed upstream in 1.12, and the modern replacements were never added to the async list. The hasattr guard in _instrument() skips the missing ones, so this degraded silently rather than raising. Add query_points, query_points_groups and query_batch_points to the async list, and query_points_groups to the sync list. Legacy entries are kept: the package declares support for qdrant-client >= 1.7, where they exist. The test group pinned qdrant-client >= 1.9.1, < 1.12, so CI never ran against a client where these methods were absent. Lift the pin to >= 1.12 and add tests asserting the sync and async lists stay in parity, that the modern query surface is covered, and that non-legacy entries resolve on the installed client. Note: uv.lock is regenerated here. Its requires-python was stale at ">=3.9, <4" while pyproject.toml already declared ">=3.10,<4", so the py39 wheel entries drop out. This is a lockfile realignment, not a change to supported Python versions. Fixes traceloop#3492 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cda44a6 to
19bc238
Compare
2 participants
feat(instrumentation): ...orfix(instrumentation): ....No screenshots: the change is the absence vs. presence of spans, which the added tests assert directly. Happy to attach a trace view if you'd prefer.
The problem
AsyncQdrantClientemits no search spans at all onqdrant-client >= 1.12.Every search method in
async_qdrant_client_methods.json—search,search_batch,search_groups,query,query_batch,discover,discover_batch,recommend,recommend_batch,recommend_groups— was removed fromqdrant-clientin 1.12. The modern replacements were added to the sync list but never to the async one.Because
_instrument()guards each wrap withhasattr, nothing raises. The instrumentor just silently wraps nothing:Across both lists, 24 of 48 entries no longer resolve. The sync client is largely fine —
query_pointsandquery_batch_pointsare listed — but it is missingquery_points_groups, so grouped queries are untraced there too.This went unnoticed because the
testdependency group pinsqdrant-client>=1.9.1,<1.12, so CI has never run against a client where these methods are absent.The fix
query_points,query_points_groupsandquery_batch_pointsto the async list; addquery_points_groupsto the sync list.query_points_groupsthrough_set_search_attributes, and map its collection-name attribute tosearch_groups, matching the existingquery_points→searchandquery_batch_points→search_batchmappings.qdrant-client>=1.12.tests/test_method_coverage.py: asserts the sync and async lists stay in parity, that the modern query surface is covered, and that every non-legacy entry resolves against the installed client — so this cannot silently rot again.Legacy entries are deliberately kept. The package declares
qdrant-client >= 1.7, where those methods still exist and are still worth tracing. Thehasattrguard makes them harmless on newer clients. The new coverage test excludes them explicitly.On #3492
This started from #3492, which reports
AttributeError: type object 'QdrantClient' has no attribute 'upload_records'. That crash no longer reproduces — thehasattrguard in_instrument()fixed it after v0.48.1. What the guard did was convert a loud failure into silent span loss, which is what this PR addresses. Happy to retitle or split if you'd rather track that separately.Note on
uv.lockThe lockfile is regenerated for the dependency change. Its
requires-pythonwas stale at">=3.9, <4"whilepyproject.tomlalready declared">=3.10,<4", so the Python 3.9 wheel entries drop out. That is a lockfile realignment, not a change to supported Python versions.Verification
8/8tests pass againstqdrant-client1.19.0,ruffclean. The new coverage tests fail against the pre-fix method lists and pass after:Possible follow-ups
retrieve,count,facetandsearch_matrix_pairs/search_matrix_offsetsare public data-plane operations that are currently untraced. Left out to keep this focused — glad to add them in a separate PR if wanted.Summary by CodeRabbit