log errors regardless of ignoreSentryErrors

[mihasya]

If we don't even log anything when ignoreSentryErrors is enabled, it is impossible to tell when there's something wrong with the integration. For example, when transaction payloads get too big and we start getting 413s from Sentry's service, the options are a. cause 500s in the service that's being monitored (worst possible outcome) or b. have zero way to tell it's happening. This introduces c. make it possible to add a warning alarm on occurences of sentry errors in the logs.

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

[mihasya]

Edit: was able to run yarn fix after reading the contributing doc 👍

[lforst]

Hi, thanks for opening this. I brought this back to the team - it's not ok that we interfere with return values (or crash any lambdas for that matter).

I am currently evaluating if we should just catch all errors that happen while flushing and log them out. Will get back to you very soon!

[lforst]

Suggested change

	logger.error(e);
	IS_DEBUG_BUILD && logger.error(e);

[lforst]

Hi, we recently merged #5091. With that change, all errors while flushing are caught by default - no more crashing lambdas. Additionally, it logs those errors. I believe this makes this PR redundant, however, feel free to double check me on that one.

Even though this probably won't get merged, thank you very much for the contribution - it got the ball rolling.

Please check this thread for when this will reach our released lamda layers.

[mihasya]

Due to the use of IS_DEBUG_BUILD flag in the PR you linked, this would not address the core issue this PR is targetting. Note that even passing debug: true to Sentry.AWSLambda.init would not cause these logs to appear. So in order to figure out that, say, a payload has gotten too large to be reported, we would need to re-deploy the whole app with a debug build. I personally am unclear on how to even do that and was unable to find it in a cursory search just now. Either way, it seems like way too much work just to find out that Sentry is returning 400s.

I'm also not sure how the PR you linked would affect the crashing behavior. It seems to just log something if the conditional is true, but does not return early or handle the error in any other special way. As far as I can tell, the error would just keep bubbling up and trigger an exception. So in order to avoid an exception, we'd still have to run ignoreSentryErrors: true, and the reliance on the IS_DEBUG_BUILD flag would prevent the error from showing up. So once again, we would be living with option b. from this PR's description: "b. have zero way to tell it's happening."

Are there arguments against always logging an error when an error occurs while communicating with the Sentry service? I can't think of any, but perhaps one of the following options would allow for sufficient flexibility to satisfy any concerns?

• adding an additional flag that specifically enables logging errors while communicating with the service
• changing this PR to make it so that debug: true enables the logging in addition to IS_DEBUG_BUILD

[lforst]

Sorry, linked the wrong PR. #5090 is the one that changes error behaviour.

IS_DEBUG_BUILD is a tiny bit misleading. Unless you're explicitly instructing your bundler/preprocessor to replace __SENTRY_DEBUG__ instances with false, IS_DEBUG_BUILD will simply evaluate to true (see docs or code for more info). So unless you're doing that, you should already be deploying debug builds and therefore be able to use debug: true to log errors.

#5090 provides an error callback for the flush function that catches (and no-ops) all errors that flush might produce. Up until now they just bubbled up, crashing lambdas, which is absolutely not what we want to happen. We also added tests to explicitly test for this behaviour.

Feel free to try our patch when it is released and ping us if something doesn't work as expected!

[github-actions]

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀