feat: add Azure AI Search instrumentation#4317
Open
sarthak-env wants to merge 4 commits into
Open
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new ChangesAzure AI Search OpenTelemetry Instrumentation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant _wrap
participant SearchClient
participant OTelSpan
Caller->>_wrap: invoke wrapped SearchClient method
_wrap->>OTelSpan: start CLIENT span (db.system="azure_search")
_wrap->>_wrap: _set_input_attributes(span, instance, args, kwargs)
_wrap->>SearchClient: call original method(*args, **kwargs)
alt success (write operation)
SearchClient-->>_wrap: IndexDocumentsBatch response
_wrap->>_wrap: _set_response_attributes → affected/succeeded counts
_wrap->>OTelSpan: set duration, StatusCode.OK
_wrap-->>Caller: response
else success (search)
SearchClient-->>_wrap: SearchItemPaged iterator
_wrap->>OTelSpan: set duration, StatusCode.OK
_wrap-->>Caller: iterator
else exception raised
SearchClient-->>_wrap: raise Exception
_wrap->>OTelSpan: record_exception, set error.type, StatusCode.ERROR
_wrap-->>Caller: re-raise Exception
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-azure-search/tests/test_documents.py (1)
26-29: ⚡ Quick winRemove blanket exception swallowing in success-path document tests.
Catching and ignoring all exceptions here can make failing behavior look green.
Proposed fix
def test_upload_documents_creates_span(exporter): client = _make_client() with patch("azure.core.pipeline.Pipeline.run", return_value=_mock_pipeline_response()): - try: - client.upload_documents(documents=[{"id": "1"}, {"id": "2"}]) - except Exception: - pass + client.upload_documents(documents=[{"id": "1"}, {"id": "2"}]) @@ def test_merge_documents_creates_span(exporter): client = _make_client() with patch("azure.core.pipeline.Pipeline.run", return_value=_mock_pipeline_response()): - try: - client.merge_documents(documents=[{"id": "1", "title": "updated"}]) - except Exception: - pass + client.merge_documents(documents=[{"id": "1", "title": "updated"}])Also applies to: 39-42
🤖 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-azure-search/tests/test_documents.py` around lines 26 - 29, The try-except block in the test for client.upload_documents is catching and ignoring all exceptions without any assertion or verification, which masks test failures and makes them appear to pass. Since this is a success-path test, remove the blanket exception handling entirely so that any unexpected errors will properly fail the test. Apply this same fix to the other occurrence mentioned at lines 39-42 where the same pattern appears.packages/opentelemetry-instrumentation-azure-search/tests/test_search.py (1)
15-19: ⚡ Quick winAvoid
except Exception: passin tests; assert expected outcomes explicitly.These blocks can hide real regressions by allowing unexpected failures to pass silently.
Proposed fix
client = _make_client() with patch("azure.core.pipeline.Pipeline.run", return_value=MagicMock(http_response=MagicMock(status_code=200, text=lambda encoding=None: '{"value": []}', headers={}))): - try: - client.search("hello world", top=5) - except Exception: - pass + client.search("hello world", top=5) @@ client._index_name = "my-index" with patch("azure.core.pipeline.Pipeline.run", return_value=MagicMock(http_response=MagicMock(status_code=200, text=lambda encoding=None: '{"value": []}', headers={}))): - try: - client.search("test query", top=10) - except Exception: - pass + client.search("test query", top=10) @@ client = _make_client() with patch("azure.core.pipeline.Pipeline.run", side_effect=Exception("Service unavailable")): - try: - results = client.search("query") - list(results) - except Exception: - pass + with pytest.raises(Exception, match="Service unavailable"): + list(client.search("query"))Also applies to: 30-33, 43-47
🤖 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-azure-search/tests/test_search.py` around lines 15 - 19, The bare except Exception: pass block in the client.search call silently swallows any exceptions without asserting expected outcomes, which can hide regressions. Remove the try/except block around the client.search("hello world", top=5) call and instead add an explicit assertion to verify the expected result. Since the mock is configured to return a successful response with status_code=200, assert on the actual return value or behavior of the search method rather than catching and ignoring exceptions.
🤖 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.
Inline comments:
In
`@packages/opentelemetry-instrumentation-azure-search/opentelemetry/instrumentation/azure_search/__init__.py`:
- Around line 74-85: The _set_response_attributes function is materializing the
response to a new list on line 78 with `results = list(response)` and then
returning that materialized list on line 82, which breaks the SDK's documented
return type. Instead, extract the needed attributes by iterating directly over
the response object without materializing it into a separate list variable, and
remove the explicit return statement on line 82 so the function returns the
original response object unchanged at the end.
In
`@packages/opentelemetry-instrumentation-azure-search/opentelemetry/instrumentation/azure_search/version.py`:
- Line 1: The version.py file is missing a trailing newline at the end, which
violates Ruff rule W292. Add a newline character at the end of the file after
the __version__ variable assignment to satisfy this lint requirement and ensure
the file ends with a proper newline as per PEP 8 conventions.
In `@packages/opentelemetry-instrumentation-azure-search/tests/conftest.py`:
- Around line 9-16: The exporter fixture in conftest.py instruments the
AzureSearchInstrumentor globally but never uninstruments it, causing wrappers to
persist across the test session and contaminate unrelated tests. Refactor the
exporter fixture to use yield instead of return, yielding the exporter object,
and then add cleanup code after the yield that calls uninstrument() on the
AzureSearchInstrumentor instance to properly clean up the instrumentation when
the session ends.
---
Nitpick comments:
In `@packages/opentelemetry-instrumentation-azure-search/tests/test_documents.py`:
- Around line 26-29: The try-except block in the test for
client.upload_documents is catching and ignoring all exceptions without any
assertion or verification, which masks test failures and makes them appear to
pass. Since this is a success-path test, remove the blanket exception handling
entirely so that any unexpected errors will properly fail the test. Apply this
same fix to the other occurrence mentioned at lines 39-42 where the same pattern
appears.
In `@packages/opentelemetry-instrumentation-azure-search/tests/test_search.py`:
- Around line 15-19: The bare except Exception: pass block in the client.search
call silently swallows any exceptions without asserting expected outcomes, which
can hide regressions. Remove the try/except block around the
client.search("hello world", top=5) call and instead add an explicit assertion
to verify the expected result. Since the mock is configured to return a
successful response with status_code=200, assert on the actual return value or
behavior of the search method rather than catching and ignoring exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2a39584-c8f4-4777-b271-0805947e53df
📒 Files selected for processing (8)
packages/opentelemetry-instrumentation-azure-search/README.mdpackages/opentelemetry-instrumentation-azure-search/opentelemetry/instrumentation/azure_search/__init__.pypackages/opentelemetry-instrumentation-azure-search/opentelemetry/instrumentation/azure_search/version.pypackages/opentelemetry-instrumentation-azure-search/poetry.tomlpackages/opentelemetry-instrumentation-azure-search/pyproject.tomlpackages/opentelemetry-instrumentation-azure-search/tests/conftest.pypackages/opentelemetry-instrumentation-azure-search/tests/test_documents.pypackages/opentelemetry-instrumentation-azure-search/tests/test_search.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In
`@packages/opentelemetry-instrumentation-azure-search/opentelemetry/instrumentation/azure_search/__init__.py`:
- Around line 121-123: The span status on lines 121-123 is unconditionally set
to OK after calling _set_response_attributes, which masks partial indexing
failures in batch operations. Update the _set_response_attributes function to
return the count of succeeded and affected documents from the IndexingResult
entries in the response. Then, after calling _set_response_attributes, check if
succeeded is less than affected; if so, set the span status to ERROR using
Status(StatusCode.ERROR) instead of unconditionally setting OK, ensuring partial
failures in batch operations like upload_documents, merge_documents,
merge_or_upload_documents, and delete_documents are properly reflected in
observability.
In `@packages/opentelemetry-instrumentation-azure-search/test_trace.py`:
- Line 26: The file test_trace.py is missing a trailing newline at the end of
the file after the print("Done") statement. Add a newline character at the very
end of the file, after the final print("Done") line, to comply with the file
formatting standard (Ruff W292).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8056060-bd68-4177-b01f-3aa713a2e373
📒 Files selected for processing (3)
packages/opentelemetry-instrumentation-azure-search/opentelemetry/instrumentation/azure_search/__init__.pypackages/opentelemetry-instrumentation-azure-search/test_trace.pypackages/opentelemetry-instrumentation-azure-search/tests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opentelemetry-instrumentation-azure-search/tests/conftest.py
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
2 participants
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.
Closes #2303
What this PR does
Adds a new instrumentation package for Azure AI Search (
azure-search-documentsSDK).Package:
opentelemetry-instrumentation-azure-searchInstrumented methods
SearchClient.search()SearchClient.upload_documents()SearchClient.merge_documents()SearchClient.merge_or_upload_documents()SearchClient.delete_documents()Span attributes captured
db.system— alwaysazure_searchserver.address— endpoint URLazure_search.index_name— index being queriedazure_search.search_text— query stringazure_search.top— max results requestedazure_search.filter— OData filter expressionazure_search.duration— operation duration in secondsazure_search.affected_documents— documents in write operationsazure_search.succeeded_documents— successfully written documentsTesting
6 tests covering search, upload, merge, and delete operations including error handling.
Notes
Follows the same pattern as
opentelemetry-instrumentation-pinecone.feat(instrumentation): ...orfix(instrumentation): ....Summary by CodeRabbit