-
Notifications
You must be signed in to change notification settings - Fork 197
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
Change recording to create a span of type "record_root" #1703
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
9ab5db6
to
049b404
Compare
712f97b
to
c9db9a1
Compare
tracer = trace.get_tracer_provider().get_tracer(TRULENS_SERVICE_NAME) | ||
|
||
# Calling set_baggage does not actually add the baggage to the current context, but returns a new one | ||
# To avoid issues with remembering to add/remove the baggage, we attach it to the runtime context. |
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.
s/To/to
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 also don't really get this token business, can you enlighten me?
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.
from what I can tell - to set the baggage you need to:
- call
set_baggage
, which returns a new, updated context - attach it to the current context with
context_api.attach
- this returns a token that can then be used to remove it. - Once the record is over, we want to remove it, and the way to do it via OTEL
context
is by telling it the baggage you want it to detach viacontext.detach(token)
root_span.set_attribute(SpanAttributes.RECORD_ROOT.APP_ID, self.app_id) | ||
root_span.set_attribute( | ||
SpanAttributes.RECORD_ROOT.RECORD_ID, otel_record_id | ||
) |
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.
the semantic convention that Piotr laid out also has the MAIN_INPUT
/MAIN_OUTPUT
/MAIN_ERROR
, but I don't see how we can get that even during __exit__
, so I'm inclined to say we can remove them. Though that raises the question of, how will the UI know what the input and output are to display. It's too close to the finish line of 2024 that I don't want to investigate how this is determined currently but can next year haha.
There's also TOTAL_COST
but I'm not as worried about that.
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.
yeah we should chat about this - I think one idea I had while typing this up was to omit the record_root span type, and instead:
- Attributes that are initially proposed to be stored in record_root should now be stored in the baggage name/version/id/record_id)
- Every trace in the record should track all of the attributes above
- the root of the record is essentially the span with no parent
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.
We still need the root span type though to denote when we start a record though since if you create a otel trace before you start calling your app a bunch of times they'll all be part of the same trace but we want them to be separate records.
But more importantly, I'm confused, how does this help us determine the MAIN_INPUT
/MAIN_OUTPUT
/MAIN_ERROR
?
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.
if you create a otel trace before you start calling your app a bunch of times they'll all be part of the same trace but we want them to be separate records.
I know we discussed this before, but my memory's failing me - what does the pseudocode for that look like? is it:
with tru_app as recording:
tru_app.respond_to_query(query)
tru_app.do_something_else(query)
just so I can experiment with it a little more :)
But more importantly, I'm confused, how does this help us determine the MAIN_INPUT/MAIN_OUTPUT/MAIN_ERROR?
we aren't doing this in code as of yet, but I'm thinking that we should track the input/output/error for every span, so the way we determine the main input/output/error semantically would be:
- Find the span with no parent
- Use its input/output/error as the main input/output/error for the span.
@@ -51,6 +58,10 @@ def wrapper(*args, **kwargs): | |||
span.set_attribute( | |||
"parent_span_id", parent_span.get_span_context().span_id | |||
) | |||
span.set_attribute( |
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.
we want app/run id in the baggage as well I guess, but given that it's not totally clear yet no point in doing it now I suppose.
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'll put the app id in baggage for now I guess since that seems pretty good to me. For run_id it's a little more ambiguous what the shape of the API will look like so I'll leave that out for now.
6b063f5
to
d300f67
Compare
src/core/trulens/core/app.py
Outdated
@@ -421,6 +422,18 @@ def tru(self) -> core_connector.DBConnector: | |||
pydantic.PrivateAttr(default_factory=dict) | |||
) | |||
|
|||
tokens: list[object] = [] |
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 list[object]
is worse than List[object]
since the former doesn't work in python 3.8 which we do support.
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.
also, is the type of the token varying or something? Why object
?
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.
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.
@@ -84,6 +90,20 @@ def wrapper(*args, **kwargs): | |||
# It's on the user to deal with None as a return value. | |||
func_exception = e | |||
|
|||
span.set_attribute("name", func.__name__) |
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 this something that the event table expects or something?
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.
yeah I'm trying to follow the schema where possible - see https://docs.snowflake.com/en/developer-guide/logging-tracing/event-table-columns#for-span-record-type
root_span.set_attribute(SpanAttributes.RECORD_ROOT.APP_ID, self.app_id) | ||
root_span.set_attribute( | ||
SpanAttributes.RECORD_ROOT.RECORD_ID, otel_record_id | ||
) |
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.
We still need the root span type though to denote when we start a record though since if you create a otel trace before you start calling your app a bunch of times they'll all be part of the same trace but we want them to be separate records.
But more importantly, I'm confused, how does this help us determine the MAIN_INPUT
/MAIN_OUTPUT
/MAIN_ERROR
?
# See https://github.com/open-telemetry/opentelemetry-python/issues/2432#issuecomment-1593458684 | ||
context_api.detach(self.tokens.pop()) | ||
|
||
if self.span_context: |
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 this ever not be true?
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 doubt it, but didn't think it would hurt to include it, if nothing, at least for the type checking haha
Description
Updated the app context manager to be able to create a span of type "record_root"
Other details good to know for developers
For reference, this is the current set of spans as produced in the notebook:
Type of change
not work as expected)
Important
Add
record_root
span type to app context manager for enhanced tracing with OpenTelemetry.record_root
to app context manager inapp.py
.App
class ininstrument.py
to managerecord_root
spans with OpenTelemetry.SPAN_TYPE
andRECORD_ID
attributes intrace.py
for span identification.trulens.experimental.otel_tracing.core.app
totrulens.experimental.otel_tracing.core.instrument
inapp.py
.This description was created by for 14869e1. It will automatically update as commits are pushed.