fix(qdrant): cover AsyncQdrantClient.query_points and query_batch_points#4202
fix(qdrant): cover AsyncQdrantClient.query_points and query_batch_points#4202NishchayMahor wants to merge 2 commits into
AsyncQdrantClient.query_points and query_batch_points#4202Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds JSON span mappings for ChangesQdrant async query span coverage
Sequence Diagram(s)sequenceDiagram
participant Test as Test case
participant AsyncClient as AsyncQdrantClient
participant Instrumentation as OpenTelemetryInstrumentation
participant Exporter as InMemoryExporter
Test->>AsyncClient: call query_points / query_batch_points
AsyncClient->>Instrumentation: instrumented call (span created)
Instrumentation->>Exporter: export span (qdrant.query_points / qdrant.query_batch_points)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py`:
- Around line 167-174: The exporter fixture accumulates spans across the session
so async tests can pick up earlier sync spans; fix by clearing the in-memory
exporter at the start of each async test (e.g., in
test_qdrant_async_query_points and test_qdrant_async_query_batch_points) by
calling exporter.clear() (before invoking the async instrumented call and before
using exporter.get_finished_spans()) so next(...) will select the async span you
just produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46de7e04-2b4f-40f0-bc6d-394b71a54d2e
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.jsonpackages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py
The session-scoped exporter is cleared via an autouse fixture between tests, but adding an explicit clear at the top of each async test makes the contract self-documenting and immune to future fixture changes. Addresses the coderabbitai review on traceloop#4202.
|
Thanks @coderabbitai :), fair callout. The Pushed in
Re-ran locally, still 6/6 passing. Appreciate the second pair of eyes :) |
|
Tip For best results, initiate chat on the files or code changes.
(ノ◕ヮ◕)ノ*:・゚✧ 🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
The sync qdrant_client_methods.json already wraps query_points and query_batch_points on QdrantClient, but the async config is missing both, so async users get no spans for the modern query APIs even after traceloop#3500 quieted the original upload_records crash. Add the matching AsyncQdrantClient entries, plus two tests that exercise each method through the in-memory client and assert the span shows up with the expected attributes.
The session-scoped exporter is cleared via an autouse fixture between tests, but adding an explicit clear at the top of each async test makes the contract self-documenting and immune to future fixture changes. Addresses the coderabbitai review on traceloop#4202.
ef41cfc to
ace73d4
Compare
Summary
Follow-up to #3492 / #3500 picking up the async-parity gap @kngyeol flagged in the issue thread.
The original crash from
upload_recordsis gone since #3500, but thequery_points/query_batch_pointsmismatch between the sync and async method configs is still present onmain. Both methods exist onAsyncQdrantClientin currentqdrant-clientreleases, the sync JSON already lists them, the async JSON does not, so async users get no spans for the modern query APIs.Changes
async_qdrant_client_methods.json: addAsyncQdrantClient.query_pointsandAsyncQdrantClient.query_batch_pointsnext to the existingquery/query_batchentries, mirroring the sync layout.tests/test_qdrant_instrumentation.py: add two async tests that drivequery_pointsandquery_batch_pointsthrough the in-memoryAsyncQdrantClient, run viaasyncio.run(no new test deps), and assert the spans show up with the expected attributes. Usedupsertfor setup sinceAsyncQdrantClient.upload_collectionis sync internally and returnsNone.Test plan
uv run pytest tests/reports6 passed(4 existing sync tests + 2 new async ones).uv run ruff check .reportsAll checks passed!.AsyncQdrantClientin currentqdrant-clientso the wrap will not skip them at instrumentation time.Refs #3492.
Summary by CodeRabbit
New Features
Tests