fix(langchain): Added new method for fetching model name from association metadata #3237
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (5)
WalkthroughThis change introduces a new helper function to extract a model name from association metadata and updates the Changes
Sequence Diagram(s)sequenceDiagram
participant CallbackHandler
participant ContextAPI
participant SpanUtils
CallbackHandler->CallbackHandler: on_llm_end()
alt Model name in response metadata
CallbackHandler->CallbackHandler: Use model name from response
else Model name not found
CallbackHandler->ContextAPI: Check for get_value
ContextAPI->CallbackHandler: Return association_properties
CallbackHandler->SpanUtils: _extract_model_name_from_association_metadata(association_properties)
SpanUtils-->CallbackHandler: Return model name or "unknown"
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure ✨ Finishing Touches
🧪 Generate 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. 🪧 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 44106ae in 1 minute and 16 seconds. Click for details.
- Reviewed
38lines 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:511
- Draft comment:
Consider adding an inline comment here to explain that if the response metadata doesn't yield a model name, the association metadata (from context_api) is used as a fallback. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
2. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py:372
- Draft comment:
Consider adding a docstring for _extract_model_name_from_association_metadata to clarify what metadata is expected and that it retrieves the 'ls_model_name', returning 'unknown' if not found. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
3. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py:373
- Draft comment:
Typo: There's an extra space between 'None' and ')' in the function parameter declaration. Consider removing the space for consistency. - 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% While the comment is technically correct about the extra space, this is an extremely minor formatting issue. Such minor spacing issues are typically handled automatically by code formatters like black, pylint, or other tools. It doesn't affect functionality and isn't worth a human reviewer's attention. The comment is factually accurate. Perhaps maintaining strict formatting consistency is important for this project's standards. Even if formatting is important, this is too minor to warrant a manual review comment. This should be handled by automated tools. Delete this comment as it's too minor of a formatting issue to warrant manual review attention.
Workflow ID: wflow_soMz3T6dtXfrolRh
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: 0
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
371-372: Remove unnecessary empty lines.The extra empty lines before the new function are not needed and don't follow the existing spacing pattern in the file.
- - +
📜 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(2 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
_extract_model_name_from_association_metadata(373-376)
🔇 Additional comments (3)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
373-376: LGTM! Clean implementation of the helper function.The function correctly implements the fallback mechanism for extracting model names from association metadata. The logic is straightforward, handles edge cases appropriately, and follows the existing code conventions.
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/callback_handler.py (2)
40-40: LGTM! Proper import of the new helper function.The import is correctly added and follows the existing import pattern from the span_utils module.
512-514: LGTM! Well-implemented fallback mechanism.The integration properly implements the cascading fallback approach for model name extraction. The safety check for
hasattr(context_api, "get_value")is a good defensive programming practice, and the logic correctly uses the association_properties from the current context.
|
@nirga Hey Nir, this PR extends the existing logic for fetching the model name in metrics export. Previously, as per @LakshmiPriyaSujith ’s PR, the model name was fetched from the response metadata. However, in some cases, the model name was missing in the response metadata, and as a result, the metrics were showing the model name as "unknown". This PR adds a fallback mechanism: After these changes, the model name is correctly populated in the exported metrics where it was previously "unknown". |
…tion metadata (#3237) Co-authored-by: Nir Gazit <[email protected]>
feat(instrumentation): ...orfix(instrumentation): ....Important
Adds method to fetch model name from association metadata in
span_utils.pyand updateson_llm_end()incallback_handler.pyto use it as a fallback._extract_model_name_from_association_metadata()inspan_utils.pyto fetch model name from association metadata.on_llm_end()incallback_handler.pyto use_extract_model_name_from_association_metadata()if model name is not found in response metadata._extract_model_name_from_association_metadata()returns model name fromassociation_propertiesor "unknown" if not found.This description was created by
for 44106ae. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit