-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Comments
+1, I also have lots of these “minified” exceptions in production. Stack traces help, but they're not always captured reliably. |
We've also hit this problem, however our stacktraces give no clue as to what's happened. 👍 for meaningful error messages in production. |
+1, wondering if there have been any updates here |
+1 👍 |
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. |
For 0.12 I hacked a solution where I replaced some of the 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 |
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. |
@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. |
@spicyj Maybe we'd want to use Maybe |
@syranide Yeah, I do want to do this. It's probably the best solution here. |
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. |
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. 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: 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 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:
|
Yes, this is #2461 which is a large project but obviously important. |
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. |
As a summary we should try to solve something for this while also optimizing for constraints:
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 |
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. |
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. |
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. |
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 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. |
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. |
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. |
@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. |
@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. |
@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 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 |
|
@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.
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 |
@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. |
@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 Another thing that could help catch race conditions with a dev build is babel-plugin-transform-react-remove-prop-types (I assume that @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. |
@keyanzhang started working on an error code system in #6874. |
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
I'm not including in my debug build:
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. |
@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). |
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. |
Well done - eagerly awaiting 15.2.0 so we can diagnose and squash a few heisenbugs. |
This is out in React 15.2.0. |
@gaearon is the error mapping available somewhere? And what's the policy for future changes to it. |
@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[]=. |
@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. |
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. |
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) |
@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. |
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. |
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. |
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. |
@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. |
Yes I'm aware. However that doesn't help us much since almost all our code On Sun, Jul 3, 2016, 3:18 PM Dan Abramov notifications@github.com wrote:
|
@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? |
Oh thanks @mathieumg ! I'm totally blind 😅
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 ^^ |
@rafayepes I thought #2686 (comment) would have answered that concern! 😛 |
@rafayepes The error list is automatically maintained via a Gulp task, so forgetting to do it isn't a concern. See bfd1531. |
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. |
@benvinegar |
if (process.env.NODE_ENV === 'production') { 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? |
It's much slower. |
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. |
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 |
VDOM Stacktraces in errors like Error message mappings are useless usually. |
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!
The text was updated successfully, but these errors were encountered: