Skip to content
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

Add TraceEventInfo for TraceItem #1248

Merged

Conversation

matthewdavidrodgers
Copy link
Collaborator

This adds a new TraceItem::EventType union member - TraceEventInfo. This allows event info for invocations of a trace or tail worker to show up when tailing it (i.e. attaching a tail to a tail worker).

TraceEventInfo only contains an array "traces" of objects with a single "scriptName", i.e. the script that the script you're tracing was tracing. There's hypothetically more that could be included on these items, but any sort of recursive could ballon the size of these events or even get into circular references, so it's kept simple for now.

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@matthewdavidrodgers
Copy link
Collaborator Author

I have read the CLA Document and I hereby sign the CLA

@matthewdavidrodgers
Copy link
Collaborator Author

This work is to ultimately support a wrangler tail session on a tail worker - without a differentiating event on the TraceItem, the tail session does not know what to do with the trace and drops it.

Additionally, the question remains as to whether the "traces" property on the TraceEventInfo should actually be "tails" or "tailItems" (the naming story doesn't seem to be settled there)

@irvinebroque
Copy link
Collaborator

👏 @matthewdavidrodgers

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/trace-events-for-trace-infos branch from 15c1e56 to 4f802d1 Compare September 29, 2023 15:15
matthewdavidrodgers added a commit to matthewdavidrodgers/workers-sdk that referenced this pull request Sep 29, 2023
When tailing a tail worker, messages previously had a null event
property. Following cloudflare/workerd#1248,
these events have a valid event, specifying which scripts produced
events that caused your tail worker to run.

As part of rolling this out, we're filtering out tail events in the
internal tail infrastructure, so we control when these new messages are
forward to tail sessions, and can merge this freely.

One idiosyncracy to note, however, is that tail workers always report an
"OK" status, even if they run out of memory or throw. That is being
tracked and worked on separately.
matthewdavidrodgers added a commit to matthewdavidrodgers/workers-sdk that referenced this pull request Sep 29, 2023
When tailing a tail worker, messages previously had a null event
property. Following cloudflare/workerd#1248,
these events have a valid event, specifying which scripts produced
events that caused your tail worker to run.

As part of rolling this out, we're filtering out tail events in the
internal tail infrastructure, so we control when these new messages are
forward to tail sessions, and can merge this freely.

One idiosyncracy to note, however, is that tail workers always report an
"OK" status, even if they run out of memory or throw. That is being
tracked and worked on separately.
matthewdavidrodgers added a commit to matthewdavidrodgers/workers-sdk that referenced this pull request Sep 29, 2023
When tailing a tail worker, messages previously had a null event
property. Following cloudflare/workerd#1248,
these events have a valid event, specifying which scripts produced
events that caused your tail worker to run.

As part of rolling this out, we're filtering out tail events in the
internal tail infrastructure, so we control when these new messages are
forward to tail sessions, and can merge this freely.

One idiosyncracy to note, however, is that tail workers always report an
"OK" status, even if they run out of memory or throw. That is being
tracked and worked on separately.
@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/trace-events-for-trace-infos branch from 4f802d1 to a76cbb1 Compare October 2, 2023 16:46
@matthewdavidrodgers
Copy link
Collaborator Author

this probably needs to hold a sec, we probably want to change "traces" per product decisions

@matthewdavidrodgers matthewdavidrodgers force-pushed the matthewrodgers/trace-events-for-trace-infos branch from d4ebb9c to 921020a Compare October 3, 2023 18:15
@matthewdavidrodgers
Copy link
Collaborator Author

this probably needs to hold a sec, we probably want to change "traces" per product decisions

Updated to change "traces" to "consumedEvents" as well as updating class names (TraceEventInfo -> TailEventInfo and TraceEventInfo::TraceItem -> TailEventInfo::TailItem) so that generated ts types match product names better.

This is now ready to go

matthewdavidrodgers added a commit to matthewdavidrodgers/workers-sdk that referenced this pull request Oct 3, 2023
When tailing a tail worker, messages previously had a null event
property. Following cloudflare/workerd#1248,
these events have a valid event, specifying which scripts produced
events that caused your tail worker to run.

As part of rolling this out, we're filtering out tail events in the
internal tail infrastructure, so we control when these new messages are
forward to tail sessions, and can merge this freely.

One idiosyncracy to note, however, is that tail workers always report an
"OK" status, even if they run out of memory or throw. That is being
tracked and worked on separately.
This adds a new TraceItem::EventType union member - TraceEventInfo. This
allows event info for invocations of a trace or tail worker to show up
when tailing it (i.e. attaching a tail to a tail worker).

TraceEventInfo only contains an array "traces" of objects with a single
"scriptName", i.e. the script that the script _you're_ tracing was
tracing. There's hypothetically more that could be included on these
items, but any sort of recursive could ballon the size of these events
or even get into circular references, so it's kept simple for now.
@matthewdavidrodgers matthewdavidrodgers merged commit 1d9158a into main Oct 5, 2023
7 of 8 checks passed
@matthewdavidrodgers matthewdavidrodgers deleted the matthewrodgers/trace-events-for-trace-infos branch October 5, 2023 15:14
matthewdavidrodgers added a commit to cloudflare/workers-sdk that referenced this pull request Oct 6, 2023
When tailing a tail worker, messages previously had a null event
property. Following cloudflare/workerd#1248,
these events have a valid event, specifying which scripts produced
events that caused your tail worker to run.

As part of rolling this out, we're filtering out tail events in the
internal tail infrastructure, so we control when these new messages are
forward to tail sessions, and can merge this freely.

One idiosyncracy to note, however, is that tail workers always report an
"OK" status, even if they run out of memory or throw. That is being
tracked and worked on separately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants