-
Notifications
You must be signed in to change notification settings - Fork 801
feat(milvus): Add error.type attribute from OpenTelemetry Semantic Conventions #3009
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
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.
Caution
Changes requested ❌
Reviewed everything up to d112361 in 1 minute and 30 seconds. Click for details.
- Reviewed
178
lines of code in2
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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py
Outdated
Show resolved
Hide resolved
…strumentation/milvus/wrapper.py Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@nirga could you take a look at this PR ? |
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.
Left a small comment
or to_wrap.get("method") == "hybrid_search" | ||
): | ||
_add_search_result_events(span, return_value) | ||
except Exception as e: |
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.
you should also set the span status as error
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.
@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.

I have also added a test to verify that the span status is correctly set to "error" line 88 in test_error.py
…nventions (traceloop#3009) Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: Nir Gazit <[email protected]>
…nventions (#3009) Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: Nir Gazit <[email protected]>
Fixes #3008
feat(instrumentation): ...
orfix(instrumentation): ...
.Important
Adds
error.type
attribute to Milvus spans for better error tracking, with tests for invalid vector search.error.type
attribute to spans inwrapper.py
usingERROR_TYPE
from OpenTelemetry Semantic Conventions.code_to_error_type
dictionary._wrap()
to seterror.type
on exceptions.test_error.py
to testerror.type
attribute setting during a failed search operation.error.type
is set tointernal_error
for invalid vector search.This description was created by
for d112361. You can customize this summary. It will automatically update as commits are pushed.