Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Change recording to create a span of type "record_root" #1703
Changes from all commits
2b4a3a9
49d79dc
7207252
f433019
2b03079
a8e4955
a6c0b63
0ca7ebc
1c6cbc2
3059d79
021a43c
42f0575
fb7cf3b
9c313b4
20bb651
d0b4398
99dcb44
834d6e7
eb61153
7fd7de2
aad3ffa
9e27b0f
8004a55
f35f2b0
4ed4546
c1516c7
85f2e62
47672f2
aafae55
3d23e0b
fbc6e60
efa8c6e
cf97904
dc98c62
7dae6ff
e1b685c
bf80a11
e64f8ee
79b157f
0e3564b
4528258
a1c461c
1c34cd8
7814807
c390f94
fe12d1b
a7e3f53
5dbc83a
3f1607c
2a6e699
41b37b0
2067909
c61dda6
402c0e5
364f6aa
17619e6
d300f67
659f82c
bc26f34
86850df
57b6809
14869e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.
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:
set_baggage
, which returns a new, updated contextcontext_api.attach
- this returns a token that can then be used to remove it.context
is by telling it the baggage you want it to detach viacontext.detach(token)
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:
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.
I know we discussed this before, but my memory's failing me - what does the pseudocode for that look like? is it:
just so I can experiment with it a little more :)
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:
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