-
Notifications
You must be signed in to change notification settings - Fork 411
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
fix(llmobs): refactor annotation context implementation #10976
base: main
Are you sure you want to change the base?
Conversation
|
current_tags = span.get_tag(TAGS) | ||
if current_tags: | ||
span_tags.update(json.loads(current_tags)) | ||
current_tags_str = span.get_tag(TAGS) |
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.
I think it makes more sense for the current tag dictionary to be updated with the latest set of tags, not the other way around
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.
Would we need a deprecation notice for this? Since it technically changes the behavior of annotate
... and maybe some users relied on tags not being overriden
BenchmarksBenchmark execution time: 2024-10-09 21:30:14 Comparing candidate commit bc87235 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 371 metrics, 53 unstable metrics. |
# default the context id to the annotation id | ||
ctx_id = annotation_id | ||
if current_ctx is None: | ||
current_ctx = Context(is_remote=False) |
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.
is there any problem to creating a "dummy" context like this just to store a baggage item?
This PR fixes two issues with annotation contexts
on_span_start
hookI put it all in one PR since it's a single refactor of the underlying implementation for annotation contexts, but let me know if you would prefer this to be separated into two PR's -
Issue 1: Nesting
Currently, annotation contexts register a single
annotate
function to be called with specific arguments on span start when we the context manager is entered.If multiple annotation contexts are nested, the order in which the annotations happen are non-deterministic. This is because span start function hooks are stored as a set.
Instead of registering each annotate function as a separate hook, we only register one annotation function on span start that iterates through a list of annotation arguments.
Entering/exiting the annotation context enqueues/dequeues arguments from the list of annotation arguments stored in the LLMObs instance.
This ensures that annotations are applied in the order in which annotation contexts are entered.
Issue 2: Cross trace context pollution
Annotations are applied as a global function hook on span start, meaning they apply to all spans across all trace contexts.
We update this so that we store each annotation invocation with an
annotation_context_id
representing the current trace context.annotation_context_id
in the baggageannotation_context_id
from the baggage or add a new oneWhen we apply the annotations, we fetch the
annotation_context_id
from the current trace context and check that it matches theannotation_context_id
stored with each annotation.Checklist
Reviewer Checklist