Skip to content

Conversation

divyapathak24
Copy link
Contributor

@divyapathak24 divyapathak24 commented Jun 11, 2025

Fixes #3008

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

Important

Adds error.type attribute to Milvus spans for better error tracking, with tests for invalid vector search.

  • Behavior:
    • Adds error.type attribute to spans in wrapper.py using ERROR_TYPE from OpenTelemetry Semantic Conventions.
    • Maps error codes to error types with code_to_error_type dictionary.
    • Updates _wrap() to set error.type on exceptions.
  • Testing:
    • Adds test_error.py to test error.type attribute setting during a failed search operation.
    • Verifies error.type is set to internal_error for invalid vector search.

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

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.

Caution

Changes requested ❌

Reviewed everything up to d112361 in 1 minute and 30 seconds. Click for details.
  • Reviewed 178 lines of code in 2 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/tests/test_error.py:87
  • Draft comment:
    The test verifies that error.type is set to 'internal_error', which works if the exception has a matching code. Consider adding tests for exceptions both with and without a 'code' attribute to fully validate the fallback behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment suggests testing error handling more thoroughly, which is generally good practice. However, without seeing the actual error handling code being tested or understanding what the 'code' attribute refers to, we can't be certain this suggestion is relevant or actionable. The test file is new, so we're seeing all changes. I don't have visibility into the actual error handling code being tested, so I can't be sure if testing for a 'code' attribute is even applicable. The comment might be making assumptions about implementation details we can't verify. While better error test coverage could be valuable, without seeing the implementation code, we can't be confident this specific suggestion is relevant or actionable. The comment should be deleted as it makes assumptions about implementation details we can't verify from the test file alone, and thus may not be actionable or relevant.

Workflow ID: wflow_L40sXdEABayPmKJG

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

divyapathak24 and others added 3 commits June 11, 2025 20:36
…strumentation/milvus/wrapper.py

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@divyapathak24
Copy link
Contributor Author

@nirga could you take a look at this PR ?

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.

Left a small comment

or to_wrap.get("method") == "hybrid_search"
):
_add_search_result_events(span, return_value)
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

you should also set the span status as error

Copy link
Contributor Author

@divyapathak24 divyapathak24 Jun 29, 2025

Choose a reason for hiding this comment

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

@nirga I have raised the exception at line 85 in wrapper.py, which automatically sets the span status to "error"—as shown in the attached screenshot as well.

Screenshot 2025-06-29 at 9 04 33 AM

I have also added a test to verify that the span status is correctly set to "error" line 88 in test_error.py

cursor[bot]

This comment was marked as outdated.

@nirga nirga merged commit 8148ca0 into traceloop:main Jul 2, 2025
10 checks passed
amitalokbera pushed a commit to amitalokbera/openllmetry that referenced this pull request Jul 15, 2025
…nventions (traceloop#3009)

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: Nir Gazit <[email protected]>
nina-kollman pushed a commit that referenced this pull request Aug 11, 2025
…nventions (#3009)

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: Nir Gazit <[email protected]>
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.

🚀 Feature: Add error.type Tag from OpenTelemetry Semantic Conventions
2 participants