feat(integrations): console.error is captured as an exception #4034
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…args contain an Error
freekii
changed the title
kamilogorek
suggested changes
Oct 8, 2021
Capture only the first argument for error level logs rather than checking all arguments. Co-authored-by: Kamil Ogórek <[email protected]>
|
Code looks good so +1, but before merging and releasing, we should discuss how it can impact creation of new issues in the event stream. cc @getsentry/team-webplatform |
|
I don't agree, a |
|
Sure, that I agree with. Although it would be useful to quickly add integration and capture all console.errors as the first step to integrate Sentry IMO. We can maybe add this behind a configurable option? |
rhcarvalho
reviewed
Oct 8, 2021
Comment on lines
65
to
70
| } else if (level === 'error') { | ||
| if (args[0] instanceof Error) { | ||
| hub.captureException(args[0]); | ||
| } else { | ||
| hub.captureMessage(message); | ||
| } |
There was a problem hiding this comment.
Code-wise this is the same as
Suggested change
| } else if (level === 'error') { | |
| if (args[0] instanceof Error) { | |
| hub.captureException(args[0]); | |
| } else { | |
| hub.captureMessage(message); | |
| } | |
| } else if (level === 'error' && args[0] instanceof Error) { | |
| hub.captureException(args[0]); |
|
Perhaps we can hear from @freekii what was the motivation behind this change? If we understand the use case better we can discuss if the code change is appropriate or if there's something else we can do. |
|
I don't believe that I am the best person to ask. I have very recently begun contributing to open source projects and found this issue which seemed like a good place to get started! Sorry I am unable to add more clarity. |
|
@freekii, got it. My bad I didn't realize the related issue 😓 |
|
hi guys, is there a plan to release this change? |
Yes, we're planning a release next week. |
mcdurdin
added a commit
to keymanapp/keyman
that referenced
this pull request
Jul 5, 2022
The back story: Currently Keyman for Android reports errors sent to the console via `console.error()` into Sentry but sentry-manager itself does not. This means that Keyman for iPhone and iPad and other users of sentry-manager do not report these errors. Many of these errors are important. What's worse is that Keyman for Android's error reporting here (via the `sendKMWError()` function in Keyman Engine for Android) does not capture stack traces, and so many of the errors we get do not have enough information to resolve them. Furthermore, by having `sendKMWError()` in Keyman for Android, we capture exceptions and other program errors twice -- once on the web side, and once on the Java side -- this adds noise to our error reporting. Sentry also tends to lump many unrelated `sendKMWError()` events together, so tracking resolution to the errors is painful. The fix: This adds a patch to sentry-manager to capture `console.error()` and `console.warning()` events and report them through Sentry's normal error reporting, and disables the `sendKMWError()` report (although we leave the breadcrumb in place for when there are later, related Java errors). There is a Sentry integration called CaptureConsole, but it does not support capturing stack traces until v6.14 (getsentry/sentry-javascript#4034). Updating Sentry to 6.14 or newer is a bigger job (due to ES6 baseline req. etc.) Note that Keyman for iOS currently has some other stubs in place overriding the `console.*` functions. These should be removed for release builds, so that we can use this pattern instead. I will try and tackle this in a follow-up PR (I will build it on my mac so I can test it). A sample error report captured with this mechanism (no sourcemaps here because this is a -local build): https://sentry.io/organizations/keyman/issues/3401287467/events/c40fd2cebb7743cc8dfe72e0dd34bf65/?project=5983524 I am proposing we back-port this to 15.0-stable as we are missing a lot of data in many of our error reports on Android.
mcdurdin
added a commit
to keymanapp/keyman
that referenced
this pull request
Jul 6, 2022
The back story: Currently Keyman for Android reports errors sent to the console via `console.error()` into Sentry but sentry-manager itself does not. This means that Keyman for iPhone and iPad and other users of sentry-manager do not report these errors. Many of these errors are important. What's worse is that Keyman for Android's error reporting here (via the `sendKMWError()` function in Keyman Engine for Android) does not capture stack traces, and so many of the errors we get do not have enough information to resolve them. Furthermore, by having `sendKMWError()` in Keyman for Android, we capture exceptions and other program errors twice -- once on the web side, and once on the Java side -- this adds noise to our error reporting. Sentry also tends to lump many unrelated `sendKMWError()` events together, so tracking resolution to the errors is painful. The fix: This adds a patch to sentry-manager to capture `console.error()` and `console.warning()` events and report them through Sentry's normal error reporting, and disables the `sendKMWError()` report (although we leave the breadcrumb in place for when there are later, related Java errors). There is a Sentry integration called CaptureConsole, but it does not support capturing stack traces until v6.14 (getsentry/sentry-javascript#4034). Updating Sentry to 6.14 or newer is a bigger job (due to ES6 baseline req. etc.) Note that Keyman for iOS currently has some other stubs in place overriding the `console.*` functions. These should be removed for release builds, so that we can use this pattern instead. I will try and tackle this in a follow-up PR (I will build it on my mac so I can test it). A sample error report captured with this mechanism (no sourcemaps here because this is a -local build): https://sentry.io/organizations/keyman/issues/3401287467/events/c40fd2cebb7743cc8dfe72e0dd34bf65/?project=5983524 I am proposing we back-port this to 15.0-stable as we are missing a lot of data in many of our error reports on Android.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4029.
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).