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

Implements the web platform standard reportError API #1979

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 6, 2024

src/workerd/io/worker.c++ Outdated Show resolved Hide resolved
@jasnell jasnell requested review from dom96 and hoodmane April 6, 2024 14:11
@jasnell jasnell marked this pull request as ready for review April 6, 2024 14:11
@jasnell jasnell requested review from a team as code owners April 6, 2024 14:11
@irvinebroque
Copy link
Collaborator

If I call reportError — what happens here with Tail Workers? Will the error will show up in the exceptions field or the logs field?

wintercg/proposal-common-minimum-api#41
nodejs/node#38947

@jasnell
Copy link
Member Author

jasnell commented Apr 6, 2024

... what happens here with Tail Workers?

I think that still needs to be determined. The reporting is the same as the existing logUncaughtException(...) but I don't believe that ends up directly in the tail worker's exceptions field. That'll be something we want to determine, I think.

@irvinebroque
Copy link
Collaborator

Yeah we should figure out before merging any change, since would be weird if this ended up showing as logs. @zebp and @maxwellpeterson may know and can point to internal tail worker code

@jasnell jasnell force-pushed the jsnell/reporterror branch 2 times, most recently from e138439 to 4468492 Compare April 8, 2024 18:53
@jasnell
Copy link
Member Author

jasnell commented Apr 8, 2024

@irvinebroque ... ok, the PR has been updated to publish the reported error to the worker tracer as an exception. Unfortunately we can't actually have a test to verify it in workerd since workerd does not initialize the worker tracer at all. Manually verified in an internal test, however, that the reported error does, in fact, show up in the worker tracer as an exception, so I think we're good on that front.

Copy link
Collaborator

@irvinebroque irvinebroque left a comment

Choose a reason for hiding this comment

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

Supportive of landing this

@jasnell jasnell merged commit 2627d4a into main Apr 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants