fix(langchain): spans dictionary memory leak#3216
Merged
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant LangChain
participant CallbackHandler
participant OpenTelemetry
LangChain->>CallbackHandler: on_llm_end()
CallbackHandler->>CallbackHandler: Record duration metric
CallbackHandler->>CallbackHandler: _end_span(run_id)
CallbackHandler->>OpenTelemetry: End all child spans (if exist)
CallbackHandler->>OpenTelemetry: End parent span
CallbackHandler->>CallbackHandler: Delete span entry for run_id
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
4 tasks
![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 8528eb0 in 38 seconds. Click for details.
- Reviewed
44lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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:
Good cleanup: Deleting the span from self.spans here prevents the memory leak. Additionally, checking 'if child_id in self.spans' before ending child spans avoids potential KeyErrors if they've already been removed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py:543
- Draft comment:
Reordering in on_llm_end is correct: recording the duration before calling _end_span ensures that self.spans[run_id] is still available for reading, preventing a KeyError after deletion. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_YsoyESAqboNuigIF
You can customize 
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed dba192a in 1 minute and 40 seconds. Click for details.
- Reviewed
31lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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:180
- Draft comment:
Minor formatting cleanup: removed trailing whitespace in the child span check. Since this method is responsible for ending spans, please verify that child spans are also cleaned up (i.e. removed from thespansdict) to avoid lingering references and potential memory leaks. - Reason this comment was not posted:
Comment was on unchanged code.
2. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py:550
- Draft comment:
Removed an unnecessary blank line before calling _end_span in the LLM end handler. This is a pure formatting change intended to improve readability. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, describing a formatting change that doesn't affect functionality. It doesn't provide a suggestion or raise a concern about the code.
Workflow ID: wflow_BxWPst7SVZjVZK4Y
You can customize
doronkopit5
approved these changes
Aug 3, 2025
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5c1ce20 in 39 seconds. Click for details.
- Reviewed
397lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft 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/tests/cassettes/test_async_batch_metadata/test_async_batch_metadata_in_span_attributes.yaml:19
- Draft comment:
The updated traceparent (and associated header values, e.g. timestamps and CF-RAY) reflect the changes in span handling. Ensure that any dynamically generated fields are normalized or handled in tests to avoid brittleness in future runs. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_batch_metadata/test_batch_metadata_in_span_attributes.yaml:3
- Draft comment:
In this cassette, the request and response bodies (including binary encoded span attributes) and several header timings have been updated. These changes are consistent with improved error handling and recording duration metrics before ending spans. Confirm that these cassette updates fully capture the resolved memory leak behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_GE4XfyAiVX86ZFoo
You can customize
nirga
force-pushed
the
langchain-memory-leak
branch
from
5c1ce20 to
9dc249c
Compare
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9dc249c in 2 minutes and 56 seconds. Click for details.
- Reviewed
8163lines of code in18files - Skipped
0files when reviewing. - Skipped posting
8draft 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/tests/test_lcel.py:1005
- Draft comment:
In the helper function assert_message_in_logs, you convert log.log_record.body to a dict without checking its type. Consider adding a type check or comment so that future maintainers know what type of object to expect. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py:24
- Draft comment:
The tests make good use of pipelines (using the | operator) to compose the chain. Consider adding a brief inline comment explaining that instrument_legacy, instrument_with_content, and instrument_with_no_content are fixtures that toggle the telemetry content to avoid confusion for future readers. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py:44
- Draft comment:
When asserting the set of span names, consider using a more explicit approach (e.g., sorted lists) for better diagnostic output in case of mismatch. Consistency between tests that use set equality and those that rely on list equality (e.g. test_invoke) might be beneficial. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py:813
- Draft comment:
In test_lcel_with_datetime, the test compares a datetime converted to an ISO string. This is clear, but be aware that any change in the serialization format may break the test. Consider adding a comment that this test validates the expected ISO 8601 format. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py:192
- Draft comment:
The expected AI choice event in test_simple_lcel_with_events_with_content is hardcoded with tool call arguments. Ensure that these expected values remain in sync with the actual function conversion behavior in convert_pydantic_to_openai_function. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py:140
- Draft comment:
For asynchronous tests using @pytest.mark.asyncio and await invocations, the structure and parent-child span assertions are clear. Consider adding brief comments to note the difference between synchronous and asynchronous pipelines to aid future maintainers. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_batch_metadata/test_batch_metadata_in_span_attributes.yaml:182
- Draft comment:
Typo: The 'Transfer-Encoding:' header appears to be missing its value or list item. Please add the appropriate value or remove the header if unintended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_lcel/test_stream_with_events_with_no_content.yaml:155
- Draft comment:
There appears to be an odd concatenation in the streamed text around the tokens 'flaming', 'or', 'os', 'tacos'. When merged, it reads as "flaming or os tacos," which seems unintended. Could you verify if the intended word is "flamingos" (or another phrase)? - 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 suggests there might be an issue with token concatenation, but looking at the context, this appears to be intentional. The text is describing different types of sock designs, and "flamingos or tacos" makes perfect sense as quirky sock pattern options. The tokens are being streamed correctly as part of the API response. Could there be an actual technical issue with how the tokens are being streamed that I'm missing? The comment author may have noticed something in the implementation that isn't visible in just this test file. No - this is a test cassette file that records actual API responses. The token streaming pattern shown here matches OpenAI's normal behavior, and the text content itself is coherent and intentional. The comment should be deleted. The apparent "odd concatenation" is actually the intended text content being streamed correctly, describing sock designs including "flamingos or tacos".
Workflow ID: wflow_IXbt41kovuf3lGSU
You can customize
nirga
force-pushed
the
langchain-memory-leak
branch
from
9dc249c to
e21167b
Compare
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed e21167b in 1 minute and 0 seconds. Click for details.
- Reviewed
481lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft 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/tests/test_batch_metadata.py:7
- Draft comment:
Skip marker: The test is deliberately skipped due to VCR issues in CI. Ensure that this is re-enabled once VCR is fixed to maintain proper test coverage. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:57
- Draft comment:
Skip marker for async test: The async test is also skipped due to VCR issues in CI; make sure to re-enable it once the CI environment is fixed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:41
- Draft comment:
Consider asserting for a consistent key format for metadata attributes instead of using an OR condition. This dual check may mask naming issues in the span attributes. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:59
- Draft comment:
Minor: The docstring in the async test refers to 'abatch calls'. Consider renaming it to 'async batch' for clarity. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
5. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:59
- Draft comment:
Typo found: 'abatch' in the docstring might be a typo. Consider replacing 'abatch' with 'batch'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_UvzbLX0IEsD74pRR
You can customize
nina-kollman
pushed a commit
that referenced
this pull request
Aug 11, 2025
Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Aug 12, 2025
4 tasks
1 task
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.
Fixes #2790
feat(instrumentation): ...orfix(instrumentation): ....Important
Fixes memory leak in
TraceloopCallbackHandlerby deleting spans after use and updates tests for API changes.TraceloopCallbackHandlerby deleting spans after use in_end_span().on_llm_end().test_batch_metadata_in_span_attributesandtest_async_batch_metadata_in_span_attributesas skipped due to VCR issues in CI.This description was created by
for e21167b. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests