fix(milvus): Enhanced Milvus VectorDB Instrumentation for Improved search Monitoring #2815
Conversation
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 3fe4f70 in 3 minutes and 16 seconds
More details
- Looked at
454lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:313
- Draft comment:
BUG: In function _add_search_result_events, the variable 'query_match_ids' used in set_global_stats is not defined in the outer scope. It should be aggregated outside the loop or passed appropriately. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:222
- Draft comment:
Potential Issue: The _encode_partition_name function is used on 'partition_names', which might be a list. Consider encoding lists appropriately if multiple partition names are expected. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The comment points out a potential issue but doesn't provide strong evidence that it's actually a problem. The str() function works fine on lists, converting them to a readable string representation. There's no evidence this is causing any issues. The code already handles partition_names as a potential list elsewhere (line 216 uses count_or_none on it).
The comment could be right that this wasn't the intended behavior - maybe partition names should be encoded differently than just str(). But without more context about requirements or bugs, we can't be sure.
While the encoding could potentially be done differently, the current implementation is not clearly wrong - str() is a reasonable way to encode both single values and lists for logging/tracing purposes.
The comment raises a speculative concern without clear evidence of an actual problem. The current implementation appears functional.
3. packages/opentelemetry-instrumentation-milvus/tests/test_search.py:103
- Draft comment:
Note: The tests assert string representations for list attributes (e.g. '[5]' for query vector dimensions). Ensure that this string conversion is intended and documented. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/wrapper.py:313
- Draft comment:
The variable 'query_match_ids' is used in set_global_stats without being defined in an outer scope. In a single-query scenario it relies on the loop variable. Consider initializing and aggregating match IDs outside the loop for clarity and to prevent potential issues if no results are returned. - 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%
While technically the variable is defined in a narrower scope than where it's used, the code flow ensures it will always be defined before use. The single_query case only happens when len(kwargs) == 1, meaning we'll always go through the loop exactly once before calling set_global_stats(). The code is actually safe as written.
The reviewer raises a valid point about code clarity and potential fragility. If the code structure changes in the future, this implicit dependency could become problematic.
However, the current implementation is logically sound and the variable will always be defined when needed. The suggested change would make the code more verbose without adding real safety.
The comment should be deleted as it points out a theoretical issue that isn't actually a problem in practice, and the suggested change would not meaningfully improve the code.
5. packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py:25
- Draft comment:
There seems to be an inconsistency in the naming of the Pinecone usage unit keys. 'PINECONE_DB_USAGE_READ_UNITS' uses the dot notation in its value ('db.pinecone.usage.read_units'), but 'PINECONE_DB_USAGE_WRITE_UNITS' uses an underscore ('db.pinecone.usage_write_units'). Consider updating it to 'db.pinecone.usage.write_units' for consistency. - 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.
Workflow ID: wflow_5CBtqlqXvBq3Y90S
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| span, AISpanAttributes.MILVUS_SEARCH_ANNS_FIELD, kwargs.get("anns_field") | ||
| ) | ||
| _set_span_attribute( | ||
| span, AISpanAttributes.MILVUS_SEARCH_PARTITION_NAMES, _encode_partition_name(kwargs.get("partition_names")) |
There was a problem hiding this comment.
The _encode_partition_name function is used to encode partition_names, which may be a list. Consider enhancing this helper to handle list inputs appropriately if that's expected.
| ) | ||
| _set_span_attribute( | ||
| span, | ||
| AISpanAttributes.MILVUS_SEARCH_RADIUS, |
There was a problem hiding this comment.
Consider storing kwargs.get("search_params") in a local variable to avoid redundant calls and improve readability when extracting radius, metric_type, and index_type.
packages/opentelemetry-instrumentation-milvus/tests/test_search.py
Outdated
Show resolved
Hide resolved
nirga
left a comment
nirga
left a comment
There was a problem hiding this comment.
Thanks @divyapathak24! Can you fix the broken CI?
Sure. I will fix them @nirga |
|
@nirga I think the correct approach is to first add the new semantic conventions-ai and publish a new version of the package (say 0.4.4). Only after that can I safely use those attributes in Milvus—otherwise, the tests will fail because the new attributes won’t be available to milvus yet right? |
galkleinman
left a comment
galkleinman
left a comment
There was a problem hiding this comment.
Hey @divyapathak24,
Thanks for submitting this one! please look at my comments - if you agree with them, please adjust (all occurrences of the things i mentioned, i didn't comment on all of them), otherwise let's discuss it here :)
| _set_span_attribute( | ||
| span, | ||
| AISpanAttributes.MILVUS_SEARCH_DURATION_IN_MS, | ||
| ((end_time - start_time) * 1000), | ||
| ) | ||
| _set_span_attribute( | ||
| span, AISpanAttributes.MILVUS_SEARCH_RESULT_STATUS, "success" | ||
| ) |
There was a problem hiding this comment.
wondering wether these attrs are needed... the search time (the way it's being calculated here) can be inferred implicitly by the start and end time of the span.
for the MILVUS_SEARCH_RESULT_STATUS, wondering if we need it, because it's sort of const here and because we can just use the generic "span.error"/"span.status_code" to report errors.
| if kwargs.get("search_params") and "radius" in kwargs.get("search_params") | ||
| else None |
| if kwargs.get("search_params") | ||
| and "metric_type" in kwargs.get("search_params") | ||
| else None |
| MILVUS_SEARCH_INDEX_TYPE = "db.milvus.search.index_type" | ||
| MILVUS_SEARCH_TIMEOUT = "db.milvus.search.timeout" | ||
| MILVUS_SEARCH_RESULT_COUNT = "db.milvus.search.result_count" | ||
| MILVUS_SEARCH_RESULT_STATUS = "db.milvus.search.status" |
There was a problem hiding this comment.
Just to confirm, do you mean remove these lines entirely and prepare a separate PR on semconv_ai ?
| MILVUS_SEARCH_RESULT_MIN_DISTANCE = "db.milvus.search.result_min_distance" | ||
| MILVUS_SEARCH_RESULT_MAX_DISTANCE = "db.milvus.search.result_max_distance" | ||
| MILVUS_SEARCH_RESULT_AVG_DISTANCE = "db.milvus.search.result_avg_distance" | ||
| MILVUS_SEARCH_DURATION_IN_MS = "db.milvus.search.duration_ms" |
Hi @galkleinman |
Hey again @divyapathak24, It isn't span internal, an no additional post-processing is needed, it's just the way OTEL works afaik... @nirga wdyt? |
Hi @galkleinman @nirga, Thanks for the feedback! I will remove the redundant attributes. Just wanted to confirm if everything else looks good to you both. Let me know if you have any other suggestions for other attributes. |
|
@nirga @galkleinman Can you please review the changes? I have removed redundant attributes and pushed changes and updated test cases accordingly. |
|
Thanks @divyapathak24 - can you sign the CLA? - #2815 (comment) |
|
@nirga done. Can you re-trigger the build? |
@nirga @galkleinman It seems my test cases are failing as it is not able to find the newly added attributes in semconv ai. How do I resolve this? By changing semconv-ai package version from 0.4.3 to 0.4.4 ? |
|
Yes @divyapathak24 :) |
|
@nirga @galkleinman I have opened another PR #2883 for semconv version update which has updated search attributes. |
…arch Monitoring (#2815)
4 participants
Fixes #2808
feat(instrumentation): ...orfix(instrumentation): ....New search attributes for single-vector searches
New search attributes for multi-vector searches
Search Events to log results:
Important
Enhance Milvus VectorDB instrumentation with detailed search monitoring attributes and events, and add tests for single and multi-vector searches.
_wrap()inwrapper.py._add_search_result_events()inwrapper.py.MILVUS_SEARCH_RADIUS,MILVUS_SEARCH_METRIC_TYPE, andMILVUS_SEARCH_INDEX_TYPEinsemconv_ai/__init__.py.test_search.pyto test single and multi-vector searches, verifying span attributes and events.timemodule inwrapper.pyfor measuring search duration.This description was created by
for 3fe4f70. It will automatically update as commits are pushed.