-
Notifications
You must be signed in to change notification settings - Fork 328
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
Start instrumenting for streaming tail workers #3323
base: main
Are you sure you want to change the base?
Conversation
console.log(...args); | ||
return (...args) => { | ||
console.log(...args); | ||
}; |
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.
Reviewers: ignore this and the below worker.js for now
context.getMetrics().reportTailEvent(context, [&] { | ||
return tracing::Onset(tracing::QueueEventInfo(kj::mv(queueName), batchSize), | ||
tracing::Onset::WorkerInfo{}, kj::none); | ||
}); |
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.
For reviewers: The getWorkerTracer()
block above is for the old style tail workers. The reportTailEvent
is for streaming tail workers
So my two cents is that your work is actually adding two things rather than one
The benefit in my mind is that you separate the concerns and the code. Your worker might have a tracer, it doesn't need to know if it's a streaming tail worker or a non streaming tail worker, it just reports traces to it. And your worker also has some trace event producer that produces the correct type of events depending on your workers trace event format. I can definitely see good reasons for users to want all four permutations. |
Well, that's generally what we're already doing. If you look, for instance, both the original Eventually what I want to end up with is, instead of the current two-separate instrumentation points this PR takes in starting to instrument for streaming, I want to have a single instrumentation point that handles both, but I want to do that as a separate step to make sure we do not accidentally break the current stuff while developing the new stuff. |
This PR starts to add instrumentation for various parts of the streaming tail workers work. Note that the approach this takes right now is generally a straw man that we need to evaluate. We have to support both legacy and streaming tail workers and right now the legacy tail workers instrumentation (using
getWorkerTracer()
) is untouched/unchanged with this approach but it means having to dual-instrument the various places in the code where we publish the event. That's... less than ideal. We can coalesce APIs here with a bit more effort. Consider this PR to be the place to discuss the various API options here. Does this work or should we try for a better/cleaner approach?@rohinlohe ... just for visibility so you can see things are moving