Merged
Conversation
WalkthroughReplaces context_api.detach with _RUNTIME_CONTEXT.detach and broadens exception handling in callback_handler._end_span. Adds a new test validating context detachment error handling under concurrent async LangGraph execution; the test appears duplicated identically within the file. Changes
Sequence Diagram(s)sequenceDiagram
participant App as LangGraph workflow
participant Handler as CallbackHandler._end_span
participant RT as _RUNTIME_CONTEXT
participant OTel as Tracer/Exporter
App->>Handler: end_span(span, token)
Handler->>RT: detach(token)
alt detach succeeds
RT-->>Handler: ok
Handler->>OTel: span.end()
else detach raises
RT-->>Handler: error
note over Handler: Swallow exception
Handler->>OTel: span.end()
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to b8b6bef in 2 minutes and 15 seconds. Click for details.
- Reviewed
168lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft 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. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py:190
- Draft comment:
Using the private _RUNTIME_CONTEXT to detach the token avoids unwanted error logging, but it relies on an internal API. If no public alternative exists, that's acceptable; however, please add a note about the potential fragility if the internal API changes. - 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 comment makes a valid point about private API usage, but the code already has a clear comment explaining why this approach was chosen. The suggested change is to add a warning comment, but this doesn't materially improve the code. The existing comment already documents the intent and tradeoff. Adding another comment about API fragility would be redundant since using a private API inherently implies potential fragility. The comment raises a legitimate concern about private API usage that could cause future maintenance issues. Documentation about potential breakage could help future maintainers. While the concern is valid, the existing comment already explains why this approach was chosen. The private API nature is obvious from the underscore prefix, and adding another warning comment would be redundant without providing actionable guidance. Delete the comment. While it raises a valid concern, the code already has sufficient documentation explaining the tradeoff, and adding another warning comment would be redundant without providing actionable value.
2. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py:194
- Draft comment:
Catching Exception (in addition to ValueError and RuntimeError) may hide unexpected issues. If this broad catch is intended to ensure robustness in async contexts, consider documenting why all exceptions are silently ignored. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/opentelemetry-instrumentation-langchain/tests/test_langgraph.py:343
- Draft comment:
The new test for context detachment error handling is comprehensive and simulates concurrent asynchronous operations well. It verifies that no 'Failed to detach context' errors are logged. Ensure that future maintainers understand the intent by keeping the inline comments clear. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It praises the test and suggests keeping inline comments clear, which is not specific or actionable enough to be useful.
Workflow ID: wflow_8DJHiTBMzu84OWxy
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_langgraph.py (2)
458-470: Make log assertion resilient to upstream message wording changesMatching the exact message string is brittle. Filter by logger name and semantic keyword to future-proof the assertion while still guarding against detach errors.
Apply this diff:
- error_logs = [ - record.message - for record in caplog.records - if record.levelno >= logging.ERROR - ] - context_errors = [ - msg for msg in error_logs if "Failed to detach context" in msg - ] - - assert len(context_errors) == 0, ( - f"Found {len(context_errors)} context detachment errors in logs. " - f"This indicates the fix is not working properly. Errors: {context_errors}" - ) + context_error_records = [ + r + for r in caplog.records + if r.levelno >= logging.ERROR + and r.name == "opentelemetry.context" + and "detach" in r.getMessage() + ] + assert not context_error_records, ( + f"Found {len(context_error_records)} context detachment errors in logs. " + f"This indicates the fix is not working properly. Errors: " + f"{[r.getMessage() for r in context_error_records]}" + )
472-481: Reduce O(n^2) parent lookup to O(n) for readability and efficiencyCreate an index of spans by span_id to simplify the parent assertions and avoid repeated scans.
Apply this diff:
- for nested_span in nested_spans: - assert nested_span.parent is not None, "Nested spans should have parents" - parent_span = next( - (s for s in spans if s.context.span_id == nested_span.parent.span_id), - None, - ) - assert parent_span is not None, "Parent span should exist" - assert ( - parent_span.name == "concurrent_async_span" - ), "Nested span should be child of concurrent_async_span" + spans_by_id = {s.context.span_id: s for s in spans} + for nested_span in nested_spans: + assert nested_span.parent is not None, "Nested spans should have parents" + parent_span = spans_by_id.get(nested_span.parent.span_id) + assert parent_span is not None, "Parent span should exist" + assert parent_span.name == "concurrent_async_span", ( + "Nested span should be child of concurrent_async_span" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py(1 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_langgraph.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/opentelemetry-instrumentation-langchain/tests/test_langgraph.py (2)
packages/opentelemetry-instrumentation-langchain/tests/conftest.py (1)
instrument_legacy(77-95)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
get_finished_spans(40-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_langgraph.py (2)
343-356: Good, targeted async-concurrency test to exercise context detachment pathsThe test sets up realistic async/concurrent spans and validates no error logs. This directly covers the regressions observed around detach. Looks solid.
343-471: Confirmed: Unique Test and Exporter Reset per Test
- No duplicate definitions of
test_context_detachment_error_handlingwere found intest_langgraph.py.- Although
span_exporteris session-scoped, theclear_exporterautouse fixture clears it before each test, ensuring a fresh exporter for span-count assertions.No changes needed here.
Comment on lines
+191
to
197
| # Use the runtime context directly to avoid logging from context_api.detach() | ||
| from opentelemetry.context import _RUNTIME_CONTEXT | ||
| _RUNTIME_CONTEXT.detach(token) | ||
| except (ValueError, RuntimeError, Exception): | ||
| # Context detach can fail in async scenarios when tokens are created in different contexts | ||
| # This includes ValueError, RuntimeError, and other context-related exceptions | ||
| # This is expected behavior and doesn't affect the correct span hierarchy |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid importing and using private _RUNTIME_CONTEXT inline; add a safe, module-level fallback and narrow exception scope
Importing a private symbol inside the method on every call is brittle and adds overhead. Also, catching broad Exception can hide unrelated bugs. Prefer a module-level optional import (with fallback to context_api.detach) and only catch the expected detach errors.
Apply this diff within the selected range:
- try:
- # Use the runtime context directly to avoid logging from context_api.detach()
- from opentelemetry.context import _RUNTIME_CONTEXT
- _RUNTIME_CONTEXT.detach(token)
- except (ValueError, RuntimeError, Exception):
- # Context detach can fail in async scenarios when tokens are created in different contexts
- # This includes ValueError, RuntimeError, and other context-related exceptions
- # This is expected behavior and doesn't affect the correct span hierarchy
- pass
+ try:
+ # Prefer the runtime context to avoid error-level logging in context_api.detach()
+ if _OTEL_RUNTIME_CONTEXT is not None:
+ _OTEL_RUNTIME_CONTEXT.detach(token)
+ else:
+ context_api.detach(token)
+ except (ValueError, RuntimeError):
+ # Detach can fail when token belongs to a different async context; ignore.
+ passAnd at the top of the file (module scope), add a safe optional import:
# Prefer runtime context when available to avoid logging in context_api.detach()
try:
from opentelemetry.context import _RUNTIME_CONTEXT as _OTEL_RUNTIME_CONTEXT # type: ignore[attr-defined]
except Exception:
_OTEL_RUNTIME_CONTEXT = None # type: ignore[assignment]Rationale:
- _RUNTIME_CONTEXT is a private API; guarding with a fallback limits blast radius if the upstream package changes.
- Narrowing to (ValueError, RuntimeError) prevents masking unexpected failures while still addressing the async token-mismatch cases.
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
around lines 191-197, replace the inline import and broad Exception catch: add a
module-level optional import of _RUNTIME_CONTEXT (named _OTEL_RUNTIME_CONTEXT)
at the top of the file with a safe fallback to None, then in this block use
_OTEL_RUNTIME_CONTEXT.detach(token) when available and otherwise call
context_api.detach(token); also narrow the except clause to only catch
ValueError and RuntimeError so unrelated exceptions are not swallowed.
nirga
approved these changes
Aug 13, 2025
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.
Important
Fix context detachment issue in async scenarios by modifying
_end_span()incallback_handler.pyand adding a test intest_langgraph.py._end_span()incallback_handler.pyto use_RUNTIME_CONTEXT.detach()instead ofcontext_api.detach()to avoid logging errors._end_span()to includeRuntimeErrorandException.test_context_detachment_error_handling()intest_langgraph.pyto verify context detachment errors are handled without logging.This description was created by
for b8b6bef. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests