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

Fixed recursion on payload too large #492

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

woutersamaey
Copy link

Goal

Fixed the endless recursion when the payload to submit is too large. The PHP max function nesting will be reached.

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@Cawllec
Copy link
Contributor

Cawllec commented Oct 10, 2018

Hi @woutersamaey, it seems this would interfere with our retrying logic that would generally trigger to split up large payloads. While I understand having a singular payload event that's too large could be problematic, and will recommend a fix for that to our products team, I don't think this approach will be usable.

If you could describe the particular issues that led you to create this PR, that would help us narrow down and approach and create a fix much more easily.

@woutersamaey
Copy link
Author

We had an exception with a huge message that could not be sent to Bugsnag.
This made debugging the issue on production very hard.

The idea is that if bugsnag can't take the exception, then at least it should help me out instead of failing with another error, masking the original.

The underlying issue was easy to fix for me, but because bugsnag could not handle it, it became a huge thing. So that's why I wrote the message out.

@Cawllec
Copy link
Contributor

Cawllec commented Oct 10, 2018

I agree, that doesn't sounds ideal at all. I think adding a failure condition with full logs would be a practical way to approach this as suggested, and I'll suggest we prioritise this to get a fix as soon as we can.

@woutersamaey
Copy link
Author

@Cawllec What about creating a fallback LoggerInterface where we can attach our own logic to, so that will be used in case Bugsnag can't handle it?

I'd be able to implement per-application logic and log the error to the regular application error log, which did not happen before as it could not get to that point.

@GrahamCampbell
Copy link
Contributor

What about creating a fallback LoggerInterface where we can attach our own logic to, so that will be used in case Bugsnag can't handle it?

For now, I think it's best to do what the library already does, which is to fallback to php's own global logging functions. Changing how the library deals with internal exceptions is a separate discussion to be had I think, if we want to change that. One of the reasons for the current design is to prevent the possibility non-termination due to bugsnag being the psr logger itself.

@GrahamCampbell
Copy link
Contributor

For now, I think it's best to do what the library already does

Ah, looking at your changes, I see that's what you've done. Perfect! :)

@woutersamaey
Copy link
Author

Huh?

@abigailbramble abigailbramble added backlog We hope to fix this feature/bug in the future bug Confirmed bug labels Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog We hope to fix this feature/bug in the future bug Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants