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

Error reporting in production #2686

Closed
matthewwithanm opened this issue Dec 9, 2014 · 71 comments
Closed

Error reporting in production #2686

matthewwithanm opened this issue Dec 9, 2014 · 71 comments

Comments

@matthewwithanm
Copy link
Contributor

We've been using Sentry to log errors in our server-side rendering and client-side. Unfortunately, the error message that React throws in production mode is pretty useless, so we wind up with a lot of unactionable reporting.

React has two levels of errors—warnings (those logged to the console) and errors (the ones that are thrown)—but both are enabled/disabled using the same env var/compile option. It would be really great if there were two different options so that I could reap the benefits of disabling warnings (e.g. from runtime type checking) without losing meaningful error messages (for example, from checksum violations).

Thanks!

@gaearon
Copy link
Collaborator

gaearon commented Dec 15, 2014

+1, I also have lots of these “minified” exceptions in production. Stack traces help, but they're not always captured reliably.

@PrototypeAlex
Copy link

We've also hit this problem, however our stacktraces give no clue as to what's happened.

👍 for meaningful error messages in production.

@why-jay
Copy link

why-jay commented Aug 2, 2015

+1, wondering if there have been any updates here

@mathieumg
Copy link
Contributor

+1 👍

@Rob-ot
Copy link

Rob-ot commented Sep 2, 2015

I don't know how feasible this is but it'd be nice to be able to have full error messages in production without performance measurements and other things that slow down the app.

@Rob-ot
Copy link

Rob-ot commented Nov 30, 2015

For 0.12 I hacked a solution where I replaced some of the if ("production" !== process.env.NODE_ENV) { checks with my own environment variable. It worked pretty well.

If I pullrequest a similar approach to 0.15 would that be the right direction for it to be accepted?

I'm thinking I'd replace some (not all) of the checks with something like

if ("production" !== process.env.NODE_ENV && !process.env.REACT_PRODUCTION_DEBUGGING) {

so you could set REACT_PRODUCTION_DEBUGGING and NODE_ENV=production to do a production build with error reporting enabled. This will still allow minifiers to optimize out the unused code. Is that best way to approach this?

@sophiebits
Copy link
Collaborator

I would sooner have a REACT_ENV or something that takes precedence over NODE_ENV, but changing how this works will cause a lot of pain since everyone would need to update their build processes so I don't want to do it lightly.

@syranide
Copy link
Contributor

syranide commented Dec 1, 2015

@spicyj On another note, could it not make sense to have IDs associated with errors and keep those in the minified builds along with outputting the additional arguments provided, that way you can always match up the message and values later without significantly impacting bundle size.

@Rob-ot
Copy link

Rob-ot commented Dec 1, 2015

@spicyj Maybe we'd want to use REACT_ENV if it exists and fallback to NODE_ENV for backwards compat? Then REACT_ENV would be one of 3 options: production, production-with-debugging, or anything else for dev mode?

Maybe REACT_ENV is a bad name since it's more of a build mode than an environment. The important bit is in production-with-debugging mode it has usable error messages but none of the slow type checking and assertions. You get the idea though.

@sophiebits
Copy link
Collaborator

@syranide Yeah, I do want to do this. It's probably the best solution here.

@PrototypeAlex
Copy link

I agree with @Rob-ot about the fallback for backwards compatibility.

Though I'm not a fan of flipping feature switches based of broad ENV types, I'd rather see it as an isolated ENV variable with a meaningful name, you suggested REACT_PRODUCTION_DEBUGGING for example, as apposed to overloading REACT_ENV.

But that's just me 😄, I'm actually happy with anything that will enable us to switch on debugging.

@slorber
Copy link
Contributor

slorber commented Dec 11, 2015

Maybe we need a different env variable for each feature instead of having an env variable then: we should allow fine grained control over what is enabled/disabled.
The NODE_ENV could be used to compute a meaningful default for all features, and be compatible with existing build systems.

Like:

var isProduction = process.NODE_ENV === "production";

var perfMeasuresDefault = !isProduction;

var isPerfMeasures = process.REACTJS_PERF_MEASURES ? process.REACTJS_PERF_MEASURES === "true" : perfMeasuresDefault

It could be nice to have a little guide on how each of these features could impact performance so that we can decide if we want them or not.

However I think having meaningful errors in production should be a default setting. I don't understand who would be interested to have little perf/bundle size improvements over useless error messages.

I constantly got useless bug reports like this one:

comment bug

The user says he can't delete his own comment (but actually it can come from anywhere because there was probably an error thrown in render())

The user is kind enough to give a screenshot of the console error, however the error is useless and I have no way to make it more useful.
I also use GetSentry and once React starts to throw an error in a render method is generates a lot of errors afterward and make a lot of error reportings in GetSentry.

I guess it is related to catching errors in render() too, and some existing issues like #5528

In a general way it would be nice to have some doc on how to run React for production (I mean, more than just setting the NODE_ENV flag). I'm talking about topics like:

  • Ease React production bug reporting
  • Don't make the UI totally unusable after some errors are thrown
  • Don't generate 10000 useless errors if the UI is in a really bad state, and eventually give opportunity to ask the user to refresh the page or something

@sophiebits
Copy link
Collaborator

I also use GetSentry and once React starts to throw an error in a render method is generates a lot of errors afterward and make a lot of error reportings in GetSentry.

I guess it is related to catching errors in render() too, and some existing issues like #5528

Yes, this is #2461 which is a large project but obviously important.

@dcramer
Copy link

dcramer commented Apr 12, 2016

fwiw I can speak for many other Sentry users who this is also affecting, including the Sentry team itself who uses React. Let us know what we can do and we're happy to help make this happen.

@sebmarkbage
Copy link
Collaborator

As a summary we should try to solve something for this while also optimizing for constraints:

  1. React is big to download/parse!
  2. The React ecosystem has too many options/configurations to learn!
  3. React is too slow!
  4. Error messages are too short and cryptic!
  5. Different tools doesn't work with different combinations of global configurations/compliers.

Solving one issue at a time doesn't work for constraint solving.  

Anyway. This came up recently again. I think it was in a React Native issue. The solution I suggested there was that we'd compile to error codes and put them in a lookup somehow. Like @syranide and @spicyj suggested above.

This is what we do for stack traces. Minified and compiled code often doesn't have legible stack traces or even the stack frames at all for the debugging. Source maps reinsert them so that's what we should do as well.

This seems like a much more general problem though. This is something the ecosystem needs to address, just like what to do about console.error calls from our warnings. I don't think the React repo itself should be on the hook for solving general JS problems. Maybe React should just spit out throw new Error('...') and stripping the error message + generating a source map + error code lookup file is left up to your minifier/compiler. Someone should build this solution for Babel and then we'll adopt it.

@dcramer
Copy link

dcramer commented Apr 13, 2016

What I'd like to see is JavaScript adopt standard exceptions. Is there an actual concern with generating consistent error messages? I don't think solutions like what Angular does (embedding URLs and trying to add debug info) are needed, but rather we need to treat JavaScript like every other programming language. 
Tools like Sentry can be responsible for parsing and making sense of errors at a higher level, but they can only do that if the libraries and frameworks don't obscure details. 
I don't know the internals of React, but what is the concern with just ensuring that the errors, at the very least, throw standard exceptions?

@sebmarkbage
Copy link
Collaborator

Note that preserving error messages is not a very standard feature of programming languages in general. Many native application just provide very small and coarse grained symbols.

Systems like C# or Java that preserve them don't have to deal with the problem of download size and memory usage.

Error messages at scale add a lot of bytes to an application and stripping them out is a significant win. You could imagine a compromise with shorter messages, but if you need to look up what it means, you might as well just go all the way and add an error code.

Right now the relative size is pretty small but we have plans on focusing on minimizing the byte size of the React package because it has been of some concern to people. At that point, these messages will represent a significantly larger relative size that it will start making an impact. When you expand the problem to Relay and React Native it becomes more noticable.

Of course, we could put this burden on the end user of our library. E.g. if you don't have a compiler for your website, then you don't get it and your users suffer for it. That doesn't help for CDN usage and we're also not being very good citizens of the community. Many sites don't have very advanced polyfill and compiler solutions. To some degree, we cater to a lower common denominator so we want to provide a decent solution for existing tooling.

Similarly, we do include an Object.assign polyfill by default even though we could just require you to have a polyfill. There isn't really good standard solution for global polyfills in Node for example. So we just do it ourselves so that most people get a decent production set up by default.

I'd be happy to have our transform spit out some unique error code for each message that goes into a json file for reference though. That way you could restore the error messages.

Note that our thrown errors are not super helpful anyway. Less useful than the warnings. I'd be curious to see what kind of errors you get and if it is much more helpful.

@sebmarkbage
Copy link
Collaborator

I think the bigger issue is that we intentionally remove a lot of our thrown errors all together because the test adds execution time to hot paths.

Since we have so few errors left, maybe it doesn't matter anymore, but it also doesn't solve the issue.

What you really want is the ability to add fine grained error tracking for a subset of users that include some of the warnings. Our new devtools plugin system will hopefully let you do that at least for "profiling" builds of React.

@mitsuhiko
Copy link

@sebmarkbage

Note that preserving error messages is not a very standard feature of programming languages in general. Many native application just provide very small and coarse grained symbols.

This typically only affects symbols and not actual error messages. The only thing that gets removed in production builds are typically asserts that go away. Error messages are retained as often they show up in the console or elsewhere. However for the rare case that an error does get removed, often the function the error is raised from is named in such a way that it becomes obvious from the stacktrace what happened. As an example on iOS core foundation will execute code from __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ which shows up pretty clear in stacktraces.

JavaScript in theory has sourcemaps however they only map locations, not symbol names so that would mean if that function name gets minimized it's not really helping.

@sebmarkbage
Copy link
Collaborator

That's generally just for dynamically linked, right? For statically linked libraries like is common on iOS, you optimize those symbols unless you have the debug symbols locally which is similar to a source map.

Source maps does include references to source code and the ability to expand a stack trace to more frames, e.g. when a function was inlined. So you can reproduce the stack trace. React Native's error messages does this by default for example.

Btw, we don't do anything to change execution flow between modes. We've had a lot of production bugs due to such strategies in the past (both JS and native). An example is that you do a network request or mutation right after an assertion. If it gets aborted in development mode it might look like a small bug. You'll never know how bad that crash will be when it goes into production when the network request or mutation continues along. We've had pretty bad bugs due to this so we've made it very intentional to never change execution flow.

This means that we don't strip out assertions that would crash or throw in development. Similarly, because we want to be able to strip out the more expensive assertions, we turn them into logs instead of throws.

@mitsuhiko
Copy link

That's generally just for dynamically linked, right? For statically linked libraries like is common on iOS, you optimize those symbols unless you have the debug symbols locally which is similar to a source map.

I think we should make a distinction between symbols and error messages here. Symbols are a solved problem in both native and javascript with debug symbols/pdb files and sourcemaps respectively.

If hypothetically such optimized exceptions would carry an identifier that can be used to look up some useful information that would already go ways. This i what HRESULT on windows does in many ways. It's like an errno on steroids that can be resolved to quite appropriate error messages without creating binary bloat.

@jedwards1211
Copy link
Contributor

@sebmarkbage I completely understand and agree with you that unminified errors in a real live production app are a bad idea. However, they would be a big help when testing/debugging the production build in a sandbox, because sometimes errors occur in production that don't occur in a development build. So I think an option to have unminified errors in a production build, with documentation that explains all the drawbacks of using it for anything other than testing/debugging, would be the biggest win for developers.

@jimfb
Copy link
Contributor

jimfb commented Apr 14, 2016

@jedwards1211 do you have an example of an error (non-contrived) that only occurred in production? While it is conceivable that such a thing could happen, it should be extraordinary uncommon in practice. If it does happen, we'd like to know asap.

@jedwards1211
Copy link
Contributor

@jimfb it definitely wasn't an inconsistency between the development and production builds of React; it was a race condition in my own code. The difference in speed and load time of the development and production builds of course can affect race conditions.

Unfortunately I don't remember the exact details, but I know it was because I forgot to wrap my ReactDOM.render call in Meteor.startup(() => {...}) (I'm using Meteor). Somehow in dev mode it managed to always run after Meteor was ready anyway, but in production it ran too soon, and tried to access some Meteor feature that wasn't yet loaded, throwing an exception. I don't remember exactly how the error got caught and minified by React though.

So unless it should be impossible for userland race conditions to throw errors that would get caught and minified by React (and I would think it's always possible, for instance a component's render accidentally returns undefined because the programmer mistakenly assumed certain async-loaded data would be present by the time it renders, but it's not), the possibility of minified errors only in production exists.

@jedwards1211
Copy link
Contributor

render returning undefined is not the best example, since programmers should guarantee it returns an element, it was just the only way to generate a React error I could think of off the top of my head. I'm sure there are other ways.

@jimfb
Copy link
Contributor

jimfb commented Apr 14, 2016

@jedwards1211 The tricky thing about race conditions is that, in a build with better error messages (and potentially some warnings), you're still going to have a performance impact. The main differences in performance are a result of us eliminating all the debugging information and logic. If you add that info back into the production build, the performance characteristics will likely resemble those of the dev build.

to throw errors that would get caught and minified by React

React won't catch/minify userland code/traces. If your userland code is minified, that's some other tool in your tool chain. If you just want an unminified production build of React, you can build dev with NODE_ENV=production, which would get you what you want (an unminified production build).

@sebmarkbage
Copy link
Collaborator

@jedwards1211 I think the example you mention requires a middle ground build. Something that have most of the expensive checks removed so that the race condition gets hit in time. But still with nice debug messages.

The problem with this is that these different options create an explosion of different options. You could conceive of a configuration which would catch this scenario but not another so which configuration do you use? Then we have an explosion of best practices that all depend on whether someone hit a particular scenario involved. I think it configuration like this needs to have a very high bar.

Luckily there has been talk of a PROFILE configuration. A build that is set up to run at near full speed while retaining a lot more debug information for profiling purposes. It is designed to be used on development machines. Perhaps your scenario would've been satisfied by that option as well.

@jedwards1211
Copy link
Contributor

@jimfb you know, now that I think about it, I probably was dealing with an error minified by my own toolchain. It was over a year ago...I should really keep a log of tricky issues I've dealt with. The marginal possibility of userland code performing invalid React operations as a result of a race condition still exists, but it does seem way less likely than userland code throwing TypeErrors.

Another thing that could help catch race conditions with a dev build is babel-plugin-transform-react-remove-prop-types (I assume that propTypes checks have the biggest performance penalty of any dev-specific feature?)

@sebmarkbage in any case the profile configuration sounds good to me, and if it failed to catch a scenario like this, I don't think I would be clamoring for more options, because at that point it would be hard to predict what would even have an effect on the race condition.

@gaearon
Copy link
Collaborator

gaearon commented May 25, 2016

@keyanzhang started working on an error code system in #6874.
Check it out and let us know what you think about the plan!

@dgoldstein0
Copy link

just to chime in here... I'm super interested to see what the outcome of this will be. Unfortunately, though, the codebase I work on is still on the ancient react .12 (we are in process of upgrading to .13, but we've avoided jsx in the past which has made this upgrade extra difficult), so for our purposes I've decided to fork and implement something related on top of .12.0. I realize I'll probably have to port this forward until we can catch up to whatever react version this change ships with - which I suspect will be 15 or 16?

Anyhow I started a fork of react 12 for my purposes. You can find it here, if you are interested: https://github.com/dropbox/react/tree/v12-dropbox. Basically my main change is to build a debug version which

  • keeps around error messages from invariants
  • keeps a few key deprecation warnings - legacy factory deprecation and transferPropsTo deprecation. Since when we upgrade, these warnings will turn into breakages.
  • code that appears to be important for keeping around displayNames for debugging

I'm not including in my debug build:

  • prop type checking and object freezing in prod, as the former is well known to be slow, and the latter has comments to that effect
  • avoiding various internal react errors / warnings, which I suspect are there more for developing on react itself.

Hopefully this provides an interesting data point for you guys to work from; we are working hard to catch up with the latest react now, and would definitely prefer not to have to maintain our own fork once we do.

@keyz
Copy link
Contributor

keyz commented Jun 17, 2016

@dgoldstein0 Thanks! The production error code system has been implemented in #6882, #6946, and #6948; it'll be shipped with the 15.2.0 release. I totally understand that upgrading from 0.12 could take some time so it might be possible to move the error system from the current version to your fork (the newly added parts are mostly build-time only).

@dgoldstein0
Copy link

I'll look at it, but I think my fork is probably mostly working already - just need to do some testing. But I'll keep the option in mind.

@STRML
Copy link
Contributor

STRML commented Jun 26, 2016

Well done - eagerly awaiting 15.2.0 so we can diagnose and squash a few heisenbugs.

@gaearon
Copy link
Collaborator

gaearon commented Jul 1, 2016

This is out in React 15.2.0.
Thanks to @keyanzhang!

screen shot 2016-07-01 at 21 00 23

screen shot 2016-07-01 at 21 00 35

@gaearon gaearon closed this as completed Jul 1, 2016
@mitsuhiko
Copy link

@gaearon is the error mapping available somewhere? And what's the policy for future changes to it.

@gaearon
Copy link
Collaborator

gaearon commented Jul 1, 2016

@mitsuhiko It’s automatically extracted at the build time. I think we plan to treat it as append-only. You can find the latest version here. Error messages link you to a page on the website where we decode them, e.g. https://facebook.github.io/react/docs/error-decoder.html?invariant=31&args[]=object%20with%20keys%20%7Bname%7D&args[]=.

@keyz
Copy link
Contributor

keyz commented Jul 1, 2016

@mitsuhiko here is the script that extracts errors. As @gaearon mentioned, it's append-only, which means the code that have been already there won't be changed.

@mitsuhiko
Copy link

mitsuhiko commented Jul 1, 2016

Append only is good. The not so nice thing is that there is no meta information on those exceptions that tools can access. That might be a nice thing to have in the future.

@dgoldstein0
Copy link

question: shouldn't the react version number be in the error urls - at least the major version? (I don't quite follow what "append only" means in the context of this thread)

@keyz
Copy link
Contributor

keyz commented Jul 2, 2016

@dgoldstein0 The JSON file and the website will always be the latest release version. "Append-only" means we won't modify or delete an existing error mapping, and a new error (or an existing error that gets newly edited) will be appended to the end of the file and get a new number. Therefore we make sure that old versions are supported.

@dgoldstein0
Copy link

got it. But don't think that helps me right now - I'm looking at backporting this to react .12 and doubt that the errors are going to come out in the same order.

@sophiebits
Copy link
Collaborator

We don't use the major version in the URLs because the page doesn't need it. We don't have plans to support React 0.12 in the official error decoder page but you could make your own page and error code map if you were very motivated. If you are doing a custom build it may be easier for you to just delete the build code that minifies the error messages.

@dgoldstein0
Copy link

already did that. The problem though is that the error messages add 32kb to react when minified, which was a bit more than I was hoping. So I'm preceding with backporting it.

I completely understand that react .12 is unsupported now; just unfortunate reality that I still have to deal with it. And even when I'm done with .12, I'll probably have .13/.14 to deal with for 2-3 months before we finally can get to 15.

@gaearon
Copy link
Collaborator

gaearon commented Jul 3, 2016

@dgoldstein0 Just checking—are you aware that some of the deprecations / changes can be automated with https://github.com/reactjs/react-codemod? You don’t have to update all of the code by hand.

@dgoldstein0
Copy link

Yes I'm aware. However that doesn't help us much since almost all our code
is coffeescript, and we didn't start using jsx until recently.

On Sun, Jul 3, 2016, 3:18 PM Dan Abramov notifications@github.com wrote:

@dgoldstein0 https://github.com/dgoldstein0 Just checking—are you aware
that some of the deprecations / changes can be automated with
https://github.com/reactjs/react-codemod? You don’t have to update all of
the code by hand.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2686 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABBndxjbkt1KGS6DX5lN2cmIq0PebkUxks5qSDUkgaJpZM4DGLA4
.

@rafayepes
Copy link

@keyanzhang I don't see anything indicates the react version in the error URL https://facebook.github.io/react/docs/error-decoder.html?invariant=1&args[]=Foo&args[]=Bar

How is the versioning of codes.json managed?

@mathieumg
Copy link
Contributor

mathieumg commented Jul 12, 2016

@rafayepes #2686 (comment) and #2686 (comment)

@rafayepes
Copy link

rafayepes commented Jul 12, 2016

Oh thanks @mathieumg ! I'm totally blind 😅

@spicyj what about future versions of React? So let's say React 16 or React 17 adds/deletes/modifies some errors (totally completely normal). A user of React 15 could go to the error URL and find an error message that is not actually the error he/she had. Are you going to make sure that old error indexes are never removed/modified? How?

So looks like the approach is never modify old errors. I'm still curious how can this be ensured and how this can be scaled across multiple developers. Seems something fairly easy to forget.

Thanks! Love this feature ^^

@mathieumg
Copy link
Contributor

@rafayepes I thought #2686 (comment) would have answered that concern! 😛

@joshkel
Copy link

joshkel commented Jul 13, 2016

@rafayepes The error list is automatically maintained via a Gulp task, so forgetting to do it isn't a concern. See bfd1531.

@benvinegar
Copy link

benvinegar commented Aug 10, 2016

As a follow-up to this, we've modified @getsentry to fetch the original message and display it in our crash reporting UI. Pull request is getsentry/sentry#3632, announcement on our blog here.

@can-cc
Copy link

can-cc commented Sep 19, 2017

@benvinegar
I try Sentry but it don't caught any minified error.
it need other configure?

@tushar-borole
Copy link

if (process.env.NODE_ENV === 'production') {
module.exports = require('./cjs/react-dom-server.node.production.min.js');
} else {
module.exports = require('./cjs/react-dom-server.node.development.js');
}

react-dom-server.node.production.min.js suppress all the error, can we use react-dom-server.node.development.js on production and what implication it will have?

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2018

It's much slower.

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2018

react-dom-server.node.production.min.js suppress all the error

It shouldn't "suppress" any error. All errors in production version of React exists in the same way, and you should get an error code in every message so you can decode them back.

@antonsivogrivov
Copy link

And what about exceptions? I have got the following error without any links:

"Minified exception occurred; use the non-minified dev environment for the full error message and additional helpful warnings."

React v15.4.1

@a-x-
Copy link

a-x- commented Jul 15, 2019

VDOM Stacktraces in errors like The above error occurred in the <Foo> component:
are still mess in latest React versions.
How to apply a source maps to them before send them to error-logger tools like Sentry?

Error message mappings are useless usually.
The main info is a VDOM stack trace.

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

No branches or pull requests