-
Notifications
You must be signed in to change notification settings - Fork 807
fix(milvus): Set default values when metrics are disabled #3344
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
Conversation
WalkthroughInitializes metric-related locals to None in MilvusInstrumentor._instrument before checking is_metrics_enabled(), then conditionally assigns real metric instruments if enabled. Passes these variables to the wrapper regardless of metrics state. Adds a clarifying comment. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant MilvusInstrumentor
participant MetricsProvider as Metrics Provider
participant Wrapper as Method Wrapper
App->>MilvusInstrumentor: _instrument()
rect rgba(200,220,255,0.2)
note over MilvusInstrumentor: Initialize metric locals = None
end
MilvusInstrumentor->>MilvusInstrumentor: is_metrics_enabled()?
alt Metrics enabled
MilvusInstrumentor->>MetricsProvider: create/get metric instruments
MetricsProvider-->>MilvusInstrumentor: metric objects
else Metrics disabled
note over MilvusInstrumentor: Metrics remain None
end
MilvusInstrumentor->>Wrapper: _wrap(..., metrics or None...)
Wrapper-->>App: Patched methods operational
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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 6c6f51c in 31 seconds. Click for details.
- Reviewed
18
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft 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-milvus/opentelemetry/instrumentation/milvus/__init__.py:88
- Draft comment:
Initializing all metric variables to None is a good fix to prevent uninitialized variable errors when metrics are disabled. Ensure _wrap handles None values correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment suggests a good practice of initializing variables to prevent errors, which is a valid point. However, it also includes a suggestion to ensure that a specific function handles None values correctly, which is a form of asking the author to double-check something. This part of the comment violates the rules.
Workflow ID: wflow_6yrQ8O29LnYQmrCT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 (2)
packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py (2)
6-6
: Optional: add precise type hints for metrics to aid static analysisAnnotating these locals clarifies intent and helps type checkers without changing runtime behavior.
Apply:
-from typing import Collection +from typing import Collection, Optional @@ -from opentelemetry.metrics import get_meter +from opentelemetry.metrics import get_meter, Counter, Histogram @@ - query_duration_metric = None - distance_metric = None - insert_units_metric = None - upsert_units_metric = None - delete_units_metric = None + query_duration_metric: Optional[Histogram] = None + distance_metric: Optional[Histogram] = None + insert_units_metric: Optional[Counter] = None + upsert_units_metric: Optional[Counter] = None + delete_units_metric: Optional[Counter] = NoneIf
Counter
/Histogram
types aren’t exported in your pinned OpenTelemetry version, fall back toOptional[object]
or introduce local Protocols—ping me and I’ll adjust accordingly.Also applies to: 9-9, 88-94
109-123
: Nit: consider unit "1" for dimensionless counters per OTel guidanceUsing unit "1" (dimensionless) instead of empty string improves semantic clarity and aligns with UCUM/OTel conventions, while typically remaining exporter-safe. Treat this as low-risk/optional.
insert_units_metric = meter.create_counter( Meters.DB_USAGE_INSERT_UNITS, - "", + "1", "Number of insert units consumed in serverless calls", ) upsert_units_metric = meter.create_counter( Meters.DB_USAGE_UPSERT_UNITS, - "", + "1", "Number of upsert units consumed in serverless calls", ) delete_units_metric = meter.create_counter( Meters.DB_USAGE_DELETE_UNITS, - "", + "1", "Number of delete units consumed in serverless calls", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.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-milvus/opentelemetry/instrumentation/milvus/__init__.py
⏰ 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: Lint
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.10)
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py (2)
88-94
: Good defensive init to avoid UnboundLocalError when metrics are offInitializing the metric locals prevents the runtime error described in the PR when
is_metrics_enabled()
is false. This is a minimal, targeted fix with no behavior change when metrics are enabled.
136-144
: wrap metric None-safety confirmedAll metric usages in
_wrap
are safe when any of the metric arguments isNone
:• Query duration
- Guarded by
if duration > 0 and query_duration_metric and method == "query"
before callingquery_duration_metric.record(...)
.
• Distance metric- In
set_search_response
, it checksif distance_metric and distance is not None
beforedistance_metric.record(...)
.
• Insert/Upsert/Delete units- Invoked via
_set_response_attributes
, which is decorated with@dont_throw
. Any.add()
call on aNone
metric will raise internally but be caught and suppressed by the decorator, preventing anAttributeError
from escaping_wrap
.No changes needed to guard further—
_wrap
preserves correct behavior when metrics are disabled.
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.
Thanks!
feat(instrumentation): ...
orfix(instrumentation): ...
.This PR fixes an initialization error in the Milvus instrumentor that occurs when metrics are disabled:
Important
Fixes initialization error in
MilvusInstrumentor
by setting default metric values when metrics are disabled.MilvusInstrumentor
when metrics are disabled by setting default values forquery_duration_metric
,distance_metric
,insert_units_metric
,upsert_units_metric
, anddelete_units_metric
in_instrument()
.This description was created by
for 6c6f51c. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit