Skip to content

Conversation

@haneric00
Copy link
Contributor

Modified how we grab initial attributes in ChatModelStart and LLMStart

  • separated the 1 shared function for this into two separate functions, one for each parameter we use (serialized and metadata)
  • added some extra logic to getNameFromCallback to reduce duplicate code (extra if check for name value)
  • modified init.py (bug fix, accidentally deleted the code to remove handlers upon unwrapping)
  • added stop sequence to span attributes

IMPORTANT CHANGES:
chat and llm start spans are now named after the API we use ('chat ChatBedrock' or 'chat BedrockLLM' for example) rather than the model used.

  • technically against the semantic convention, probably going to change it later but at the current moment this is done to match the JS implementation

haneric00 and others added 4 commits July 25, 2025 17:14
ported updated version from the ADOT repo

changed formatting, changed some variable and class names, etc.

1 small bug fix in init.py (added back removal of handlers)

changed logic for grabbing names in llm start and chatmodel start
@yiyuan-he yiyuan-he requested a review from Copilot August 21, 2025 22:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the Python implementation to match the JavaScript implementation for LangChain v2 instrumentation. The changes modify how initial attributes are handled in ChatModelStart and LLMStart events and rename spans to use API names instead of model names.

  • Refactored attribute extraction into separate functions for serialized and metadata parameters
  • Modified span naming to use API names rather than model names to match JS implementation
  • Added stop sequences attribute support and fixed handler removal bug

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
span_attributes.py Renamed class from Span_Attributes to SpanAttributes and added GEN_AI_REQUEST_STOP_SEQUENCES attribute
callback_handler.py Refactored parameter extraction into separate functions, updated span naming logic, and fixed attribute references

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.



def _set_request_params_serialized(span, serialized, span_holder: SpanHolder):
if serialized and serialized["kwargs"]:
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code will raise a KeyError if 'kwargs' key doesn't exist in serialized dict. Should use serialized.get("kwargs") instead.

Suggested change
if serialized and serialized["kwargs"]:
if serialized and serialized.get("kwargs"):

Copilot uses AI. Check for mistakes.
@yiyuan-he
Copy link
Owner

I don't think we should go against the semantic conventions to align with JS.

Was there a reason we didn't align with semantic conventions in JS?

@yiyuan-he
Copy link
Owner

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants