-
Notifications
You must be signed in to change notification settings - Fork 844
fix(qdrant):changes to support all versions of qdrant package #3500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughUpdates add support for QdrantClient.query_points and query_batch_points, extend wrapper attribute handling with backward-compatible naming, add safer instrumentation initialization with exception handling, update tests to new APIs, and add a sample app demonstrating query_points usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to d096021 in 1 minute and 25 seconds. Click for details.
- Reviewed
212lines of code in5files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.py:54
- Draft comment:
Good defensive wrapping with try/except; consider using logger.exception (or including the traceback) to help with debugging instrumentation failures. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/qdrant_client_methods.json:47
- Draft comment:
New methods 'query_points' and 'query_batch_points' have been added. Ensure these names match the actual Qdrant client API and that any breaking changes are documented. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py:64
- Draft comment:
The wrapper now includes 'query_points' and 'query_batch_points' (with backward-compatible mapping to 'search' and 'search_batch'). Consider updating docstrings or external docs to reflect this behavior. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py:80
- Draft comment:
Test updates now invoke 'query_points' and 'query_batch_points'. Verify if legacy search methods should also retain tests for backward compatibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. packages/sample-app/sample_app/qdrant_app.py:47
- Draft comment:
Sample app correctly uses the new 'query_points' method. For style consistency, please ensure the file ends with a newline. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py:87
- Draft comment:
Typo noticed: The function name 'test_qdrant_searchs' may be incorrect (possibly intended to be 'test_qdrant_search' or 'test_qdrant_searches'). Please confirm the intended naming. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_gaDPKGYPVWlPX25K
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
@nirga @gyliu513 I have submitted this PR for the issue FIx #3492 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.py (1)
54-68: Narrow the exception handling to specific exception types.Catching broad
Exceptioncan mask unexpected errors. Consider catching specific exceptions thatwrap_function_wrappermight raise, such asAttributeError,ImportError, orValueError.Apply this diff:
target_class = getattr(qdrant_client, wrap_object, None) if target_class and hasattr(target_class, wrap_method): try: wrap_function_wrapper( MODULE, f"{wrap_object}.{wrap_method}", _wrap(tracer, wrapped_method), ) - except Exception as e: + except (AttributeError, ImportError, ValueError) as e: logger.warning( "Failed to instrument %s.%s: %s", wrap_object, wrap_method, str(e), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.py(1 hunks)packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/qdrant_client_methods.json(1 hunks)packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py(3 hunks)packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py(3 hunks)packages/sample-app/sample_app/qdrant_app.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.pypackages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.pypackages/sample-app/sample_app/qdrant_app.pypackages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py
🧬 Code graph analysis (3)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.py (2)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py (1)
_wrap(31-76)packages/opentelemetry-instrumentation-weaviate/opentelemetry/instrumentation/weaviate/wrapper.py (1)
_wrap(34-51)
packages/sample-app/sample_app/qdrant_app.py (1)
packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
Traceloop(36-267)init(48-198)
packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py (2)
packages/opentelemetry-instrumentation-qdrant/tests/conftest.py (1)
exporter(12-22)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
get_finished_spans(40-43)
🪛 Flake8 (7.3.0)
packages/sample-app/sample_app/qdrant_app.py
[error] 4-4: 'numpy as np' imported but unused
(F401)
🪛 Ruff (0.14.7)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/__init__.py
62-62: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (3)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/qdrant_client_methods.json (1)
47-51: LGTM!The new method mappings for
query_pointsandquery_batch_pointsare properly structured and consistent with the existing entries.Also applies to: 57-61
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py (1)
64-64: LGTM! Excellent backward compatibility approach.The additions of
query_pointsandquery_batch_pointsare well-implemented with backward-compatible attribute naming. This ensures existing monitoring and alerting systems continue to work while the span names reflect the new API methods.Also applies to: 70-70, 81-92, 122-124
packages/opentelemetry-instrumentation-qdrant/tests/test_qdrant_instrumentation.py (1)
80-98: LGTM! Tests properly validate new APIs and backward compatibility.The test updates correctly exercise the new
query_pointsandquery_batch_pointsmethods with updated models (QueryRequest), while importantly verifying that the backward-compatible attribute names (e.g.,QDRANT_SEARCH_COLLECTION_NAME) are still populated correctly.Also applies to: 102-124
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py (1)
60-71: Wiringquery_points/query_batch_pointsinto existing search paths looks correctIncluding
query_pointsin the search-like branch andquery_batch_pointsin the batch-search branch cleanly reuses the existing attribute logic without changing behavior for older methods. This is a straightforward, compatible extension.If you want to keep style consistent with the multi-line list above, you could also break the line at Line 70 into a multi-line list, but that’s purely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py(3 hunks)packages/sample-app/sample_app/qdrant_app.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sample-app/sample_app/qdrant_app.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py (6)
packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py (2)
_set_search_attributes(280-328)_set_span_attribute(53-57)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py (1)
_set_span_attribute(31-38)packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py (1)
_set_span_attribute(105-109)packages/opentelemetry-instrumentation-llamaindex/opentelemetry/instrumentation/llamaindex/custom_llm_instrumentor.py (1)
_set_span_attribute(71-75)packages/opentelemetry-instrumentation-chromadb/opentelemetry/instrumentation/chromadb/wrapper.py (1)
_set_span_attribute(26-30)packages/opentelemetry-instrumentation-weaviate/opentelemetry/instrumentation/weaviate/wrapper.py (1)
_set_span_attribute(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py (2)
79-91: Backward-compatibleattribute_methodmapping is consistent and avoids new attribute keysUsing
attribute_methodto map:
query_points→searchquery_batch_points→search_batchbefore constructing
qdrant.{attribute_method}.collection_namekeeps all collection-name attributes under the existingqdrant.search.*/qdrant.search_batch.*namespaces, which helps avoid fragmenting metrics across old and new method names.This looks correct and consistent with the intent of supporting newer client APIs without breaking existing dashboards.
119-123: Batch search attribute mapping forquery_batch_pointsmatches collection-name behaviorDefaulting
requeststo an empty list and then mapping:
attribute_method = "search_batch"whenmethod == "query_batch_points"so that you emit
qdrant.search_batch.requests_countkeeps batch request-count metrics aligned with the priorsearch_batchnaming. This mirrors the mapping in_set_collection_name_attributeand maintains backward compatibility without changing behavior for other batch methods.No issues here.
feat(instrumentation): ...orfix(instrumentation): ....Important
Enhance Qdrant instrumentation by adding
query_pointsandquery_batch_pointsmethods, updating tests, and adding a sample app.query_pointsandquery_batch_pointsmethods toqdrant_client_methods.json._instrument()in__init__.pyto handle new methods and log errors if instrumentation fails._wrap()inwrapper.pyto support new methods and map them for backward compatibility.test_qdrant_instrumentation.pyto testquery_pointsandquery_batch_pointsmethods.qdrant_app.pyto demonstrate usage ofquery_pointswith a sample dataset.This description was created by
for d096021. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.