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

Payload size is limited to the browser URL length limit? #245

Closed
paavohuhtala opened this issue Jun 16, 2017 · 17 comments
Closed

Payload size is limited to the browser URL length limit? #245

paavohuhtala opened this issue Jun 16, 2017 · 17 comments

Comments

@paavohuhtala
Copy link

While working on the client side Bugsnag integration for one of our internal apps, I noticed that error notifications failed to be sent when I included the application state as metadata.

For context, our application stores the UI state in a single object using Redux; serialized to JSON it's about 160 kilobytes. That is quite a lot of data, but gzip reduces it to under 30 kilobytes and we could most likely remove a lot of extraneous data to make it even smaller.

However, since this library sends everything encoded as a query string (#217), it seems that the maximum length for an error notification is under 2000 characters, which isn't enough for anything non-trivial.

It's very likely we have missed dozens if not hundreds of error reports because the stack trace or metadata has been too large. :/

We are going to write a minimalistic JSON-based replacement for this library. In the mean time, it would nice if the readme or the documentation mentioned this fairly low request size limit.

@konrad-garus
Copy link

I hit the same problem, and it's simply a show stopper for us.

@ankurk91
Copy link

ankurk91 commented Aug 17, 2017

Setting the handler to xhr should do the trick.

Bugsnag.notifyHandler = "xhr" ;

https://docs.bugsnag.com/platforms/browsers/configuration-options/

P.S. It does not solve issue

@bengourley
Copy link
Contributor

Hey folks, sorry to have left you hanging for so long on this one!

As @ankurk91 suggests, setting the notifyHandler to "xhr" will use a HTTP POST delivery mechanism which allows much larger payloads.

The default "GET" method is to support some old IE browsers (the specifics of which escape me at this moment in time), so just be aware that there may be some impact there depending on the browser usage of your users.

@konrad-garus
Copy link

Looking at the source, it still seems to be using GET:

if (notifyHandler === "xhr") {
  var xhr = new XMLHttpRequest();
  xhr.open("GET", url, true);
  xhr.send();
}

I'm still getting HTTP 413.

CORS does not seem to be quite right on this resource either:

No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://acme.com:2990' is therefore not allowed access. The response had HTTP status code 413.

@paavohuhtala
Copy link
Author

I agree with @konrad-garus. "xhr" doesn't fix anything. It still uses ´GET` and sends the payload as an URL - it suffers from exactly the same issues as the legacy method.

@bengourley
Copy link
Contributor

bengourley commented Aug 18, 2017

Argh, sorry I definitely jumped the gun with recommending that solution as you are both correct, it doesn't help with your problem 😞 .

For a bit of context, I've just started here at Bugsnag to look after the JS platforms. This means the JS notifiers (this repo, and the node one) will be receiving more attention from now on. Please excuse me while I hit the ground running and get up to speed on the current state of things.

In terms of a solution that will help you right now I can't offer any suggestion other than not including large data structures in metaData, but have you found the breadcrumbs feature to be useful at all? Could you add a piece of Redux middleware that drops a breadcrumb on each Redux state mutation with the action/data? I know it won't get you the entire state data structure at the time of the error, but you would see the state mutations that led to the reported error.

Longer term, we are looking at revamping this notifier entirely, and will be offering more robust delivery mechanisms. The specifics of which are still to be fleshed out, but the feedback provided here is definitely shaping requirements and the future of the project, so thank you for taking the time write up the issues you've been facing 🙏.

P.S. I've added an internal ticket to get this added 👇

In the mean time, it would nice if the readme or the documentation mentioned this fairly low request size limit.

@bengourley
Copy link
Contributor

@valeronm
Copy link

The worst thing here that 413 error may appear even when metadata parameter is empty. Stacktrace could also easily exceed the limit when script path is a long string and stacktrace is moderately deep.

I think that the library should allow skipping stacktrace for handled exceptions.

@marbemac
Copy link

marbemac commented Nov 12, 2017

@bengourley any news on this one? Really enjoy Bugsnag, but we're about to look for another solution because we can't realistically use this if errors randomly don't get sent (for example, when stack trace is anything but very short). We have removed the metadata, but stack traces push the request over the character limit.

@bengourley
Copy link
Contributor

Hi @marbemac, we are working on a full rewrite of the JS notifier which is nearing completion. It uses a cross-domain POST so this problem completely goes away.

It should be ready for early/beta testing in the next few weeks. If you're up for trying it (as for anybody on this thread) let me know! Otherwise it won't be much longer than that (provided there are no major issues to resolve) that it will be fully released.

Thanks for your patience!

@marbemac
Copy link

Exactly what we were hoping for, looking forward to it!

@marbemac
Copy link

@bengourley let us know when its ready and we'll try it out on our staging environment.

@bengourley
Copy link
Contributor

Hey @marbemac (and anybody else on this thread who's interested), you can now try out the v4 beta!

Intro/instructions are here: https://docs.google.com/document/d/1HBBLyVv9FHsNmbkRLx58WbtisMRVHwDoR3pnxac9JQQ/

Please let us know how you get on.

@bengourley
Copy link
Contributor

🚢 v4 is now published. Please upgrade!

See UPGRADING.md and full docs for more info 😄

@thomaslangston-mongodb
Copy link

I assume that means the new max payload size is 1 MB, since that's the default for ngnix, which I think you're using.

@bengourley
Copy link
Contributor

We're not using nginx, but yeah it's 1MB

@thomaslangston-mongodb
Copy link

thomaslangston-mongodb commented Dec 18, 2018 via email

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

7 participants