fix(qdrant): support qdrant-client v1.12+ and handle removed methods gracefully#4004
fix(qdrant): support qdrant-client v1.12+ and handle removed methods gracefully#4004Koushik-Salammagari wants to merge 2 commits intotraceloop:mainfrom
Conversation
…gracefully - Remove upload_records from method lists (removed in qdrant-client 1.12) - Wrap wrap_function_wrapper in try/except AttributeError so removed methods in future releases do not crash instrumentation on startup - Update test dependency upper bound from <1.12 to <2 - Add error span recording (record_exception + StatusCode.ERROR) on wrapped call failures Fixes 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGuarded method wrapping and removed obsolete method mappings for Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Instr as QdrantInstrumentor
participant Wrapt as wrapt
participant Client as QdrantClient
participant Tracer as Tracer/Span
App->>Instr: instrument()
Instr->>Wrapt: wrap_function_wrapper(target method)
alt method exists
Wrapt->>Client: replace method with wrapper
else method missing
Wrapt-->>Instr: AttributeError
Instr-->>App: log debug, continue
end
App->>Client: call wrapped method(...)
Client->>Tracer: Tracer.start_span(record_exception=False,set_status_on_exception=False)
Tracer->>Client: execute original method
alt method raises
Client-->>Tracer: exception
Tracer->>Tracer: record_exception + set Status(ERROR)
Tracer-->>App: re-raise exception
else success
Tracer->>Tracer: set Status(OK) if response truthy
Tracer-->>App: return response
end
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 the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/wrapper.py`:
- Around line 73-76: In the exception handler in wrapper.py replace the
span.set_status call that includes str(e) with an error-only status (e.g., call
span.set_status(Status(StatusCode.ERROR)) or set_status(StatusCode.ERROR)
depending on the existing API) so only the error code is set while leaving
span.record_exception(e) intact; update the call sites using
span.record_exception and span.set_status (the try/except block containing
span.record_exception(e) and span.set_status(Status(StatusCode.ERROR, str(e))))
to remove the high-cardinality description.
🪄 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: e9c48b76-5867-4baf-b9a7-913fd1fdcc92
📒 Files selected for processing (5)
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.toml
💤 Files with no reviewable changes (2)
- packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/async_qdrant_client_methods.json
- packages/opentelemetry-instrumentation-qdrant/opentelemetry/instrumentation/qdrant/qdrant_client_methods.json
… status - Add record_exception=False, set_status_on_exception=False to start_as_current_span so the manual try/except is the sole handler - Drop str(e) from Status(StatusCode.ERROR) to avoid sensitive data leaking into span status descriptions Addresses coderabbitai review feedback on traceloop#4004
Summary
Fixes #3492
upload_recordsfrom bothqdrant_client_methods.jsonandasync_qdrant_client_methods.json— this method was removed in qdrant-client 1.12 and caused anAttributeErroron instrumentation startupwrap_function_wrapperintry/except AttributeErrorso any future method removals in new qdrant-client releases degrade gracefully (logs a debug message and skips the method) instead of crashing the whole instrumentation<1.12to<2to allow testing against current qdrant-client releasesspan.record_exception()+StatusCode.ERRORto the wrapped call inwrapper.py(also addresses 🐛 Bug Report: Errors are not logged #412 for qdrant)Test plan
qdrant-client>=1.12and verifyQdrantInstrumentor().instrument()completes withoutAttributeErrorupsert,search,query, etc.) still produce spansnpx nx run opentelemetry-instrumentation-qdrant:testSummary by CodeRabbit
Bug Fixes
Chores