Skip to content

fix(cohere): avoid double-ending async error spans#4375

Open
euisuh wants to merge 2 commits into
traceloop:mainfrom
euisuh:fix/cohere-async-error-double-end
Open

fix(cohere): avoid double-ending async error spans#4375
euisuh wants to merge 2 commits into
traceloop:mainfrom
euisuh:fix/cohere-async-error-double-end

Conversation

@euisuh

@euisuh euisuh commented Jul 24, 2026

Copy link
Copy Markdown

Summary

Fixes Cohere async instrumentation error handling so spans are not ended twice when an async Cohere call raises.

_awrap() uses tracer.start_as_current_span(...), which already ends the span when the context manager exits. The exception handler also called span.end(), causing OpenTelemetry to emit:

Calling end() on an ended span.

This removes the redundant manual end call while preserving ERROR status, exception recording, and re-raising behavior.

Related

Related to #2338, which tracks Cohere async instrumentation behavior.

Test Plan

  • uv run pytest tests/test_async_error_handling.py tests/test_rerank.py::test_cohere_v2_rerank_legacy_async tests/test_chat.py::test_cohere_chat_legacy_async -q
  • uv run pytest tests/ -q
  • uv run ruff check .

Summary by CodeRabbit

  • Bug Fixes
    • Improved OpenTelemetry error handling for asynchronous Cohere chat requests.
    • Exceptions are still recorded with ERROR status, and spans now finish cleanly without duplicate-end warnings.
  • Tests
    • Added an async test to verify the exception is raised once, the span is reported once with the correct error details, and includes an exception event.

@CLAassistant

CLAassistant commented Jul 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: eff0b00b-ec28-47d0-b034-85c3288a13d8

📥 Commits

Reviewing files that changed from the base of the PR and between e27f636 and 9c303fd.

📒 Files selected for processing (1)
  • packages/opentelemetry-instrumentation-cohere/tests/test_async_error_handling.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/opentelemetry-instrumentation-cohere/tests/test_async_error_handling.py

📝 Walkthrough

Walkthrough

The async Cohere wrapper no longer explicitly ends spans in its exception path. A new test verifies exception recording, error status, single span completion, and absence of duplicate-end warnings during cleanup.

Changes

Cohere async error handling

Layer / File(s) Summary
Async span lifecycle and regression coverage
packages/opentelemetry-instrumentation-cohere/opentelemetry/instrumentation/cohere/__init__.py, packages/opentelemetry-instrumentation-cohere/tests/test_async_error_handling.py
The wrapper records exceptions and re-raises without explicitly ending the span; async coverage verifies one finished cohere.chat error span and no duplicate-end warning.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main fix: preventing async Cohere error spans from being ended twice.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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-cohere/tests/test_async_error_handling.py`:
- Around line 29-35: Extend the async error-handling test assertions for the
finished span returned by span_exporter.get_finished_spans() to verify it
contains an exception event with the recorded failure message "cohere async
failure". Keep the existing status, description, and caplog assertions
unchanged.
🪄 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 Plus

Run ID: 5ef2b7b8-d193-4da8-944e-75389fbad6b5

📥 Commits

Reviewing files that changed from the base of the PR and between 93429cf and e27f636.

📒 Files selected for processing (2)
  • packages/opentelemetry-instrumentation-cohere/opentelemetry/instrumentation/cohere/__init__.py
  • packages/opentelemetry-instrumentation-cohere/tests/test_async_error_handling.py
💤 Files with no reviewable changes (1)
  • packages/opentelemetry-instrumentation-cohere/opentelemetry/instrumentation/cohere/init.py

Comment thread packages/opentelemetry-instrumentation-cohere/tests/test_async_error_handling.py Outdated
Extend the async error-path regression test to assert the recorded exception event message, matching CodeRabbit review feedback.

@gnanirahulnutakki gnanirahulnutakki left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reproduced one telemetry-correctness issue on the submitted head. The inline comment includes the failing exact-cardinality assertion and a locally verified minimal fix; otherwise the change is focused and the stated async tests pass.

@@ -287,7 +287,6 @@ async def _awrap(
if span.is_recording():
span.set_status(Status(StatusCode.ERROR, str(e)))
span.record_exception(e)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The outer start_as_current_span(...) context manager already records any exception that escapes this block (its default is record_exception=True), so this explicit call still creates duplicate exception telemetry even though the duplicate end() is gone. I reproduced this on 9c303fd: changing the new test to assert len(exception_events) == 1 fails with 2 == 1.

Please give one layer sole ownership of exception recording—e.g. remove this manual try/except and let the context manager set ERROR/record the exception, or disable its automatic exception handling—and make the regression assert exact cardinality. With the manual handler removed, the strengthened test plus test_cohere_v2_rerank_legacy_async and test_cohere_chat_legacy_async all pass locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants