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

fix: Attempt to provide some information for <unlabeled event> #1397

Merged
merged 1 commit into from
Jun 19, 2018

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Jun 15, 2018

This one was very tricky to debug and fix. Some details and reasoning behind the fix.

First, there are 3 ways to trigger this issue, so let's go one by one:

captureException('');
captureMessage('');
throw '';

Before

captureException('')

screen shot 2018-06-15 at 11 55 27

There's no type and no message, as when a string is passed, it'll fallback to captureException. The thing why it's special is that we default to stracktrace: true and there's synthetic stack trace created by new Error('') which should help a user to debug the issue.


captureMessage('')

screen shot 2018-06-15 at 11 59 07

No type, no message, and no stack trace, as we default to false when using captureMessage directly :(


throw ''

screen shot 2018-06-15 at 12 00 27

No type, no message, no stack trace, and parse error on top of that :(

After

captureException('')

No changes.


captureMessage('')

screen shot 2018-06-15 at 12 08 31

We have a stack trace now \o/


throw ''

screen shot 2018-06-15 at 12 09 43

Overriden type. More reasonable message. No processing error and file, line and column of the main call \o/

FAQ

  • Why it only affects ''?
    Because null and undefined coerce to their string representation and are valid labels

  • Why can we not fix it using synthetic stack traces in TraceKit?
    Because by the time throw '' reaches global error handler, you cannot new Error('') it anymore. It has no data that can be fed to the constructor.

  • Are there other ways around if once I find the culprit?
    Yes, you can use

Raven.wrap(function () {
   ...your code...
})

// or 

try {
  ...your code...
} catch (e) {
  // Synthetic stack traces are possible in `try/catch` clauses, even with `throw ''` calls
  const ex = new Error(e);
  Raven.captureException(ex);
}

Ref: #1271

@kamilogorek kamilogorek requested a review from HazAT June 15, 2018 10:15
Copy link
Member

@mitsuhiko mitsuhiko left a comment

Choose a reason for hiding this comment

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

WFM. This is an example of something where we might want to report to the relay a hint going forward that users should not do that :D

@codecov-io
Copy link

Codecov Report

Merging #1397 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1397   +/-   ##
======================================
  Coverage    73.6%   73.6%           
======================================
  Files          31      31           
  Lines         701     701           
  Branches      104     104           
======================================
  Hits          516     516           
  Misses        182     182           
  Partials        3       3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5317a75...c699027. Read the comment docs.

@victorpavlenko
Copy link

@HazAT pls merge

@kamilogorek kamilogorek merged commit aa72795 into master Jun 19, 2018
@kamilogorek kamilogorek deleted the unlabeled-event branch June 19, 2018 10:38
@glebmachine
Copy link

@HazAT please publish

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.

None yet

6 participants