fix(langchain): allow configuration of metadata key prefix #3367
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a configurable metadata key prefix (Config.metadata_key_prefix) with a new LangchainInstrumentor constructor parameter; callback handler now uses that config when creating metadata span attributes and includes the error string in span status; one span attribute constant was switched to a different semantic attribute; tests updated to expect additional log events and re-enabled AI choice assertions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Instrumentor as LangchainInstrumentor
participant Config as Config
participant Handler as CallbackHandler
participant Tracer as OpenTelemetry Tracer
rect rgb(235,245,255)
User->>Instrumentor: instantiate(..., metadata_key_prefix?)
alt provided
Instrumentor->>Config: set metadata_key_prefix = value
else default
Instrumentor->>Config: keep default (SpanAttributes.TRACELOOP_ASSOCIATION_PROPERTIES)
end
end
rect rgb(245,255,235)
Handler->>Config: read metadata_key_prefix
Handler->>Tracer: start span(...)
note right of Tracer: attach attributes using\nConfig.metadata_key_prefix + metadata_key
end
rect rgb(255,240,240)
Handler->>Tracer: on error -> span.set_status(Status(ERROR), str(error))
note right of Tracer: status now includes error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py (6)
🔇 Additional comments (6)
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. Comment |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5bcaf72 in 2 minutes and 36 seconds. Click for details.
- Reviewed
58lines of code in3files - 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/__init__.py:40
- Draft comment:
Missing import for 'Optional' from the typing module. Since you're using 'Optional[str]' in the function signature, addfrom typing import Optionalat the top. - 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/opentelemetry/instrumentation/langchain/__init__.py:56
- Draft comment:
Consider avoiding mutation of the global Config for 'metadata_key_prefix'. Storing this configuration as an instance attribute could improve isolation when multiple instrumentors are used concurrently. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_J6RF7kgIJ4ANhUUO
You can customize 
...emetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
Show resolved
Hide resolved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (1)
289-295: Fix undefined name: Config.Ruff/Flake8 report F821 at Line 293. Add the missing import.
Apply this import near the other langchain instrumentation imports:
+from opentelemetry.instrumentation.langchain.config import Config
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (1)
289-295: Normalize custom prefixes to avoid double dots.If a caller supplies a trailing dot (e.g., "langfuse.metadata."), resulting keys become "..key". Strip trailing dots once and reuse.
Apply:
- if metadata is not None: - for key, value in sanitized_metadata.items(): - _set_span_attribute( - span, - f"{Config.metadata_key_prefix}.{key}", - value, - ) + if metadata is not None: + prefix = Config.metadata_key_prefix.rstrip(".") + for key, value in sanitized_metadata.items(): + _set_span_attribute( + span, + f"{prefix}.{key}", + value, + )packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py (1)
56-57: Use explicit None check and normalize the prefix.Truthiness skips valid empty strings; prefer
is not None. Also strip trailing dots once.Apply:
- if metadata_key_prefix: - Config.metadata_key_prefix = metadata_key_prefix + if metadata_key_prefix is not None: + Config.metadata_key_prefix = metadata_key_prefix.rstrip(".")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py(1 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py(1 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.pypackages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.pypackages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
🧬 Code graph analysis (3)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py (1)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-261)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py (1)
Config(6-10)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py (1)
Config(6-10)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
293-293: Undefined name Config
(F821)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
40-40: Undefined name Optional
(F821)
🪛 Flake8 (7.2.0)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
[error] 293-293: undefined name 'Config'
(F821)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
[error] 40-40: undefined name 'Optional'
(F821)
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py (1)
4-4: Configurable prefix default looks good.Importing SpanAttributes and defaulting metadata_key_prefix to TRACELOOP_ASSOCIATION_PROPERTIES matches current behavior while enabling overrides.
Also applies to: 10-10
.../opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
Outdated
Show resolved
Hide resolved
.../opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
Outdated
Show resolved
Hide resolved
|
Hi @nirga, could you please review the updated PR. |
.../opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
Outdated
Show resolved
Hide resolved
.../opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
Outdated
Show resolved
Hide resolved
|
Hi @nirga, updated |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py (1)
39-51: Consider adding validation for metadata_key_prefix parameter.The parameter implementation looks good, but there's no validation to ensure the provided prefix is valid. An empty string, whitespace-only, or string with invalid characters could cause issues when constructing attribute keys like
f"{Config.metadata_key_prefix}.{key}".Consider adding validation in the constructor:
""" super().__init__() Config.exception_logger = exception_logger Config.use_legacy_attributes = use_legacy_attributes + if not metadata_key_prefix or not metadata_key_prefix.strip(): + raise ValueError("metadata_key_prefix cannot be empty or whitespace") Config.metadata_key_prefix = metadata_key_prefix self.disable_trace_context_propagation = disable_trace_context_propagation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py(2 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py(3 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.pypackages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
🧬 Code graph analysis (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py (1)
Config(6-10)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py (2)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-245)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py (1)
Config(6-10)
🔇 Additional comments (4)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (3)
30-30: LGTM! Config import enables dynamic metadata prefix.The import is necessary for accessing
Config.metadata_key_prefixat line 294, enabling the configurable metadata key prefix feature.
290-296: LGTM! Dynamic metadata prefix implementation.The implementation correctly uses
Config.metadata_key_prefixfor constructing metadata attribute keys, enabling support for different backends as intended by this PR.
756-756: Verify: Unrelated change adding error message to span status.This change adds the error description to the span status, which improves observability. However, it appears unrelated to the PR's stated objective of adding a configurable metadata key prefix.
Consider:
- Was this change intentional or incidental?
- Should it be in a separate PR for clearer change tracking?
- Could error messages contain sensitive information that shouldn't be in span status?
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py (1)
18-18: LGTM! Import needed for default parameter value.The
SpanAttributesimport is required for the default value of themetadata_key_prefixparameter.
| super().__init__() | ||
| Config.exception_logger = exception_logger | ||
| Config.use_legacy_attributes = use_legacy_attributes | ||
| Config.metadata_key_prefix = metadata_key_prefix |
There was a problem hiding this comment.
🧩 Analysis chain
Potential issue: Class variable causes instance configuration conflict.
Config.metadata_key_prefix is a class variable shared across all instances. If multiple LangchainInstrumentor instances are created with different metadata_key_prefix values, they will overwrite each other, causing unexpected behavior where one instance's configuration affects another's.
Example scenario:
# Instance A with custom prefix
instrumentor_a = LangchainInstrumentor(metadata_key_prefix="langfuse.metadata")
instrumentor_a.instrument()
# Instance B with default prefix - overwrites A's configuration
instrumentor_b = LangchainInstrumentor()
instrumentor_b.instrument()
# Now A's spans use the default prefix instead of "langfuse.metadata"Consider whether:
- Multiple instrumentor instances are a supported use case
- If yes, store
metadata_key_prefixas an instance variable and pass it to the callback handler - If no, document that only one instrumentor instance should exist per process
Run this script to check if the codebase or documentation discusses multiple instrumentor instances:
🏁 Script executed:
#!/bin/bash
# Search for patterns indicating multiple instrumentor instances or singleton usage
echo "=== Searching for multiple instrumentor instance patterns ==="
rg -n -C3 "LangchainInstrumentor\(\)" --type=py
echo -e "\n=== Checking for singleton or instance management patterns ==="
rg -n -C3 "instrumentor.*=.*LangchainInstrumentor" --type=py
echo -e "\n=== Looking for documentation about instrumentation usage ==="
fd -e md -e rst | xargs rg -l "LangchainInstrumentor"Length of output: 2500
I need to examine the Config class and understand how metadata_key_prefix is actually used to verify if the class variable issue poses a real problem.
Now I need to examine the actual code to understand how Config.metadata_key_prefix is used and whether the class variable design actually causes problems.
🏁 Script executed:
#!/bin/bash
# Examine Config class and metadata_key_prefix usage
echo "=== Finding Config class definition ==="
fd -e py -type f | xargs grep -l "class Config" | grep -E "(config|instrumentation/langchain)" | head -5
echo -e "\n=== Looking at Config class ==="
find . -path "*instrumentation/langchain*" -name "*.py" -type f | xargs grep -l "class Config" | head -1 | xargs cat
echo -e "\n=== Searching for metadata_key_prefix usage ==="
rg -n "metadata_key_prefix" --type=py | head -20Length of output: 1512
I need to examine how the callback handler is instantiated and whether it captures metadata_key_prefix at creation time or reads it dynamically.
🏁 Script executed:
#!/bin/bash
# Examine callback handler instantiation and metadata_key_prefix usage
echo "=== Looking for callback handler instantiation in __init__.py ==="
rg -n -A10 -B5 "callback_handler" packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py | head -40
echo -e "\n=== Checking callback_handler.py around line 294 ==="
sed -n '280,310p' packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
echo -e "\n=== Looking at full callback handler initialization ==="
rg -n "class.*CallbackHandler" packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
echo -e "\n=== Checking if metadata_key_prefix is passed to callback handler ==="
rg -n -B5 -A5 "CallbackHandler\(" packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.pyLength of output: 3660
Now let me check the README to understand the intended usage pattern and whether multiple instances are documented as supported or unsupported.
🏁 Script executed:
#!/bin/bash
# Check README for usage documentation
echo "=== Reading LangchainInstrumentor README ==="
cat packages/opentelemetry-instrumentation-langchain/README.mdLength of output: 1180
Store metadata_key_prefix as an instance variable to prevent configuration conflicts.
The issue is confirmed: Config.metadata_key_prefix is a class variable read dynamically by the callback handler at span creation time (callback_handler.py line 294). Multiple LangchainInstrumentor instances with different metadata_key_prefix values will overwrite each other's configuration via line 55 in __init__.py.
Currently, the codebase and README show only single-instance usage (LangchainInstrumentor().instrument()), and no concurrent multi-instance scenarios exist. However, this design is a latent footgun: if future code creates multiple instances with different prefixes, spans will silently use the wrong prefix.
Recommended fix: Pass metadata_key_prefix to TraceloopCallbackHandler at instantiation (line 89) and have it store/use the instance-specific value instead of reading from the shared Config class variable. Alternatively, document explicitly that only one LangchainInstrumentor instance should be active per process.
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py
around line 55 (and referencing the TraceloopCallbackHandler instantiation at
~line 89 and its use at callback_handler.py line 294), the code currently
assigns metadata_key_prefix to the shared Config.class variable which causes
cross-instance config overwrite; change the implementation to store
metadata_key_prefix as an instance attribute on LangchainInstrumentor (not
Config), modify the TraceloopCallbackHandler constructor call at line 89 to
accept this metadata_key_prefix and update the TraceloopCallbackHandler to store
and use the passed instance-specific prefix instead of reading
Config.metadata_key_prefix at runtime (callback creation/usage around line 294).
ianchi
force-pushed
the
langchain
branch
2 times, most recently
from
df8082f to
1024fee
Compare
|
Hi @nirga fixed linting (tested locally) and rebased to fix merge conflicts |
|
Hi @nirga, I added a fix on an invalid semconv attribute. This one does not exist upstream It errors and completely breaks logging of output |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/opentelemetry-instrumentation-langchain/tests/test_chains.py (2)
760-776: Validate or document the second log entry.The test expects 2 logs but only validates
logs[0]. The AI choice event validation is commented out (lines 770-775), suggesting uncertainty about whether the second log is consistently emitted. If the second log is expected, uncomment and validate it. If it's optional or inconsistent, add a comment explaining why.
805-813: Validate or document the second log entry.The test expects 2 logs but only validates
logs[0]. The AI choice event validation is commented out (lines 811-812), suggesting uncertainty about whether the second log is consistently emitted. If the second log is expected, uncomment and validate it. If it's optional or inconsistent, add a comment explaining why.packages/opentelemetry-instrumentation-langchain/tests/test_llms.py (1)
341-357: Remove misleading comments about non-existent logs indices.Line 341 asserts
len(logs) == 3, which guaranteeslogs[2]exists. The comment "# logs[2] does not exist" on line 357 is contradictory and appears to be a leftover from before this PR changed the expected log count from 2 to 3.The git diff shows the PR uncommented these assertions while changing
len(logs) == 2→len(logs) == 3, but the old comments were left behind. Similar misleading comments appear on lines 209, 253, 406, 567, 627, 766, 818, and many others throughout the file.Remove the comments on these assertion lines—they contradicted the code logic and should have been deleted during the refactoring.
♻️ Duplicate comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_llms.py (2)
392-406: Same critical issue repeated: logs[2] assertion contradicts comments.Multiple tests follow the same problematic pattern:
- Lines 392, 539, 612: Assert
len(logs) == 3- Lines 406, 567, 627: Access
logs[2]with comment# logs[2] does not existThis indicates a systematic issue across all OpenAI event tests. Either the length assertions are wrong, the comments are wrong, or the tests haven't been executed.
Also applies to: 539-567, 612-627
748-766: Systematic issue across all LLM provider tests.Anthropic (lines 748, 766, 804, 818) and Bedrock (lines 946, 964, 1008, 1022) tests repeat the same contradictory pattern: asserting
len(logs) == 3while commenting thatlogs[2] does not exist.This pervasive issue suggests the test updates were not validated through execution.
Also applies to: 804-818, 946-964, 1008-1022
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/opentelemetry-instrumentation-langchain/tests/test_chains.py(2 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py(3 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_llms.py(34 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_structured_output.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.pypackages/opentelemetry-instrumentation-langchain/tests/test_chains.pypackages/opentelemetry-instrumentation-langchain/tests/test_llms.pypackages/opentelemetry-instrumentation-langchain/tests/test_structured_output.py
🧬 Code graph analysis (2)
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (2)
packages/opentelemetry-instrumentation-langchain/tests/test_chains.py (1)
assert_message_in_logs(815-823)packages/opentelemetry-instrumentation-langchain/tests/test_agents.py (1)
assert_message_in_logs(245-252)
packages/opentelemetry-instrumentation-langchain/tests/test_structured_output.py (11)
packages/opentelemetry-instrumentation-langchain/tests/test_chains.py (1)
assert_message_in_logs(815-823)packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (1)
assert_message_in_logs(152-160)packages/opentelemetry-instrumentation-langchain/tests/test_llms.py (1)
assert_message_in_logs(1677-1685)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_ai21.py (1)
assert_message_in_logs(186-197)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_cohere.py (1)
assert_message_in_logs(208-219)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_imported_model.py (1)
assert_message_in_logs(147-158)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_anthropic.py (1)
assert_message_in_logs(1063-1074)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_meta.py (1)
assert_message_in_logs(740-751)packages/opentelemetry-instrumentation-bedrock/tests/traces/test_nova.py (1)
assert_message_in_logs(1362-1373)packages/opentelemetry-instrumentation-langchain/tests/test_lcel.py (1)
assert_message_in_logs(1004-1012)packages/opentelemetry-instrumentation-langchain/tests/test_tool_calls.py (1)
assert_message_in_logs(1227-1235)
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_structured_output.py (1)
59-72: LGTM!The log count updates and AI choice event validations are consistent and properly implemented. Both tests expect 2 logs and validate both entries appropriately.
Also applies to: 91-102
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (1)
96-115: LGTM!The log count updates and AI choice event validations are properly implemented. Both tests expect 2 logs and validate both the user message event and AI choice event appropriately.
Also applies to: 142-149
|
Hi @nirga, it seems that when the faulty attribute was introduced, some test must have failed (indicating the problem), but they where changed to ignore it. I've tried to revert them all, but I'm having trouble running them locally. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (1)
96-116: Test updates look correct; minor naming inconsistency.The updated log count and activated AI choice event validation properly reflect the new log emission behavior. However, the variable
_choice_eventon line 110 uses a leading underscore, which is inconsistent withchoice_eventon line 148 in the similar test below.Consider removing the leading underscore for consistency:
- _choice_event = { + choice_event = { "index": 0, "finish_reason": "unknown", "message": {"content": response["output_text"]}, } - assert_message_in_logs(logs[1], "gen_ai.choice", _choice_event) + assert_message_in_logs(logs[1], "gen_ai.choice", choice_event)
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (1)
152-160: Optional: Consider extracting the duplicated helper.The
assert_message_in_logshelper is duplicated intest_chains.py. Consider extracting it to a shared test utilities module to reduce duplication.packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
321-324: Replace debug print with proper logging.The debug print statement should use Python's logging module instead of
print()for better observability and consistency with standard logging practices.Apply this diff to use proper logging:
+import logging + +logger = logging.getLogger(__name__) + # ... (in the exception handler) except Exception as e: # If there's any issue processing usage metadata, continue without it - print(f"DEBUG: Error processing usage metadata: {e}") + logger.debug("Error processing usage metadata: %s", e) pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py(2 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py(3 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py(1 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py(1 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_chains.py(2 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py(3 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_llms.py(34 hunks)packages/opentelemetry-instrumentation-langchain/tests/test_structured_output.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/opentelemetry-instrumentation-langchain/tests/test_structured_output.py
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/config.py
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py
- packages/opentelemetry-instrumentation-langchain/tests/test_llms.py
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/init.py
- packages/opentelemetry-instrumentation-langchain/tests/test_chains.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.pypackages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py
🧬 Code graph analysis (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-245)
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (1)
packages/opentelemetry-instrumentation-langchain/tests/test_chains.py (1)
assert_message_in_logs(815-823)
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-langchain/tests/test_documents_chains.py (1)
142-149: LGTM!The test correctly expects two log entries and validates the AI choice event with empty content, consistent with the test's no-content scenario.
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
349-349: Fix correctly addresses missing upstream cache attribute constants.The change from
GenAIAttributes.GEN_AI_USAGE_CACHE_READ_INPUT_TOKENStoSpanAttributes.LLM_USAGE_CACHE_READ_INPUT_TOKENSis confirmed correct. The upstreamopentelemetry.semconv._incubating.attributeslacks these cache constants, and the customSpanAttributesin this repository correctly provides them with documentation noting they are "missing from incubating semantic conventions." No other instances of the incorrect pattern exist in the codebase, and other packages like anthropic already follow this same pattern.
|
@nirga I see that commit d9a0d7d tried to address part of the problem, but it introduced yet another bug. It is using:
but this does not exist in SpanAttributes. In this PR I rebased and fixed it for langchain, but that PR also introduces the bug in Anthropic |
nirga
changed the title
Update test assertions to expect the correct number of log events: - test_simple_lcel_with_events_with_content: expect 3 logs (system + user + choice) - test_simple_lcel_with_events_with_no_content: expect 3 logs (system + user + choice) - test_async_lcel_with_events_with_content: expect 2 logs (user + choice) - test_async_lcel_with_events_with_no_content: expect 2 logs (user + choice) - test_lcel_with_datetime_with_events_with_content: expect 3 logs (system + user + choice) - test_lcel_with_datetime_with_events_with_no_content: expect 3 logs (system + user + choice) Also uncomment and re-enable the AI choice event assertions that were previously disabled. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@ianchi it does exist - I've fixed the tests anyway, let me take another look on the PR to verify and we're good to go |
|
Hi @nirga
I see it in the codebase, but can it be that the change was never released as a new package? when you install the langchain instrumentation locally it pulls version 0.4.13 of semconv_ai an it does not have those attributes. |
|
Might be the issue indeed @ianchi - I'll investigate. This should work now in any case |
3 participants
Allowing to configure the key prefix for metadata enables use on other backends.
As an example for Langfuse v3 otel backend, you can use prefix
langfuse.metadatato make user.id and session.id available.feat(instrumentation): ...orfix(instrumentation): ....Important
Add configurable metadata key prefix to Langchain instrumentation for better backend integration.
metadata_key_prefixparameter toLangchainInstrumentor.__init__()in__init__.pyto allow customization of metadata key prefix.SpanAttributes.TRACELOOP_ASSOCIATION_PROPERTIESif not specified.metadata_key_prefixattribute toConfigclass inconfig.py._create_span()incallback_handler.pyto useConfig.metadata_key_prefixfor metadata keys.This description was created by
for 5bcaf72. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.