-
Notifications
You must be signed in to change notification settings - Fork 694
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
fix(milvus):Updated the instrumentation to collect get() and create_collection() span attributes #2687
base: main
Are you sure you want to change the base?
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.
👍 Looks good to me! Reviewed everything up to 07b1144 in 2 minutes and 31 seconds
More details
- Looked at
199
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py:22
- Draft comment:
Added wrapped method for create_collection. Verify that its parameters match those expected by the span attributes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that the parameters match those expected by the span attributes. This falls under the category of asking the author to double-check things, which is against the rules.
2. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:54
- Draft comment:
New branch for 'create_collection' in _wrap looks good; ensure extracted kwargs are sufficient for tracing. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that the extracted kwargs are sufficient for tracing. This is similar to asking the author to double-check or verify something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.
3. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:94
- Draft comment:
Implementation of _set_create_collection_attributes is straightforward; consider adding type validation if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
This is a telemetry function that's already wrapped in error handling (@dont_throw). Type validation would add complexity without clear benefit since: 1) it's just for metrics/tracing 2) .get() already safely handles missing values 3) _set_span_attribute appears to handle null/empty values 4) type issues would be caught by the actual Milvus API calls.
Perhaps type validation could catch issues earlier and provide better debugging info. The values being collected are important configuration parameters.
For telemetry code, adding type validation would be overkill - the goal is to collect metrics without impacting the main code path. The @dont_throw decorator indicates this is intentionally lenient.
The comment should be removed as type validation would add unnecessary complexity to telemetry code that's intentionally designed to be permissive.
4. packages/opentelemetry-instrumentation-milvus/tests/test_query.py:201
- Draft comment:
The test for create_collection covers expected attributes. Validate if additional parameters (e.g., auto_id, max_length) should also be captured. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The test is already verifying the most important parameters of create_collection. auto_id and max_length seem like secondary configuration parameters. Without knowing the OpenTelemetry instrumentation requirements, we can't be sure if these parameters need to be captured. The comment is asking for verification rather than making a clear case for why these parameters should be captured.
I might be wrong about the importance of auto_id and max_length - they could be critical configuration parameters that affect collection behavior significantly.
Even if these parameters are important, the comment is asking for verification rather than making a clear case for why they should be captured. It's a speculative "should we?" rather than a clear "we should because..."
The comment should be deleted because it's speculative and asks for verification rather than making a clear case for adding specific test coverage.
5. packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py:161
- Draft comment:
Renamed attributes (e.g., MILVUS_DELETE_TIMEOUT, MILVUS_GET_TIMEOUT) for consistency. Ensure updated names are reflected in all related instrumentation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py:22
- Draft comment:
New instrumentation entry for 'create_collection' is correctly added. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:94
- Draft comment:
The new _set_create_collection_attributes function is consistent with existing attribute setters. Consider adding type conversion/validation if non-string values (e.g. dimension or timeout) need normalization. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. packages/opentelemetry-instrumentation-milvus/tests/test_query.py:201
- Draft comment:
The new test for create_collection properly verifies span attributes. Note that extra parameters (e.g. auto_id, max_length) are passed but not instrumented; consider documenting this behavior if intentional. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
9. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:95
- Draft comment:
Typographical Issue: The function definition on line 95 has an extra space between 'def' and '_set_create_collection_attributes'. Please change 'def _set_create_collection_attributes' to 'def _set_create_collection_attributes'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
While the comment is technically correct, it's pointing out an extremely minor style issue. The extra space doesn't affect functionality or readability significantly. Most code formatters would automatically fix this. It's not a substantive issue that requires manual intervention.
The comment is factually accurate and provides a specific fix. Style consistency can be important for code maintainability.
While style consistency matters, this is too minor of an issue to warrant a PR comment. It would be better handled by automated tooling or linting.
The comment should be removed as it addresses an extremely minor style issue that doesn't meaningfully impact code quality or functionality.
Workflow ID: wflow_W4f2FwGoKyw61NAg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
fix for the issue #2686 |
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 @elinacse! Looks like tests are failing - can you take a look?
Fixes #2686
feat(instrumentation): ...
orfix(instrumentation): ...
.Important
Update Milvus instrumentation to collect span attributes for
create_collection
andget
methods, and add corresponding tests.create_collection
toWRAPPED_METHODS
in__init__.py
._set_create_collection_attributes()
inwrapper.py
to set span attributes forcreate_collection
._set_get_attributes()
inwrapper.py
to use new span attributes.create_collection
insemconv_ai/__init__.py
.get
insemconv_ai/__init__.py
.test_milvus_create_collection()
intest_query.py
to verifycreate_collection
spans.test_milvus_query_*
intest_query.py
to verifyget
spans.This description was created by
for 07b1144. It will automatically update as commits are pushed.