Skip to content

Do not send named entrypoint errors to Sentry #6261

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

Open
maxmcd wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −2
Open

Do not send named entrypoint errors to Sentry #6261

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/workerd/io/worker.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2353,9 +2353,9 @@ kj::Maybe<kj::Own<api::ExportedHandler>> Worker::Lock::getExportedHandler(
return kj::none;
} else {
if (worker.impl->actorClasses.find(n) != kj::none) {
LOG_ERROR_PERIODICALLY("worker is not an actor but class name was requested", n);
LOG_ERROR_PERIODICALLY("NOSENTRY worker is not an actor but class name was requested", n);
} else {
LOG_ERROR_PERIODICALLY("worker has no such named entrypoint", n);
LOG_ERROR_PERIODICALLY("NOSENTRY worker has no such named entrypoint", n);
Copy link
Copy Markdown
Member

@a-robinson a-robinson Mar 5, 2026

Choose a reason for hiding this comment

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

The ideal here would be to not log this at all specifically for cross-account dynamic dispatch (or maybe for all dynamic dispatch), rather than not sending it to sentry in any case. The reasoning being that in that case it's the caller's fault rather than being something wrong with the runtime / validator / config APIs.

I imagine the original intent of this log was to catch issues where the config APIs managed to configure a route/trigger to a worker with an incorrect named entrypoint, and that case is still a real error worth raising to sentry if it happens.

If it's too hard to differentiate here then this is better than letting the log be noisy, but I wanted to clarify in case the change was done this way was based on a misunderstanding rather than due to the ideal version being too hard.

Copy link
Copy Markdown
Collaborator

@danlapid danlapid Mar 5, 2026

Choose a reason for hiding this comment

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

I wholly agree here.
Generally we should shy away from NOSENTRY as much as possible.
If something is a user error it shouldn't be logged at all BUT it should be a good user error - jsg.Error
If something is a platform error it should be in sentry.

This sounds like it should be converted to a jsg.Error under dynamic dispatch regardless of cross account right?

Copy link
Copy Markdown
Collaborator

@jasnell jasnell Mar 5, 2026

Choose a reason for hiding this comment

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

Ah, got it. I'd missed that bit of context. I had interpreted this as a thing we want to watch for (log) but not necessarily track or alert on (sentry)... Hadn't connected the " no, this is an error for the user to do something about" dots mentally. In that case, absolutely agree

Copy link
Copy Markdown
Member

@a-robinson a-robinson Mar 5, 2026

Choose a reason for hiding this comment

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

This sounds like it should be converted to a jsg.Error under dynamic dispatch regardless of cross account right?

Right.

}

KJ_FAIL_ASSERT("worker_do_not_log; Unable to get exported handler");
Expand Down
Loading