Skip to content

poc(sentry): send events from a separate thread #16

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

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

Conversation

JonasBa
Copy link
Member

@JonasBa JonasBa commented Oct 21, 2022

This is a poc of sending events to sentry via worker thread. The hypothesis is that currently, jest cannot move to the next test file while a promise is pending, offloading that to a separate thread would allow it to keep executing test files and enqueuing network requests on a separate thread.

Before continuing with the idea, I would like to investigate in optimizing some transport params first.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

This is dope. If we find that this has a noticeable impact, we should probably upstream these changes into the Node SDK so it becomes the default.

return () => {
return {
send: (envelope) => {
worker.postMessage(envelope);
Copy link
Member

Choose a reason for hiding this comment

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

My only concern here is that there might be some back-pressure issues here in high volume scenarios, but that should be no problem with the volume we have with jest

let transport = makeNodeTransport({});

parentPort.on("message", (message) => {
transport.send(envelope);
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to make envelope -> message here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yes, we def should.

@JonasBa
Copy link
Member Author

JonasBa commented Oct 24, 2022

@AbhiPrasad I actually have a blocking problem here - the SDK's makeNodeTransport factory functions expects an object with url, but since this is initialized on a different thread, I dont have access to that value meaning I'd need to pass the DSN through an initializer step and lazy initialize it... I'm tempted to just write a fully custom node transport, but I then risk falling out of sync with how the nodejs transport might evolve in the future.

Comment on lines +2 to +3
return () => {
return {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move constructing the worker here, then we can pass down the URL as a argv to the Worker constructor. That should make it easy to pass down the url right? (since the function returning should get url as an option).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants