feat: propagate trace attributes onto all child spans on update #1385
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| @_agnosticcontextmanager | ||
| def session( | ||
| self, id: str, *, as_baggage: bool = False |
There was a problem hiding this comment.
I think we should rename as_baggage to something more immediately descriptive on what the impact is. Something like propagate_via_http makes it both clear that this is propagation related and comes with security considerations (users might be unfamiliar with OTEL in general and baggage in particular). We could also consider dropping this entirely and only allow enabling it via env var only as users probably won't dynamically decide in runtime whether to use baggage or not
There was a problem hiding this comment.
For propagation, we also need to consider / test the multiprocessing case in Python, where users use Celery workers that are separate Python processes to handle background tasks. Trace attributes must be propagated there, too
|
Thank you for putting this PR together @Steffen911 -- This is exactly what we need to reliably attribute users/organization ids to all our spans 👍 |
langfuse/_client/client.py
Outdated
| f"Metadata values must be strings, got {type(v)} for key '{k}'" | ||
| ) | ||
| del metadata[k] | ||
| if len(v) > 200: |
There was a problem hiding this comment.
We ran into an error here in production where v is None and this length check failed.
File "/app/documents_parser/telemetry/middleware.py", line 41, in dispatch
with client.with_attributes(
^^^^^^^^^^^^^^^^^^^^^^^
File "/app/.venv/lib/python3.12/site-packages/opentelemetry/util/_decorator.py", line 61, in __enter__
return next(self.gen) # type: ignore
^^^^^^^^^^^^^^
File "/app/.venv/lib/python3.12/site-packages/langfuse/_client/client.py", line 440, in with_attributes
if len(v) > 200:
^^^^^^
TypeError: object of type 'NoneType' has no len()
There was a problem hiding this comment.
It's a bug on our side, but I think the package should probably warn and fail gracefully.
| host | ||
| if host is not None | ||
| else os.environ.get(LANGFUSE_HOST, "https://cloud.langfuse.com") | ||
| ) |
There was a problem hiding this comment.
The original version is more readableand to my understanding they do the same. If this is true, should we revert this?
There was a problem hiding this comment.
yes, they are both equivalent
| metadata (dict): Additional metadata to associate with all spans in the context. Values must be strings and are truncated to 200 characters. | ||
| as_baggage (bool, optional): If True, stores the values in OpenTelemetry baggage | ||
| for cross-service propagation. If False, stores only in local context for | ||
| current-service propagation. Defaults to False. |
There was a problem hiding this comment.
We should add a sentence here that this will be added to the headers of all outgoing HTTP requests. I would add the warning from below up here as not everyone is reading everything.
langfuse/_client/client.py
Outdated
| # Process metadata | ||
| if metadata is not None: | ||
| # Truncate values with size > 200 to 200 characters and emit warning including the ky | ||
| for k, v in metadata.items(): |
There was a problem hiding this comment.
Should we also have an overall limit fo e.g. 4k characters?
There was a problem hiding this comment.
@maxdeichmann IMO that would be complicated to enforce and eventually, this is an issue that will surface quickly on the client side. I would highlight it in the documentation, but don't complicate the happy path further.
There was a problem hiding this comment.
Well the risk is here that our users run into failing APIs as the header is too large to be processed. I thought we could add a simple logic such as: keeping a count of added characters to the header. If threshhold exceeded, stop adding key/values to the metadata.
langfuse/_client/client.py
Outdated
| self, | ||
| session_id: Optional[str] = None, | ||
| user_id: Optional[str] = None, | ||
| metadata: Optional[dict[str, str]] = None, |
There was a problem hiding this comment.
Should we highlight somehow that this metadata is only to be used if it should be set on all children? I'm wondering whether people understand and make use of the difference for setting metadata on current span vs all spans?
There was a problem hiding this comment.
Yes, I agree with the sentiment! Options:
- with_trace_attributes
- with_context_attributes
- with_propagated_attributes
...
Lets quickly talk sync about this. Nailing this is key.
There was a problem hiding this comment.
We should think about this from the perspective on how this is consumed, namely via context manager. so the 'with' is already there.
from langfuse import get_client
langfuse = get_client()
...
with langfuse.correlation_context(session_id='my-session'):
# do stuff
passI prefer correlation_context or set_correlation_context as it makes sure that
- this is not necessarily related to a 'trace'
- this has some propagation included ('context')
- is used for aggregating / correlating multiple spans ('correlation')
There was a problem hiding this comment.
- propagation_context
- propagated_context
- distributed_context
- trace_scope
- trace_context
- shared_attributes
|
@Steffen911 lets talk about function naming of the new functions and then we are good to go here. Marc should also see them. |
|
Docs required max length for trace metadata ascii-us characters required https://opentelemetry.io/docs/specs/otel/context/api-propagators/?utm_source=chatgpt.com#textmap-propagator |
Replaces #1349
Important
Introduces context propagation for session, user, and metadata attributes using OpenTelemetry, deprecating older methods, with comprehensive tests added.
with_attributes()inclient.pyfor session, user, and metadata propagation.update_current_trace()andupdate_trace()inclient.pyandspan.py.LangfuseSpanProcessor.on_start()inspan_processor.py.LANGFUSE_CTX_USER_ID,LANGFUSE_CTX_SESSION_ID,LANGFUSE_CTX_METADATAinconstants.py.test_core_sdk.py.This description was created by
for d210b49. You can customize this summary. It will automatically update as commits are pushed.
Disclaimer: Experimental PR review
Greptile Overview
Updated On: 2025-09-30 09:33:53 UTC
Summary
This review covers only the changes made since the last review, not the entire PR.This PR introduces a comprehensive context propagation system that automatically propagates trace attributes (
session_id,user_id,metadata) from parent contexts to child spans using OpenTelemetry's context and baggage mechanisms. The implementation adds three new context managers (session(),user(),metadata()) to both the main Langfuse client and individual span objects, enabling automatic attribute inheritance without requiring manual specification on each child span.The core architecture leverages OpenTelemetry context variables for local propagation within a single service and optional baggage for cross-service distributed tracing. When a context manager is entered, it sets the appropriate context variables and optionally baggage entries. The
LangfuseSpanProcessor.on_start()method then automatically applies these propagated attributes to new spans during creation.The implementation also deprecates the older
update_current_trace()andupdate_trace()methods with proper deprecation warnings, indicating a shift toward this more systematic context-based approach. The feature integrates well with the existing OpenTelemetry-based architecture and follows OpenTelemetry best practices for context propagation.Comprehensive test coverage validates the propagation behavior across various scenarios including nested contexts, baggage propagation for cross-service use cases, and both client-level and span-level context managers. The tests demonstrate proper attribute inheritance and context cleanup behavior.
Important Files Changed
Changed Files
Confidence score: 5/5