Skip to content

Conversation

ronensc
Copy link
Collaborator

@ronensc ronensc commented Aug 25, 2025

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

This PR fixes an initialization error in the Milvus instrumentor that occurs when metrics are disabled:

Error initializing Milvus instrumentor: cannot access local variable 'query_duration_metric' where it is not associated with a value

Important

Fixes initialization error in MilvusInstrumentor by setting default metric values when metrics are disabled.

  • Behavior:
    • Fixes initialization error in MilvusInstrumentor when metrics are disabled by setting default values for query_duration_metric, distance_metric, insert_units_metric, upsert_units_metric, and delete_units_metric in _instrument().
    • Prevents error: "cannot access local variable 'query_duration_metric' where it is not associated with a value".

This description was created by Ellipsis for 6c6f51c. You can customize this summary. It will automatically update as commits are pushed.

Summary by CodeRabbit

  • Bug Fixes
    • Improved stability when metrics collection is disabled by safely initializing metric-related fields, preventing rare runtime errors during Milvus instrumentation.
    • Ensures instrumentation operates reliably whether metrics are enabled or not, with no changes to public behavior or configuration.

Copy link

coderabbitai bot commented Aug 25, 2025

Walkthrough

Initializes 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

Cohort / File(s) Summary
Milvus instrumentor metrics initialization
packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py
Predefine metric locals (query_duration_metric, distance_metric, insert_units_metric, upsert_units_metric, delete_units_metric) as None; assign instruments only when metrics are enabled; update wrapper invocation accordingly; add comment.

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
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • fix(milvus): Add metrics support #3013: Adjusts MilvusInstrumentor._instrument to conditionally create and pass metric instruments, closely aligned with initializing metrics to None when disabled.

Suggested reviewers

  • adharshctr
  • nirga

Poem

In burrows deep I count each call,
Metrics on or none at all—
I stash my Nones like carrots neat,
Then wrap the methods, clean and sweet.
Hop, hop! No unbound fright—
Patch proceeds by day or night. 🥕✨

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch milvus-fix-undefined-vars

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 1 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% <= threshold 50% 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@coderabbitai coderabbitai bot left a 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 analysis

Annotating 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] = None

If Counter/Histogram types aren’t exported in your pinned OpenTelemetry version, fall back to Optional[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 guidance

Using 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d71b7aa and 6c6f51c.

📒 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 off

Initializing 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 confirmed

All metric usages in _wrap are safe when any of the metric arguments is None:

• Query duration

  • Guarded by if duration > 0 and query_duration_metric and method == "query" before calling query_duration_metric.record(...).
    • Distance metric
  • In set_search_response, it checks if distance_metric and distance is not None before distance_metric.record(...).
    • Insert/Upsert/Delete units
  • Invoked via _set_response_attributes, which is decorated with @dont_throw. Any .add() call on a None metric will raise internally but be caught and suppressed by the decorator, preventing an AttributeError from escaping _wrap.

No changes needed to guard further—_wrap preserves correct behavior when metrics are disabled.

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nirga nirga merged commit 8ab6a34 into main Aug 25, 2025
9 checks passed
@nirga nirga deleted the milvus-fix-undefined-vars branch August 25, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants