fix(instrumentation): pinecone span attributes for upsert and delete (#2688) - #4084
fix(instrumentation): pinecone span attributes for upsert and delete (#2688)#4084WatchTree-19 wants to merge 3 commits into
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Pinecone span attributes for ChangesPinecone Instrumentation Expansion
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant _wrap as _wrap
participant query as set_query_input_attributes
participant upsert as set_upsert_input_attributes
participant delete as set_delete_input_attributes
_wrap->>_wrap: Read to_wrap.get("method")
alt query
_wrap->>query: Set query span attributes
else upsert
_wrap->>upsert: Set upsert span attributes
else delete
_wrap->>delete: Set delete span attributes
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.py (1)
171-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange
if response:guard toif response is not None:to ensure delete operations record metrics and status.When Pinecone's
delete()returns an empty dict{}, the current guardif response:evaluates toFalse, preventing:
span.set_status(Status(StatusCode.OK))from being called — delete spans remain with statusUNSET_set_response_attributes()from being called —write_unitsmetrics are never recordedThis defeats the observability improvements being added for the delete operation in this PR. Use an explicit
Nonecheck instead:Proposed fix
- if response: + if response is not None:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.py` around lines 171 - 186, The guard currently using "if response:" skips handling when Pinecone returns an empty dict (e.g., delete()); change the condition in the response handling block to "if response is not None:" so that _set_response_attributes(span, read_units_metric, write_units_metric, shared_attributes, response) and span.set_status(Status(StatusCode.OK)) (and the query-specific set_query_response call) run for empty responses as well; update the conditional in the function where response is checked (the block that calls set_query_response, _set_response_attributes, and span.set_status) to use an explicit None check.
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.py (1)
97-159: 🏗️ Heavy liftNo tests for the new handlers.
The two new input-attribute setters have no accompanying test coverage. For instrumentation code, at minimum there should be unit tests that pass mock spans and assert the expected attributes are set — covering keyword-arg calls, positional-arg fallback, generator inputs (no count),
delete_all, and filter serialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.py` around lines 97 - 159, Add unit tests for set_upsert_input_attributes and set_delete_input_attributes: create mock/span objects (or spy on set_span_attribute) and assert correct attributes are set for (1) keyword-arg invocation (vectors, ids) that records counts, (2) positional-arg fallback when kwargs missing, (3) generator or non-len iterable inputs where no count is recorded (ensure no exception), (4) delete_all is recorded via SpanAttributes.PINECONE_DELETE_DELETE_ALL, and (5) filter is serialized to JSON when a dict is provided and passed through when non-dict. Use the exact function names set_upsert_input_attributes and set_delete_input_attributes and assert against the SpanAttributes keys used in the functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.py`:
- Around line 171-186: The guard currently using "if response:" skips handling
when Pinecone returns an empty dict (e.g., delete()); change the condition in
the response handling block to "if response is not None:" so that
_set_response_attributes(span, read_units_metric, write_units_metric,
shared_attributes, response) and span.set_status(Status(StatusCode.OK)) (and the
query-specific set_query_response call) run for empty responses as well; update
the conditional in the function where response is checked (the block that calls
set_query_response, _set_response_attributes, and span.set_status) to use an
explicit None check.
---
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.py`:
- Around line 97-159: Add unit tests for set_upsert_input_attributes and
set_delete_input_attributes: create mock/span objects (or spy on
set_span_attribute) and assert correct attributes are set for (1) keyword-arg
invocation (vectors, ids) that records counts, (2) positional-arg fallback when
kwargs missing, (3) generator or non-len iterable inputs where no count is
recorded (ensure no exception), (4) delete_all is recorded via
SpanAttributes.PINECONE_DELETE_DELETE_ALL, and (5) filter is serialized to JSON
when a dict is provided and passed through when non-dict. Use the exact function
names set_upsert_input_attributes and set_delete_input_attributes and assert
against the SpanAttributes keys used in the functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f67658e5-082d-4846-9d45-f7464738fef0
📒 Files selected for processing (3)
packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.pypackages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.pypackages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
|
gentle bump on this when you get a chance. PR adds pinecone span attributes for upsert/delete following the chromadb pattern (#2688). CLA cleared and coderabbit-bot reviewed positively. happy to address any feedback or rebase if it's gone stale. |
Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com>
d57143d to
c51b9f6
Compare
|
rebased onto main and resolved the conflict in one thing worth a call: i kept the |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-pinecone/tests/test_input_attributes.py (1)
39-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding edge-case coverage.
Tests correctly validate the dict-filter path and positional-arg counting, but a few branches in the upstream handlers (
query_handlers.py) remain untested: non-dictfiltervalues (string filter fallback), non-len()-supportingids(TypeError/generator path for delete), and the__init__.pydispatchif/elifitself routing to the correct handler by method name.♻️ Suggested additional tests
+def test_delete_non_dict_filter_passed_through(): + span = MagicMock() + set_delete_input_attributes(span, {"filter": "raw-string-filter"}, ()) + assert _attrs(span)[SpanAttributes.PINECONE_DELETE_FILTER] == "raw-string-filter" + + +def test_delete_generator_ids_not_counted(): + span = MagicMock() + set_delete_input_attributes(span, {"ids": (x for x in range(3))}, ()) + assert SpanAttributes.PINECONE_DELETE_IDS_COUNT not in _attrs(span)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-pinecone/tests/test_input_attributes.py` around lines 39 - 56, Add edge-case tests for the Pinecone input attribute handlers: cover the string-filter fallback path in the delete/query logic, verify delete IDs counting when ids is an iterator or otherwise lacks len(), and add dispatch coverage in __init__.py to confirm the method-name if/elif routes to the correct handler. Use the existing set_delete_input_attributes, set_query_input_attributes, and the method-dispatch entrypoint symbols to keep the tests aligned with the upstream query_handlers.py behavior.packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.py (1)
125-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated len()/TypeError pattern across both new setters.
The "compute count, catch
TypeErrorfor non-len()-able iterables" logic is duplicated verbatim betweenset_upsert_input_attributesandset_delete_input_attributes. Consider extracting a small helper, e.g._safe_len(x), to avoid the repetition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.py` around lines 125 - 186, The len()/TypeError fallback for counting iterable inputs is duplicated in set_upsert_input_attributes and set_delete_input_attributes. Extract the repeated “safe count” logic into a small shared helper (for example, a private _safe_len-like utility) and use it from both functions, keeping the existing behavior for vectors and ids while reducing repetition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.py`:
- Around line 125-186: The len()/TypeError fallback for counting iterable inputs
is duplicated in set_upsert_input_attributes and set_delete_input_attributes.
Extract the repeated “safe count” logic into a small shared helper (for example,
a private _safe_len-like utility) and use it from both functions, keeping the
existing behavior for vectors and ids while reducing repetition.
In
`@packages/opentelemetry-instrumentation-pinecone/tests/test_input_attributes.py`:
- Around line 39-56: Add edge-case tests for the Pinecone input attribute
handlers: cover the string-filter fallback path in the delete/query logic,
verify delete IDs counting when ids is an iterator or otherwise lacks len(), and
add dispatch coverage in __init__.py to confirm the method-name if/elif routes
to the correct handler. Use the existing set_delete_input_attributes,
set_query_input_attributes, and the method-dispatch entrypoint symbols to keep
the tests aligned with the upstream query_handlers.py behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 407376fc-41e7-4815-b7ec-acd3540f4d7d
📒 Files selected for processing (4)
packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.pypackages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/query_handlers.pypackages/opentelemetry-instrumentation-pinecone/tests/test_input_attributes.pypackages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/init.py
- packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/init.py
a successful delete returns an empty dict, which the truthiness guard skipped; check 'is not None' so delete span attributes are still set. Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com>
|
thanks - addressed the coderabbit note: a successful pinecone delete returns an empty dict, which the truthiness guard skipped, so switched to still one open question for a maintainer when someone has a moment: i kept the |
bb01458 to
acac730
Compare
2 participants
feat(instrumentation): ...orfix(instrumentation): ....Fixes #2688
Pinecone instrumentation only set span attributes for
query().upsert()anddelete()produced spans with the bare vendor tag and nothing operation-specific.Following the pattern from
chromadb(_set_upsert_attributes,_set_delete_attributesin chromadb's wrapper), this PR adds:SpanAttributesconstants insemconv_ai:PINECONE_UPSERT_VECTORS_COUNT,PINECONE_UPSERT_NAMESPACE,PINECONE_UPSERT_BATCH_SIZEPINECONE_DELETE_IDS_COUNT,PINECONE_DELETE_NAMESPACE,PINECONE_DELETE_DELETE_ALL,PINECONE_DELETE_FILTERset_upsert_input_attributesandset_delete_input_attributesinquery_handlers.py. Both@dont_throw. Both checkkwargsthen fall back toargs[0]forvectors/idssince Pinecone callers usually pass them positionally.len()is wrapped intry / except TypeErrorso a generator-typedvectorsdoesn't get materialised just to count._wrapswitched to a smallif/elifonto_wrap.get("method").Did not touch response-side attributes. The existing
_set_response_attributesalready recordsread_units/write_unitsfor any response with ausageblock, which covers the metric side for upsert. Pure upsert/delete-specific response attributes (vector IDs returned, etc.) can be a follow-up.No tests added in this PR; cassette-based tests for the new setters would be a small follow-up. Happy to add if you'd like.
Summary by CodeRabbit
Noneresponses differently from other responses.