Initial sketch of streaming tail workers #2852
Open
+3,344
−1,075
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasnell
requested review from
byule,
irvinebroque,
anonrig,
mikenomitch and
kflansburg
jasnell
force-pushed
the
jsnell/stream-trace-workers-part-1
branch
2 times, most recently
from
31ff8c6
to
2dcbac7
Compare
anonrig
reviewed
Oct 7, 2024
jasnell
force-pushed
the
jsnell/stream-trace-workers-part-1
branch
2 times, most recently
from
794cdc2
to
e40d411
Compare
|
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. |
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
reviewed
Oct 11, 2024
danlapid
reviewed
Oct 11, 2024
danlapid
reviewed
Oct 11, 2024
danlapid
reviewed
Oct 11, 2024
danlapid
reviewed
Oct 11, 2024
danlapid
reviewed
Oct 11, 2024
|
Overall LGTM |
kentonv
reviewed
Oct 11, 2024
jasnell
force-pushed
the
jsnell/stream-trace-workers-part-1
branch
2 times, most recently
from
8fde2e2
to
cb7e8ba
Compare
|
@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. |
danlapid
approved these changes
Oct 11, 2024
Adds the ability for a workerd worker configuration to specify a tail worker configuration. This is useful for testing tail worker development locally, which up to now has not been possible.
* Collapse Info events into the Onset * Remove ActorFlushInfo * Other cleanups
No need to express it explicitly. Can be inferred if necessary based on span outcome.
Original thought on these is that using an optional numeric key would allow for more efficient encoding. However, it makes the implementation more complicated so let's just use string keys.
jasnell
force-pushed
the
jsnell/stream-trace-workers-part-1
branch
from
463db3b
to
af4a3ff
Compare
Use SpanCose consistently even for the root span/stream. Rework the StreamingTrace interface to better handle rootspan.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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...