-
Notifications
You must be signed in to change notification settings - Fork 868
fix(langchain): populate metadata as span attributes in batch operations #3218
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
Fixed issue where metadata from Langchain batch() and abatch() method calls were not being populated as span attributes. The metadata was previously only stored in the OpenTelemetry context for propagation but never set as actual span attributes, making it difficult to correlate errors with specific data points. Changes: - Modified _create_span method to set sanitized metadata as span attributes - Added tests for both sync batch and async abatch operations - Metadata is now accessible as traceloop.association.properties.* attributes Fixes: #3213 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThe changes introduce logic to extract metadata from Langchain Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Langchain Instrumentation
participant OpenTelemetry Span
User->>Langchain Instrumentation: Call batch/abatch with metadata
Langchain Instrumentation->>Langchain Instrumentation: Extract and sanitize metadata
Langchain Instrumentation->>OpenTelemetry Span: Set span attributes from metadata
OpenTelemetry Span-->>Langchain Instrumentation: Span with metadata attributes
Langchain Instrumentation-->>User: Return batch/abatch results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
✨ 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 (
|
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 everything up to f0475f2 in 1 minute and 23 seconds. Click for details.
- Reviewed
572lines of code in4files - 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:237
- Draft comment:
Good enhancement: metadata is now correctly set as span attributes using sanitized values. Consider adding a short note that the attributes are prefixed with '{SpanAttributes.TRACELOOP_ASSOCIATION_PROPERTIES}' to clarify the naming convention for future maintainers. - 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 is suggesting a minor documentation improvement to clarify the prefix used. While technically correct, the prefix usage is already clear from the code itself. The comment is not pointing out a bug or suggesting a meaningful code improvement. It's just asking for slightly more verbose comments about something that's already evident in the code. The prefix pattern could be important for maintainers to understand. Having it documented explicitly could save time for future developers. While documentation can be helpful, in this case the code is self-documenting - the prefix usage is clear and explicit in the code. The suggested comment would just be repeating what's already obvious from reading the code. The comment should be deleted as it suggests adding redundant documentation for something that is already clear from the code itself.
2. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:29
- Draft comment:
The tests effectively check that batch and abatch calls propagate metadata as span attributes. Note that assertions allow for either a direct key or a dotted key (with the prefix) – consider standardizing the attribute naming format to simplify these checks. - 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 is making a suggestion about code quality and test structure. However, the dual format checks may be intentionally designed to handle both formats, rather than being an oversight. Without understanding the requirements of the telemetry system and whether both formats need to be supported, suggesting standardization could be premature or incorrect. I might be wrong about the dual format being intentional - perhaps it really is unnecessary complexity that should be standardized. The suggestion could lead to simpler, more maintainable tests. Even if standardization might be beneficial, we don't have enough context about the telemetry requirements to know if both formats need to be supported. Making assumptions about simplifying the format could break important functionality. While the comment raises an interesting point about test structure, we lack the context to know if the suggestion is valid. The dual format may be a requirement rather than an opportunity for improvement.
Workflow ID: wflow_DtDoxrRMEDJRbre1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py (2)
6-44: Comprehensive test coverage with minor cleanup needed.The test logic effectively validates sync batch metadata propagation with proper span filtering and attribute validation. The fallback checking for both direct and namespaced attributes is well-designed.
Apply this diff to remove the unused variable flagged by static analysis:
- # Call batch with metadata - responses = llm.batch(messages_list, config={"metadata": test_metadata}) + # Call batch with metadata + llm.batch(messages_list, config={"metadata": test_metadata})
46-84: Well-structured async test with minor cleanup needed.The async test properly validates abatch metadata propagation with appropriate async patterns and comprehensive attribute checking. Good coverage of the async scenario.
Apply this diff to remove the unused variable flagged by static analysis:
- # Call abatch with metadata - responses = await llm.abatch(messages_list, config={"metadata": test_metadata}) + # Call abatch with metadata + await llm.abatch(messages_list, config={"metadata": test_metadata})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py(1 hunks)packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_batch_metadata/test_async_batch_metadata_in_span_attributes.yaml(1 hunks)packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_batch_metadata/test_batch_metadata_in_span_attributes.yaml(1 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py (3)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-257)packages/opentelemetry-instrumentation-langchain/tests/metrics/test_langchain_metrics.py (1)
llm(18-19)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
get_finished_spans(40-43)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py
19-19: Local variable responses is assigned to but never used
Remove assignment to unused variable responses
(F841)
60-60: Local variable responses is assigned to but never used
Remove assignment to unused variable responses
(F841)
🪛 Flake8 (7.2.0)
packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py
[error] 19-19: local variable 'responses' is assigned to but never used
(F841)
[error] 60-60: local variable 'responses' is assigned to but never used
(F841)
🔇 Additional comments (3)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (1)
237-241: LGTM! Clean implementation of metadata as span attributes.This change effectively addresses the PR objective by setting sanitized metadata as span attributes using the proper namespace prefix. The implementation correctly leverages the existing sanitized metadata and follows established patterns in the codebase.
packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_batch_metadata/test_async_batch_metadata_in_span_attributes.yaml (1)
1-233: LGTM! Well-structured VCR cassette for async batch testing.This cassette properly records the HTTP interactions for the async batch metadata test, enabling deterministic test replay. The recorded requests match the expected test messages and include proper tracing headers.
packages/opentelemetry-instrumentation-langchain/tests/cassettes/test_batch_metadata/test_batch_metadata_in_span_attributes.yaml (1)
1-222: LGTM! Appropriate VCR cassette for synchronous batch testing.This cassette correctly records the sync batch interactions with proper headers indicating synchronous operation (note the
x-stainless-async: falseheader vsasync:asyncioin the async version). Enables deterministic testing for the sync batch metadata scenario.
- Fixed line length violations by breaking long lines - Removed unused variables - Fixed whitespace issues - Added proper newline at end of file
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 b5f242a in 38 seconds. Click for details.
- Reviewed
130lines of code in1files - 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/tests/test_batch_metadata.py:23
- Draft comment:
Good cleanup: Removed the assignment to 'responses' from the batch() call since the return value wasn’t used. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:72
- Draft comment:
Good cleanup: Removed the unused assignment from the await abatch() call, which simplifies the async test. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/opentelemetry-instrumentation-langchain/tests/test_batch_metadata.py:101
- Draft comment:
Minor style note: Add a newline at the end of the file to comply with style guidelines. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_pwYfnK3lPzhaulXr
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Please note that this now duplicates some of the properties such as:
Not an issue with most OTEL providers though. |
…ons (#3218) Co-authored-by: Claude <noreply@anthropic.com>
Fixed issue where metadata from Langchain batch() and abatch() method calls were not being populated as span attributes. The metadata was previously only stored in the OpenTelemetry context for propagation but never set as actual span attributes, making it difficult to correlate errors with specific data points.
Changes:
Fixes: #3213
🤖 Generated with Claude Code
feat(instrumentation): ...orfix(instrumentation): ....Important
Fixes metadata not being set as span attributes in Langchain batch operations by modifying
_create_spanand adding tests._create_spanincallback_handler.pyto set sanitized metadata as span attributes for batch operations.traceloop.association.properties.*attributes.test_batch_metadata.pyto verify metadata is recorded as span attributes in sync and async batch calls.test_async_batch_metadata_in_span_attributes.yamlandtest_batch_metadata_in_span_attributes.yamlfor reliable testing of metadata propagation.This description was created by
for b5f242a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit