Conversation
|
@claude review |
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
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.
Summary
langfuse_trace_namefrom LangChain root metadata intopropagate_attributes()so root and child spans receivelangfuse.trace.namelangfuse_trace_namefrom observation metadata once it is treated as a trace-level attributeon_chain_endruns in a copied async contextVerification
uv run --frozen pytest tests/unit/test_langchain.py -quv run --frozen ruff check langfuse/langchain/CallbackHandler.py tests/unit/test_langchain.pyCloses #13023
Refs LFE-9178
Disclaimer: Experimental PR review
Greptile Summary
This PR adds
langfuse_trace_nameto the set of root-chain trace attributes that are extracted from LangChain metadata and propagated to all spans viapropagate_attributes(), and strips the key from raw observation metadata to avoid it leaking as a raw metadata entry. Two regression tests are added: one for the metadata-based trace-name propagation across root and child spans, and one for the scenario whereon_chain_endfires in a copied async context.Confidence Score: 5/5
Safe to merge — changes are additive, well-scoped, and covered by targeted regression tests.
All three change sites (attribute extraction, propagation call, strip list) are consistent with existing patterns for
langfuse_user_id/langfuse_session_id. No pre-existing logic is altered, double-exit of the propagation context manager is guarded by the null check in_exit_propagation_context, and both new code paths are covered by new tests. No P0/P1 findings.No files require special attention.
Important Files Changed
langfuse_trace_nameextraction in_parse_langfuse_trace_attributes, passestrace_nametopropagate_attributes()at root chain start, and adds the key tolangfuse_trace_attribute_keysso it is stripped from raw observation metadata for all spans.TRACE_NAMEattribute is set on root and child spans andlangfuse_trace_nameis stripped from root-span metadata, and one verifying correct export whenon_chain_endruns in a copied async context.Sequence Diagram
sequenceDiagram participant LC as LangChain Runtime participant CB as CallbackHandler participant PA as propagate_attributes() participant CS as Child Spans LC->>CB: on_chain_start(metadata={langfuse_trace_name: my-trace}, parent_run_id=None) CB->>CB: _parse_langfuse_trace_attributes() extracts trace_name CB->>PA: propagate_attributes(trace_name=my-trace, ...) PA-->>CB: context manager entered (sets langfuse.trace.name on active span) CB->>CB: start_observation() with metadata stripped of langfuse_trace_name LC->>CB: on_chain_start(metadata={langfuse_trace_name: my-trace}, parent_run_id=id) CB->>CS: start_observation() — inherits langfuse.trace.name via OTel context LC->>CB: on_chain_end(run_id=root) CB->>PA: __exit__() — propagation context torn downReviews (1): Last reviewed commit: "fix(langchain): propagate trace name met..." | Re-trigger Greptile