-
Notifications
You must be signed in to change notification settings - Fork 198
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
Enable exporting spans to snowflake stage if a TruLensSnowflakeSpanExporter
is provided
#1708
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
83c4d06
to
c6a8462
Compare
8d2cece
to
b4bbdbf
Compare
SnowflakeConnector
is provided
tests/unit/static/golden/test_otel_instrument__test_instrument_decorator.csv
Outdated
Show resolved
Hide resolved
@@ -972,6 +970,22 @@ async def __aexit__(self, exc_type, exc_value, exc_tb): | |||
|
|||
return | |||
|
|||
def __call__(self, *, run_name: str, input_id: str): | |||
if not self.session.experimental_feature( |
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.
sorry can you explain to me what's going on in this function?
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.
haha - this might not have been the best way to achieve this.
The goal is to enable syntax that looks like:
with custom_app(run_name='run_name', input_id='input_id') as recording:
test_app.respond_to_query(input)
to achieve this - custom_app(run_name='run_name', input_id='input_id')
should return an object that is a recording context (in our case a class with the __enter__
and __exit__
methods), and that the setting of the run_name
and input_id
shouldn't taint the object itself.
hence - the approach here is, when custom_app
is called, it:
- Checks that OTEL tracing is enabled
- creates a clone of itself using
model_construct
+model_dump
that is classed toOTELRecordingContext
(see https://github.com/truera/trulens/pull/1708/files/5a555840da38b280a0b3c3cbabb40cbd8a3e93c4#diff-25f7f4954d77bc71539bcd8d1e7b3133552596b468bf53a6962b636b176de682R96) - then performs the normal recording context duties
61d87a8
to
9e5b60b
Compare
if not core_instruments.Instrument._have_context(): | ||
raise RuntimeError(core_endpoint._NO_CONTEXT_WARNING) | ||
|
||
def _prevent_invalid_otel_syntax(self): |
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.
why did you name this function this? Like what do you mean by otel tracing syntax?
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.
basically trying to force the user to use syntax that looks like:
with app():
...
or
with app(run_name=run_name, input_id=input_id):
...
as opposed to the conventional
with app:
...
this is because we need to be able to return a different object in order to avoid the possibility of polluting the App
SnowflakeConnector
is providedTruLensSnowflakeSpanExporter
is provided
# For use as a context manager. | ||
def __enter__(self): | ||
if not core_instruments.Instrument._have_context(): | ||
raise RuntimeError(core_endpoint._NO_CONTEXT_WARNING) |
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 to force the user to use syntax that looks like:
with app():
...
which will allow us to return a wrapping class around app to hold the OTEL properties, to avoid the risk of OTEL properties being mixed between invocations
if not core_instruments.Instrument._have_context(): | ||
raise RuntimeError(core_endpoint._NO_CONTEXT_WARNING) | ||
|
||
def _prevent_invalid_otel_syntax(self): |
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.
basically trying to force the user to use syntax that looks like:
with app():
...
or
with app(run_name=run_name, input_id=input_id):
...
as opposed to the conventional
with app:
...
this is because we need to be able to return a different object in order to avoid the possibility of polluting the App
# There might be nested records - in those scenarios use the outer one. | ||
if not get_baggage(SpanAttributes.RECORD_ID): | ||
self.attach_to_context(SpanAttributes.RECORD_ID, otel_record_id) | ||
|
||
self.attach_to_context(SpanAttributes.APP_NAME, self.app.app_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.
Can you do the same as RECORD_ID For the other attributes? Since I guess the APP_NAME and other stuff aren't thread safe since if you call the child LLM app then continue working the baggage will get changed on you.
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.
changed attach_to_context
to check for existence + ensure that value is non null
Description
Enable exporting spans to a snowflake stage if
SnowflakeConnector
is provided.Other details good to know for developers
N/A
Type of change
not work as expected)
Important
Add
TruLensSnowflakeSpanExporter
to export spans to Snowflake stage and refactor OTEL tracing inTruSession
.TruLensSnowflakeSpanExporter
inexporter/snowflake.py
to export spans to Snowflake stage._export_to_snowflake_stage
method for exporting spans as protobuf files.span_context
andtokens
fromApp
class inapp.py
._prevent_invalid_otel_syntax
inapp.py
.convert_to_any_value
toexporter/utils.py
.TruSession
to handleSpanExporter
andTracerProvider
insession.py
.experimental_force_flush
method toTruSession
for flushing spans.test_exporter.py
andtest_otel_instrument.py
to reflect new exporter functionality.api.trulens.3.11.yaml
andapi.trulens_eval.3.11.yaml
.This description was created by for 4e27038. It will automatically update as commits are pushed.