-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
deps(sentry): move from raven to @sentry/node #9325
Conversation
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.
I seem to recall there was something that motivated looking into this, maybe it could be added to the PR description so we can better decide if it's worth continuing? :)
lighthouse-core/runner.js
Outdated
@@ -46,7 +46,8 @@ class Runner { | |||
Sentry.captureBreadcrumb({ | |||
message: 'Run started', | |||
category: 'lifecycle', | |||
data: sentryContext && sentryContext.extra, | |||
// TODO is this really needed? the 'extra' data is already set at the end of sentry.js ... why put in breadcrumb? |
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.
previously you got a few nice UI perks while traversing the history of events but not sure if that's how it works with the new sdk and/or if they've changed things on the server UI
done
I don't even use Sentry, and the old SDKs aren't being deprecated, so this is a low value PR. but we can keep the branch around in case we ever need to update. |
lighthouse-core/lib/sentry.js
Outdated
scope.setExtras(opts.extra); | ||
} | ||
Sentry.captureException(err); | ||
resolve(); // TODO: idk ?? |
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.
todo: i believe this is all synchronous now
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.
I think you're right. We never await Sentry.captureException
either (with the exception of tests)
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.
L85 suggests it's necessary.
// Special case captureException to return a Promise so we don't process.exit too early
Let's just keep it.
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.
Pretty sure it was just because the previous api was a callback one, and relied on some sort of event shenanigans which may not happen before the process closes (and Node will wait for a pending Promise to resolve before closing ... probably?)
changed to just have the function be async
w/o using any promise stuff directly, just in case.
I assume that was true at the time of writing, but now the old SDKs have been deprecated. |
Confirmed errors show up in UI just fine with the appropriate tags/data.
old:
Sentry web UI has some warning about updating to the latest SDK. Seems we could ignore it. But I looked into what an upgrade entails.
Throwing this up as a draft as it started to take a longer than expected. Let me know if I should continue in this endeavor.
Remains:
https://github.com/getsentry/sentry-javascript/blob/master/MIGRATION.md