fix(instrumentation-qdrant): stop wrapping methods that qdrant-client 1.16 removed#4041
Open
saivedant169 wants to merge 2 commits intotraceloop:mainfrom
Open
fix(instrumentation-qdrant): stop wrapping methods that qdrant-client 1.16 removed#4041saivedant169 wants to merge 2 commits intotraceloop:mainfrom
saivedant169 wants to merge 2 commits intotraceloop:mainfrom
Conversation
… 1.16 removed On qdrant-client 1.16 a set of legacy client methods (search, search_batch, search_groups, recommend, recommend_batch, recommend_groups, discover, discover_batch and upload_records) were dropped in favour of the newer query / query_points APIs. The instrumentor still tried to wrap them at import time, which blew up with an AttributeError before user code had a chance to run. Drop those entries from the wrapped method tables and the wrapper's method dispatch, bump the minimum supported qdrant-client version to 1.10 so the floor matches what we actually wrap, and add a small regression test that exercises a full instrument/uninstrument cycle against the installed client. Closes traceloop#3492
|
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated Qdrant instrumentation: bumped declared minimum Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.json (1)
1-72:⚠️ Potential issue | 🟠 MajorAdd
query_pointsandquery_batch_pointsto async configuration for parity with sync instrumentation.The sync
qdrant_client_methods.jsonincludes bothquery_pointsandquery_batch_points(with dedicated wrapper.py handling that remapsquery_points→searchandquery_batch_points→search_batch), but the async version is missing these entries. SinceAsyncQdrantClientexposes both methods and the wrapper already has the necessary attribute-setting logic in place (lines 58–61, 72–75, 112), async users who migrate to the recommendedquery_points/query_batch_pointsmethods will currently receive no Qdrant spans for their primary read path.Proposed fix
{ "object": "AsyncQdrantClient", "method": "query", "span_name": "qdrant.query" }, + { + "object": "AsyncQdrantClient", + "method": "query_points", + "span_name": "qdrant.query_points" + }, { "object": "AsyncQdrantClient", "method": "query_batch", "span_name": "qdrant.query_batch" }, + { + "object": "AsyncQdrantClient", + "method": "query_batch_points", + "span_name": "qdrant.query_batch_points" + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.json` around lines 1 - 72, Add entries for "query_points" and "query_batch_points" to the AsyncQdrantClient config JSON so async instrumentation matches sync behavior: insert objects with "object": "AsyncQdrantClient", "method": "query_points", "span_name": "qdrant.query_points" and "object": "AsyncQdrantClient", "method": "query_batch_points", "span_name": "qdrant.query_batch_points" into opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.json (the wrapper logic in wrapper.py that remaps query_points→search and query_batch_points→search_batch will pick these up).
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-qdrant/pyproject.toml (1)
38-38: Test dependency range LGTM, but consider pinning an upper bound more conservatively.
qdrant-client>=1.10,<2is consistent with the instrumentation floor and lets CI pick up 1.16. Note, however, that since this PR was motivated precisely byqdrant-client 1.16removing public API, allowing the full<2range in the test group means the next minor release that drops/renames more methods may silently break users without CI catching it until after release. Consider either capping to the currently verified version (e.g.,<1.17) or wiring a periodic matrix test against the latest release so drifts are surfaced quickly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-qdrant/pyproject.toml` at line 38, The test dependency range "qdrant-client>=1.10,<2" is too broad given recent breaking changes; update the dependency spec in packages/opentelemetry-instrumentation-qdrant's pyproject to a more conservative upper bound (e.g., change "<2" to "<1.17") or alternatively add a CI matrix job that periodically tests against the latest qdrant-client to detect API drift; locate the dependency line containing "qdrant-client>=1.10,<2" and replace or augment it accordingly.packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py (1)
141-141: Leadinguninstrument()on a fresh instrumentor.Calling
uninstrument()before anyinstrument()relies onBaseInstrumentortolerating that ordering (it currently does, via internal_is_instrumented_by_opentelemetry). If that's intentional as part of the drift check, fine; otherwise drop it to keep the cycle symmetric (instrument → uninstrument → instrument → uninstrument) and finish in the uninstrumented state so this test doesn't leave global wrappers installed for subsequent tests in the module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py` at line 141, The test currently calls instrumentor.uninstrument() before any instrument(), which relies on BaseInstrumentor tolerating that ordering via the internal _is_instrumented_by_opentelemetry flag; remove that initial instrumentor.uninstrument() call so the test follows a symmetric cycle (instrument() → uninstrument() → instrument() → uninstrument()) and ends in the uninstrumented state, or alternatively replace the leading call with an assert/cleanup that ensures no global wrappers are installed without invoking uninstrument() prematurely; locate the call to instrumentor.uninstrument() in the test and remove or replace it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py`:
- Around line 127-144: The test currently only calls
QdrantInstrumentor.instrument()/uninstrument() and so misses stale method names
in WRAPPED_METHODS; update the
test_instrument_does_not_fail_on_missing_client_methods to import
WRAPPED_METHODS from opentelemetry.instrumentation.qdrant and the real Qdrant
client class used by the instrumentor, then assert that for every (obj_name,
method_name) tuple in WRAPPED_METHODS the target client class has that attribute
(use hasattr on the client class or the specific attribute container referenced
by the tuple) so the test fails if WRAPPED_METHODS contains a nonexistent/typo'd
method; keep the existing instrument/uninstrument calls to preserve the
regression check.
---
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.json`:
- Around line 1-72: Add entries for "query_points" and "query_batch_points" to
the AsyncQdrantClient config JSON so async instrumentation matches sync
behavior: insert objects with "object": "AsyncQdrantClient", "method":
"query_points", "span_name": "qdrant.query_points" and "object":
"AsyncQdrantClient", "method": "query_batch_points", "span_name":
"qdrant.query_batch_points" into
opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.json (the
wrapper logic in wrapper.py that remaps query_points→search and
query_batch_points→search_batch will pick these up).
---
Nitpick comments:
In `@packages/opentelemetry-instrumentation-qdrant/pyproject.toml`:
- Line 38: The test dependency range "qdrant-client>=1.10,<2" is too broad given
recent breaking changes; update the dependency spec in
packages/opentelemetry-instrumentation-qdrant's pyproject to a more conservative
upper bound (e.g., change "<2" to "<1.17") or alternatively add a CI matrix job
that periodically tests against the latest qdrant-client to detect API drift;
locate the dependency line containing "qdrant-client>=1.10,<2" and replace or
augment it accordingly.
In
`@packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py`:
- Line 141: The test currently calls instrumentor.uninstrument() before any
instrument(), which relies on BaseInstrumentor tolerating that ordering via the
internal _is_instrumented_by_opentelemetry flag; remove that initial
instrumentor.uninstrument() call so the test follows a symmetric cycle
(instrument() → uninstrument() → instrument() → uninstrument()) and ends in the
uninstrumented state, or alternatively replace the leading call with an
assert/cleanup that ensures no global wrappers are installed without invoking
uninstrument() prematurely; locate the call to instrumentor.uninstrument() in
the test and remove or replace it accordingly.
🪄 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: fdaabf1b-1b6b-4210-b0fb-e7651e8518ac
📒 Files selected for processing (6)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.pypackages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.jsonpackages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/qdrant_client_methods.jsonpackages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.pypackages/opentelemetry-instrumentation-qdrant/pyproject.tomlpackages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py
Pick up four items flagged in review: - Add query_points and query_batch_points entries to the async method list so AsyncQdrantClient users keep getting spans on the recommended read path. The sync list already had both, the async list did not. - Tighten the qdrant-client test dependency from <2 to <1.17 so the next breaking minor cannot slip into CI silently. We can lift the cap in a follow up once a new release has been verified. - Rework the regression test so it walks WRAPPED_METHODS against the installed qdrant-client and fails with a useful message if any (object, method) entry is not resolvable. The hasattr guards in _instrument / _uninstrument were swallowing drift quietly, so the old test only caught a truly unhandled exception. - Drop the leading uninstrument() on a fresh instrumentor and end the cycle uninstrumented so the test does not leave global wrappers in place for anything that runs after it.
Contributor
Author
|
Thanks for the thorough pass. Pushed 76380ff picking up all four items:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3492
feat(instrumentation): ...orfix(instrumentation): ....What
On
qdrant-client1.16 the client dropped a handful of legacy methods in favour of the newerquery/query_pointsAPIs. The instrumentor was still trying to wrap them up front throughwrap_function_wrapper, which meant importingQdrantInstrumentor().instrument()on a modern client raised:The methods that are no longer on the client are
search,search_batch,search_groups,recommend,recommend_batch,recommend_groups,discover,discover_batchandupload_records.Changes
qdrant_client_methods.jsonandasync_qdrant_client_methods.jsonso we no longer ask wrapt to resolve attributes that are not there.wrapper.pyso the dispatch only handles methods we still wrap._instrumentsfromqdrant-client >= 1.7toqdrant-client >= 1.10so the declared floor actually matches what we wrap (query_pointslands in 1.10), and bump the test dependency accordingly.instrument / uninstrumentcycle against whicheverqdrant-clientis installed, so if the wrapped method list ever drifts out of sync again we notice it in CI.How I checked it
Installed
qdrant-client==1.16.1in a clean venv together with this branch and ran:All 5 tests pass, including the new regression test.
uv run ruff checkis also clean on the package.Before the fix, the same setup fails immediately with the
AttributeErrorabove atQdrantInstrumentor().instrument()time.AI disclosure
Took help of an AI tool for the diff review and to draft this description. The design call, reproduction and test were done by hand.
Summary by CodeRabbit
Chores
Bug Fixes
Tests