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

WIP: Initial sketch of streaming tail workers #2852

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 7, 2024

Implements the key pieces of the new streaming tail workers model. Does not yet introduce the changes to make it active. This PR focuses on the core data model and restructuring of parts of the current tail workers impl to accomodate both models. Look at the tests (specifically trace-streaming-test.c++) to get a good feel for how the streaming trace is structured.

Among other changes included here...

  1. Enables tail workers in local dev. A workerd worker can now be configured to push tail events to a tail worker locally. In the current form this only supports the Version 1 tail workers but will soon support the streaming tail workers also.
  2. Adds an autogate that will be used to gate the use of the new streaming tail workers functionality

@jasnell jasnell force-pushed the jsnell/stream-trace-workers-part-1 branch 2 times, most recently from 31ff8c6 to 2dcbac7 Compare October 7, 2024 17:50
src/workerd/io/trace-common.h Show resolved Hide resolved
src/workerd/io/trace-common.h Outdated Show resolved Hide resolved
src/workerd/io/trace-common.c++ Show resolved Hide resolved
src/workerd/io/trace-common.c++ Show resolved Hide resolved
src/workerd/io/trace-common.h Show resolved Hide resolved
src/workerd/io/trace-common.h Show resolved Hide resolved
src/workerd/server/server.c++ Outdated Show resolved Hide resolved
src/workerd/server/server.c++ Show resolved Hide resolved
src/workerd/io/trace-common-test.c++ Outdated Show resolved Hide resolved
src/workerd/io/trace-common.h Show resolved Hide resolved
@jasnell jasnell force-pushed the jsnell/stream-trace-workers-part-1 branch 2 times, most recently from 794cdc2 to e40d411 Compare October 7, 2024 22:15
@jasnell jasnell marked this pull request as ready for review October 8, 2024 00:14
@jasnell jasnell requested review from a team as code owners October 8, 2024 00:14
@kentonv
Copy link
Member

kentonv commented Oct 8, 2024

Can we please get a design doc showing what the API looks like from the application developer's perspective? It's hard to figure that out from a PR.

@jasnell
Copy link
Member Author

jasnell commented Oct 8, 2024

Can we please get a design doc showing what the API looks like from the application developer's perspective...

There's already an internal wiki doc discussing this that has already been shared with you previously. Check your email. Was shared about a month ago and updated yesterday

@danlapid
Copy link
Contributor

Overall LGTM

# contains general metadata about the pipeline that is being traced.
struct Onset {
accountId @0 :UInt32;
stableId @1 :Text;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some fields in this struct are internal details which should not be revealed to application code. Why include them here?

Can you please document what the JavaScript API will look like? Your design doc only shows this huge Cap'n Proto definition but doesn't explicitly document how it looks in JavaScript.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please document what the JavaScript API will look like?

This PR is focused mostly on the internal structure and does not currently reflect the JS interface. I do not imagine that the actual JavaScript interfaces will look much different than the current tail workers API defined by api/trace.h except that I'm hoping to be able to do away with everything being a jsg::Object as opposed to just Plain Ole JavaScript objects. The specifics of the JS-facing API will be addressed in a follow up PR.

@jasnell jasnell force-pushed the jsnell/stream-trace-workers-part-1 branch 2 times, most recently from 8fde2e2 to cb7e8ba Compare October 11, 2024 19:27
@jasnell
Copy link
Member Author

jasnell commented Oct 11, 2024

@danlapid ... thanks for the review! I updated a couple of those. On the naming concern, I'm going to leave the names as they are for now. Those will be trivial to change later if necessary before this goes live.

@jasnell jasnell force-pushed the jsnell/stream-trace-workers-part-1 branch from cb7e8ba to d4723e1 Compare October 11, 2024 20:39
@jasnell jasnell force-pushed the jsnell/stream-trace-workers-part-1 branch from d4723e1 to 0a33472 Compare October 11, 2024 23:03
@jasnell
Copy link
Member Author

jasnell commented Oct 11, 2024

@fhanau ... updated here to incorporate the new ExecutionModel info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants