-
Notifications
You must be signed in to change notification settings - Fork 7
cohere v2 instrumentor #189
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
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.
Caution
Changes requested ❌
Reviewed everything up to f89e998 in 2 minutes and 26 seconds. Click for details.
- Reviewed
7413
lines of code in41
files - Skipped
0
files when reviewing. - Skipped posting
24
draft 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. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/cohere/event_emitter.py:97
- Draft comment:
Typo in comment: "... if it is the same as the in the event name." Consider rephrasing to "... if it is the same as in the event name." - 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 about the grammatical error, it's an extremely minor issue in an internal comment that doesn't affect code functionality. The meaning is still clear despite the typo. According to the rules, we should not make purely informative comments or comments that are unimportant. The typo could potentially confuse future developers reading the documentation. Documentation quality is important for maintainability. While documentation quality matters, this particular typo is so minor that it doesn't impact understanding. The rules specifically state not to make unimportant comments. Delete the comment as it points out an extremely minor documentation issue that doesn't impact code functionality or understanding.
2. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/cohere/utils.py:28
- Draft comment:
Typo in log message: 'OpenLLMetry' appears to be a typo. Consider changing it to 'OpenTelemetry' for consistency. - 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 name "OpenLLMetry" seems like it could be an intentional play on words, combining "LLM" with "Telemetry". Since this is a debug log message and the name might be intentional branding, changing it may not be necessary. Debug messages are primarily for developers, and this doesn't affect functionality. The comment is about something very minor that doesn't impact code quality or behavior. I might be overthinking the intentionality of the name. It could genuinely be a typo that should be fixed for consistency. Even if it is a typo, it's in a debug message that doesn't affect functionality, and the current form is still clear enough to understand the meaning. This comment should be deleted as it addresses a very minor cosmetic issue in a debug message that doesn't impact code functionality or clarity.
3. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_async.yaml:31
- Draft comment:
There appear to be typographical issues in the recorded response text: several contractions use double apostrophes (e.g., "here''s" instead of "here's", "blowin''", "it''ll", "We''ve"). Please review and confirm if these extra apostrophes are intended or if they should be corrected. - Reason this comment was not posted:
Comment was on unchanged code.
4. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_with_streaming.yaml:60
- Draft comment:
Typo: The token "''s" appears here. It looks like it should be a contraction (e.g. "here's") rather than a double apostrophe. - 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% This is a test cassette file that records actual API responses. The double apostrophe appears to be how the API actually returns the data. Modifying it would make the test cassette inaccurate to the real API behavior. The comment is suggesting we "fix" something that isn't actually broken - it's just recording the actual API response. Could the double apostrophe be causing issues in tests? Could this be a real bug in the API that should be reported? Even if it's not ideal, the test cassette should accurately reflect the API's actual behavior. If there's an issue with the API's response format, that should be handled separately from the test cassettes. Delete the comment. Test cassettes should record actual API responses exactly as they are, not how we think they should be formatted.
5. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_with_streaming.yaml:129
- Draft comment:
Typo: Found a token with double apostrophes "''" intended as part of a contraction (likely for "lootin'"). It might be a chunking error; consider using a single apostrophe. - 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% This is a test cassette file that records actual API responses. The double apostrophes are part of the real response from the Cohere API. We shouldn't try to modify or "fix" actual API responses in test cassettes, as that would make the tests inaccurate. The purpose of test cassettes is to capture real responses exactly as they are. Maybe the double apostrophes are causing issues in some test scenarios? Maybe there's a legitimate reason to normalize the response format? No, test cassettes should capture exact API responses. If there are issues with how the API formats apostrophes, that should be handled in the actual code, not by modifying test data. Delete this comment. We should not modify API responses in test cassettes as they need to accurately reflect what the API actually returns.
6. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_with_streaming.yaml:219
- Draft comment:
Typo: The contraction is rendered as "''ll". It should likely be "'ll" (as in "I'll"). - Reason this comment was not posted:
Comment looked like it was already resolved.
7. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_with_streaming_async.yaml:60
- Draft comment:
Typo: The text "''s" likely should use a single apostrophe ("'s"). This appears to be meant to form the contraction "here's" when concatenated with the preceding chunk. - 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% This is a test fixture file that records actual API responses. The double apostrophes are likely what the API actually returned, so changing them would make the test fixture inaccurate. The purpose of test fixtures is to capture real responses, not to make them prettier. Modifying the response would defeat the purpose of having recorded fixtures. Maybe the double apostrophes are actually a bug in the API that should be reported? Maybe there's a reason they use double apostrophes that we don't understand? Even if it is a bug in the API, the test fixture should accurately record what the API returns, not what we think it should return. Test fixtures are for capturing actual behavior, not ideal behavior. Delete the comment. Test fixtures should record actual API responses exactly as they are, not modify them to match our style preferences.
8. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_with_streaming_async.yaml:226
- Draft comment:
Typo: The text "''s" here seems to be a misplaced double apostrophe. It should likely be "'s" (i.e. "captain's") to preserve the correct contraction. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_legacy_with_streaming_async.yaml:276
- Draft comment:
Typo: The separate JSON chunk containing "''" in the contraction (from "want" + "in" + "''") appears incorrect. Consider using a single apostrophe ("'") so that it properly forms "wantin'". - 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% This is a test fixture file that captures real API responses. The chunked streaming response format is determined by the API, not by the code author. The suggestion to change how the contraction is formatted would require modifying actual API responses, which defeats the purpose of having accurate test fixtures. The current format, while perhaps not ideal, accurately represents how the API streams its response. Maybe the API's chunking behavior is actually configurable and could be improved? Maybe this formatting could cause issues in some test scenarios? Even if the chunking is configurable, test fixtures should capture the actual API behavior rather than an idealized version. The current format, while chunked oddly, still produces the correct final output when assembled. Delete this comment. The chunked format accurately represents the API's streaming behavior and shouldn't be modified in test fixtures.
10. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_with_events_with_content_async.yaml:31
- Draft comment:
Typo alert: The phrase "here''s" might be a mistake. Consider using "here's" or revising the quote escaping as needed. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_chat_with_events_with_content_async.yaml:33
- Draft comment:
Typo alert: "scowlin''" appears to have extra apostrophes. Did you mean "scowling" (or "scowlin'" if used colloquially)? - Reason this comment was not posted:
Comment was on unchanged code.
12. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_v2_chat_legacy_with_streaming.yaml:117
- Draft comment:
Typographical error: The text appears as "''s" which likely should be "'s" (a single apostrophe) in the word (e.g., "here's"). Please verify and correct if this wasn't intended. - 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% This is a test cassette file that records actual API responses. The double apostrophe is part of the actual response from the API. Changing it would make the test cassette not match the real API response. Test cassettes should record exactly what the API returns, not what we think it should return. Maybe the API response is actually incorrect and we should report this issue to the API provider? Even if the API response could be improved, the test cassette's job is to accurately record what the API returns, not to fix the API's responses. Modifying the recorded response would defeat the purpose of the test cassette. Delete this comment. Test cassettes should record the exact API response, even if that response contains unusual formatting.
13. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_v2_chat_legacy_with_streaming_async.yaml:117
- Draft comment:
Typo: The contraction appears as "''s". If the intended text is "here's", consider using a single apostrophe: "'s". - 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% Since this is a test fixture file recording actual API responses, we shouldn't modify the response data - that would defeat the purpose of the test fixture. The double apostrophe is part of the actual API response we're recording. Changing it would make the test fixture inaccurate. Test fixtures should record exactly what the API returned, even if it contains unusual formatting. Maybe the double apostrophe is actually a bug in the API that should be reported? Or maybe there's a specific reason the API uses double apostrophes that we're not aware of? Even if it is a bug in the API, this test fixture's job is to accurately record the API's behavior, not to fix it. The proper place to fix such issues would be in the API itself, not in test fixtures recording its behavior. Delete this comment. We should not modify API response data in test fixtures, as that would make the tests inaccurate. The double apostrophe may look unusual but it's part of the actual API response being recorded.
14. tests/test_instrumentations/test_cohere/cassettes/test_chat/test_cohere_v2_chat_legacy_with_streaming_async.yaml:436
- Draft comment:
Typo: The contraction appears as "''t" as part of "shan't". Consider using a single apostrophe ("'t") if that is intended. - 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% This is a test cassette file that records actual API responses. The double apostrophe is part of the actual response from the API - it's not a typo in our code, but rather a faithful recording of what the API returned. Making this change would make the test cassette inaccurate by modifying the actual API response. Maybe the API response is actually incorrect and we should report this as a bug to Cohere? Maybe we should normalize API responses in our test cassettes? No - test cassettes should record exactly what the API returns, warts and all. Modifying the response would defeat the purpose of having recorded tests. Delete this comment. The double apostrophe is part of the actual API response and should be preserved exactly as-is in the test cassette.
15. tests/test_instrumentations/test_cohere/cassettes/test_embed/test_cohere_v2_embed_legacy.yaml:4
- Draft comment:
Typo: There's a double space in the text 'At the 2010 United States Census' on line 4. Please remove the extra space. - 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 test fixture file containing sample data for testing. The extra space appears in test data that is being used to test the embedding functionality. The space doesn't affect the functionality being tested since this is just example text being embedded. The comment is technically correct but extremely minor and doesn't impact the actual test functionality. The space could potentially be intentional as part of the test data. Maybe it's testing how the embedding handles irregular spacing. Looking at the response data, the extra space is normalized away in the returned embeddings text anyway, so it clearly doesn't matter for the test. Delete this comment. It points out an extremely minor formatting issue in test data that has no impact on functionality.
16. tests/test_instrumentations/test_cohere/cassettes/test_embed/test_cohere_v2_embed_legacy_async.yaml:4
- Draft comment:
Typographical error: Detected extra space in the phrase "At the 2010 United States Census". Consider using a single space. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and points out a typographical error. It doesn't provide a suggestion for code improvement or ask for a specific action related to the code's functionality. Therefore, it should be removed according to the rules.
17. tests/test_instrumentations/test_cohere/cassettes/test_rerank/test_cohere_rerank_legacy.yaml:4
- Draft comment:
Typo: There appears to be an extra space in the string "At the 2010 United States Census". Consider removing the extra space between 'the' and '2010'. - 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 test fixture file that records API interactions for testing. The extra space is in test data that's likely captured from a real API call. Modifying this could make the test data less accurate to the real interaction. Test fixtures should generally match the real data exactly, even if it contains imperfections. The extra space could be a real bug in the system being tested, and fixing it in the test data could hide that bug. Test fixtures should be treated as read-only snapshots of real interactions - modifying them for style defeats their purpose of being accurate recordings. Delete this comment. Test fixtures should preserve the exact data from recorded interactions, including any imperfections.
18. tests/test_instrumentations/test_cohere/cassettes/test_rerank/test_cohere_rerank_with_events_with_content.yaml:4
- Draft comment:
Typographical error: There appears to be an extra space in the phrase "At the 2010 United States Census". Consider removing one of the spaces. - 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 test fixture file that contains recorded API interactions. The extra space in the test data is likely irrelevant to the functionality being tested. The space is in test data that's being sent to an API for reranking, where exact spacing won't affect the semantic meaning or test outcomes. Making purely cosmetic changes to test fixtures could actually make them more brittle. The extra space could potentially indicate a copy-paste error that should be fixed for cleanliness. Test data should ideally be clean and well-formatted. While clean test data is good, this extra space has no functional impact and fixing it provides no real value. The test is about API interaction and reranking functionality, not about string formatting. Delete this comment as it's purely cosmetic, doesn't affect functionality, and isn't worth the effort to fix in a test fixture file.
19. tests/test_instrumentations/test_cohere/cassettes/test_rerank/test_cohere_rerank_with_events_with_no_content.yaml:4
- Draft comment:
There appears to be an extra whitespace between 'the' and '2010' in the sentence "At the 2010 United States Census...". Please remove the double space. - 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 test fixture file that records API interactions for testing purposes. The extra space is in test data that's being sent to the API. Since this is test data, the exact formatting (including spaces) might be intentional or copied from real examples. The space doesn't affect functionality and this is just test data. The extra space could potentially cause subtle issues in tests if the exact string matching is important. Maybe the test is specifically checking formatting? This is a rerank API test that cares about semantic meaning, not exact string formatting. The extra space won't affect the test results. The comment should be deleted as it's pointing out a trivial formatting issue in test fixture data that doesn't impact functionality.
20. tests/test_instrumentations/test_cohere/cassettes/test_rerank/test_cohere_v2_rerank_legacy.yaml:5
- Draft comment:
There appears to be extra whitespace in the text: "At the 2010 United States Census" on this line. Consider removing one of the spaces. - 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 test data that's being sent to an API. The extra space is likely irrelevant to the test's functionality. The space is part of a string that's being used to test reranking functionality - the exact formatting of this test data doesn't impact the test's purpose. Making purely cosmetic changes to test fixtures could actually make the tests less valuable as real-world test cases. Maybe the extra space could cause subtle issues with the reranking algorithm's scoring? Maybe consistency in test data is important for maintainability? The reranking API is clearly working fine with this input as shown by the response. Test data should represent real-world scenarios, where text isn't always perfectly formatted. This comment should be deleted as it suggests a purely cosmetic change to test fixture data that doesn't impact functionality.
21. tests/test_instrumentations/test_cohere/cassettes/test_rerank/test_cohere_v2_rerank_legacy_async.yaml:5
- Draft comment:
Typo: Detected double space in the phrase "At the 2010 United States Census". Please remove the extra space between "the" and "2010". - 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 test fixture file that contains recorded API interactions. The double space is in test data that's being sent to the API. Since this is test data, minor formatting issues like double spaces don't affect functionality. The data appears to be working fine in the tests since this is a recorded successful interaction. Making this change would be purely cosmetic and doesn't affect the test's purpose. The double space could potentially make the test data less realistic compared to real-world usage. It might be worth fixing for consistency. While consistency is good, this is just test data and the extra space doesn't impact the test's functionality or readability. Making purely cosmetic changes to test fixtures isn't worth the effort. Delete this comment as it's pointing out a minor formatting issue in test data that doesn't affect functionality.
22. tests/test_instrumentations/test_cohere/test_embed.py:12
- Draft comment:
Typo: There is a double space before the year '2010' in this string. Consider correcting "At the 2010 United States Census" to "At the 2010 United States Census". - 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 test file and the strings are just sample data being used to test the embedding functionality. The double space is purely cosmetic and has no impact on the functionality being tested. The tests are checking the behavior of the embedding client and the telemetry/instrumentation, not the exact formatting of the test strings. The comment is technically correct but not meaningful. The double space could theoretically affect the embedding results in some subtle way. Also, maintaining consistent formatting in test data could help readability. The embedding model is designed to be robust to minor spacing variations, and this is test data where exact formatting is not critical to the test's purpose. The readability impact is minimal. The comment should be deleted as it's pointing out a trivial formatting issue in test data that doesn't affect the functionality being tested.
23. tests/test_instrumentations/test_cohere/test_embed.py:71
- Draft comment:
Typo: There is a double space before the year '2010' in this string. Consider correcting "At the 2010 United States Census" to "At the 2010 United States Census". - 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 test file and the text strings are just example data being used to test the embedding functionality. The double space is inconsequential to the test's purpose - we're testing the API integration and telemetry, not the specific content of the texts. The spacing doesn't affect the functionality being tested. This seems like an unnecessarily pedantic comment. The double space could theoretically affect the embedding vectors produced by the API. Maybe consistent formatting is important for reproducible tests. The embedding API is designed to handle natural language with various spacing. A single extra space would have negligible impact on embeddings, and the test's assertions don't depend on specific embedding values anyway. Delete this comment as it's overly pedantic about formatting in test data that doesn't impact the test's functionality.
24. tests/test_instrumentations/test_cohere/test_rerank.py:19
- Draft comment:
Typo: Detected a double space in the string literal ('At the 2010 United States Census'). Please check if this extra space is intentional. - 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 very minor formatting issue in test data. The double space doesn't affect the functionality of the tests or the code being tested. It's purely cosmetic and in test data that's meant to represent example text. The extra space could even be intentional to test handling of multiple spaces. Making the author fix this would create unnecessary work for minimal benefit. The double space could be a genuine typo that should be fixed for consistency. Multiple spaces can sometimes cause subtle issues in text processing. While consistency is good, this is test data where the exact spacing doesn't matter functionally. The cost of fixing it across multiple places outweighs the minimal benefit. Delete this comment as it points out a trivial formatting issue in test data that doesn't impact functionality.
Workflow ID: wflow_X2y8azNB0WnwykH4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
_set_span_chat_response(span, final_response) | ||
if final_response.get("error"): | ||
span.set_status(Status(StatusCode.ERROR, final_response.get("error"))) | ||
span.record_exception(final_response.get("error")) |
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.
In the async function aprocess_chat_v2_streaming_response, if final_response.get("error") is true the span is updated with error status and exception recorded but span.end() is not called. Consider ensuring span.end() is invoked on all code paths (e.g. by using a try/finally block) so the span is always ended.
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.
Caution
Changes requested ❌
Reviewed 7870fc4 in 1 minute and 48 seconds. Click for details.
- Reviewed
118
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft 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. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/cohere/__init__.py:233
- Draft comment:
Inconsistent use of use_span parameters: _wrap uses 'end_on_exit=False' while _awrap does not specify it. Verify that both async and sync spans are managed consistently. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_2bhUACDR9G0Ov7SE
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
if span.is_recording(): | ||
span.set_status(Status(StatusCode.ERROR, str(e))) | ||
span.record_exception(e) | ||
span.end() |
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.
Duplicate span.end() call in _wrap: In the exception block you call span.end() and then the finally block calls span.end() again. Consider removing the inner call to avoid redundancy.
span.end() |
if span.is_recording(): | ||
span.set_status(Status(StatusCode.ERROR, str(e))) | ||
span.record_exception(e) | ||
span.end() |
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.
Duplicate span.end() call in _awrap: The exception block calls span.end() before re-raising, yet a finally block also calls span.end(). This may lead to redundant calls.
span.end() |
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.
Important
Looks good to me! 👍
Reviewed 44c847a in 1 minute and 37 seconds. Click for details.
- Reviewed
55
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft 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. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/cohere/streaming.py:132
- Draft comment:
Good use of try…finally to ensure span.end() is always called. Note that wrapping the entire async generator means that if the consumer stops iterating early, the span may end without processing final attributes—verify this behavior is intended. - 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 comment has two parts: 1) Praise for try/finally usage which isn't necessary feedback, and 2) A request to verify behavior which violates our rules about asking for verification. The behavior itself seems correct - if iteration stops early, the span should end immediately without processing final attributes, as that's how generator cleanup works. The comment raises a valid technical point about early termination behavior. Maybe this is actually a bug that needs fixing? Early termination of async generators is a normal pattern, and the span ending immediately in such cases is reasonable behavior. If this was a bug, it would need to be clearly stated as such, not phrased as a verification request. Delete the comment. It combines unnecessary praise with a request for verification, violating our rules about not asking authors to verify behavior.
2. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/cohere/streaming.py:147
- Draft comment:
If an exception occurs during the async iteration, the span status isn’t updated (e.g. to ERROR) before span.end() is called. Consider capturing exceptions at this level to record the error on the span. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_VMzkR3ewwCZyGnAj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Introduces Cohere v2 instrumentor for OpenTelemetry, enabling tracing and event logging for Cohere client library with comprehensive test coverage.
CohereInstrumentor
in__init__.py
for tracing Cohere client library interactions.chat
,completion
,rerank
,embed
.process_chat_v1_streaming_response
andprocess_chat_v2_streaming_response
.emit_input_event
andemit_response_events
.utils.py
for handling exceptions, checking environment variables, and converting objects.span_utils.py
for request and response attributes.test_chat.py
,test_completion.py
,test_embed.py
, andtest_rerank.py
.cassettes/
directory.conftest.py
with fixtures for instrumentors and clients.This description was created by
for 44c847a. You can customize this summary. It will automatically update as commits are pushed.