Add non-top level tracing span context to js rpc calls. by mar-cf · Pull Request #4403 · cloudflare/workerd · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

Add non-top level tracing span context to js rpc calls. #4403

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add non-top level tracing span context to js rpc calls. #4403

wants to merge 1 commit into from

Conversation

mar-cf
Copy link
Contributor

@mar-cf mar-cf commented Jun 24, 2025

No description provided.

@mar-cf mar-cf requested a review from fhanau June 24, 2025 08:52
@mar-cf mar-cf force-pushed the mar/stw-trace-non-top branch from 7dba8df to dd161b0 Compare June 24, 2025 12:29
@fhanau fhanau requested a review from jasnell June 27, 2025 04:56
@fhanau
Copy link
Contributor

fhanau commented Jun 27, 2025

@jasnell we believe this could fix errors for tail worker events being reported before the onset event – possibly because we did not report an onset for non-top level JSRPC so far. I think this is worth trying

fhanau
fhanau reviewed Jun 27, 2025
View reviewed changes
src/workerd/api/worker-rpc.c++
@@ -1511,7 +1511,10 @@ class TransientJsRpcTarget final: public JsRpcTargetBase {
}

void addTrace(jsg::Lock& js, IoContext& ioctx, kj::StringPtr methodName) override {
// TODO(someday): Trace non-top-level calls?
KJ_IF_SOME(t, mapAddRef(ioctx.getWorkerTracer())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the mapAddRef() call needed here?

@fhanau
Copy link
Contributor

fhanau commented Jun 27, 2025

@jasnell we believe this could fix errors for tail worker events being reported before the onset event – possibly because we did not report an onset for non-top level JSRPC so far. I think this is worth trying

I think as long as we guard this behind the streaming tail worker compat flag, there is less risk for breakage – should be something like ioCtx.getApi().getFeatureFlags().getStreamingTailWorker(). But I don't understand the JSRPC implementation well, we need to avoid adding the onset twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fhanau fhanau fhanau left review comments

@jasnell jasnell Awaiting requested review from jasnell

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mar-cf @fhanau

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.