-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(llmobs): llmobs-specific context manager #10767
base: main
Are you sure you want to change the base?
Conversation
|
BenchmarksBenchmark execution time: 2025-01-24 23:20:28 Comparing candidate commit 8f0a797 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 364 metrics, 2 unstable metrics. scenario:iast_aspects-ospathbasename_aspect
scenario:iast_aspects-ospathdirname_aspect
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apm-core files LGTM!
Datadog ReportBranch report: ✅ 0 Failed, 130 Passed, 1378 Skipped, 4m 36.31s Total duration (34m 44.56s time saved) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT! Just a couple small nits, but feel free to ignore if they're intentional. Really cool stuff, walked through the logic a couple times and seems OK to me.
a7cd6ad
to
2f04735
Compare
114d58f
to
e9e64ca
Compare
e9e64ca
to
5feb4ba
Compare
Datadog ReportBranch report: ✅ 0 Failed, 170 Passed, 1338 Skipped, 5m 3.79s Total duration (35m 6.26s time saved) |
Gotta look at if relying on contextvars means we don't need to actively inject llmobs propagation into the futures threading integration. |
|
||
_inject_llmobs_parent_id(span_context) | ||
LLMObs._inject_llmobs_context(headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of injecting into the headers, let's add it into the context and let the injection mechanisms propagate that into the headers.
if ctx[1] is not None and config._llmobs_enabled: | ||
from ddtrace.llmobs import LLMObs | ||
|
||
LLMObs._instance._llmobs_context_provider.activate(ctx[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explore using signals instead of importing!
MLOB-1342
Summary
Public facing changes:
export_span(), annotate()
) that allow an optional span argument will now default to finding the current active LLMObs span rather than the current active APM span.Private changes:
LLMObs._instance.current_span()
, returns the current active LLMObs-generated (integration, SDK) span.LLMObs._instance._current_trace_context()
, returns current LLMObs context (which can represent both a span or a distributed span)_dd
which is a str/str dictionary containing the span/trace IDs of the APM span to correlate with. Currently these are the same span/trace IDs as the LLMObs span/trace ID, but this unlocks future steps of using independent span/trace IDs.Previous behavior
LLMObs spans are based on APM spans, except LLMObs spans' parenting involves only other LLMObs spans. So with a potential trace structure containing a mixture of APM-specific and LLMObs spans, like:
LLMObs only cares about the LLMObs spans, where span C's parent is the root span, even though in APM it would be span B. Combined with distributed tracing and multithreading, this makes it not so easy to determine that "correct" (read LLMObs) parenting tree for traces submitted to LLM Observability.
Problems with previous approach
Previously we worked around this by traversing the span's local parent tree and finding the next LLM-type span on both span start and finish for non-distributed cases, and for distributed cases we would attach the parent ID on the span context's meta field to be propagated in distributed request headers. However attaching things to the span context meta was not suitable long-term due to a couple factors:
Example ugly workaround
Any meta fields set on the context object gets propagated as span tags on all subsequent spans in the trace on span start time, except for the spans in the first service of a trace which get propagated at span finish time. Fixing this resulted in overriding these span tags on span start and more checks on span finish.Current approach
Instead of being dependent on a Context object that doesn't quite fit our use case and trying to make it fit our use case, we simply keep track of our own active LLMObs span/context:
LLMObsContextProvider
handles keeping track of the current active LLMObs span viaactive()
andactivate()
LLMObsContextProvider._activate_llmobs_span()
and set the llmobs parent ID as a tag at span start time.(called by
LLMObs._start_span()
andBaseLLMIntegration.trace(submit_to_llmobs=True)
and the bedrock integration).LLMObs.inject_distributed_headers
now uses the LLMObsContextProvider to inject the active llmobs span's ID into request headersLLMObs.activate_distributed_headers()
now uses the LLMObsContextProvider to activate the extracted llmobs context to continue the trace in a distributed case.trace_utils.activate_distributed_headers()
now includes automatic llmobs context activation if llmobs is enabled. I've config-gated this so that LLMObs is only imported for llmobs users (same forHTTPPropagator.inject()
.By keeping track of our own active LLMObs spans, spans submitted to LLM Observability have an independent set of span and parent IDs, even if the span and trace IDs are shared with APM spans for now. This is the first step to decoupling from tracer internals.
Next steps
We can go further by generating LLMObs-specific span/trace IDs which are separate from APM. This will solve some edge cases with traces involving mixed APM/LLMObs spans.
Checklist
Reviewer Checklist