feat(milvus): Add error.type attribute from OpenTelemetry Semantic Conventions #3009
Merged
nirga merged 19 commits intotraceloop:mainfrom Jul 2, 2025
Merged
feat(milvus): Add error.type attribute from OpenTelemetry Semantic Conventions #3009nirga merged 19 commits intotraceloop:mainfrom
nirga merged 19 commits intotraceloop:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to d112361 in 1 minute and 30 seconds. Click for details.
- Reviewed
178lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1draft 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>
Contributor
Author
|
@nirga could you take a look at this PR ? |
nirga
reviewed
Jun 28, 2025
| or to_wrap.get("method") == "hybrid_search" | ||
| ): | ||
| _add_search_result_events(span, return_value) | ||
| except Exception as e: |
Member
There was a problem hiding this comment.
you should also set the span status as error
Contributor
Author
There was a problem hiding this comment.
@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
nirga
approved these changes
Jul 2, 2025
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 <nirga@users.noreply.github.com>
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 <nirga@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3008
feat(instrumentation): ...orfix(instrumentation): ....Important
Adds
error.typeattribute to Milvus spans for better error tracking, with tests for invalid vector search.error.typeattribute to spans inwrapper.pyusingERROR_TYPEfrom OpenTelemetry Semantic Conventions.code_to_error_typedictionary._wrap()to seterror.typeon exceptions.test_error.pyto testerror.typeattribute setting during a failed search operation.error.typeis set tointernal_errorfor invalid vector search.This description was created by
for d112361. You can customize this summary. It will automatically update as commits are pushed.