Skip to content

Streamline integration of SQS Batch Processing with Sentry #293

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

Closed
charlax opened this issue Feb 18, 2021 · 6 comments
Closed

Streamline integration of SQS Batch Processing with Sentry #293

charlax opened this issue Feb 18, 2021 · 6 comments
Assignees
Labels
feature-request feature request

Comments

@charlax
Copy link

charlax commented Feb 18, 2021

Is your feature request related to a problem? Please describe.

The current behavior reraises exception as SQSBatchProcessingError, which hides the original exception and prevents it from showing up correctly in Sentry:

SQSBatchProcessingError
Not all records processed succesfully. 1 individual errors logged separately below.

Traceback (most recent call last):
  File "/var/task/urllib3/connectionpool.py", line 445, in _make_request
    six.raise_from(e, None)
  File "<string>", line 3, in raise_from
  File "/var/task/urllib3/connectionpool.py", line 440, in _make_request
    httplib_response = conn.getresponse()
  File "/var/task/sentry_sdk/integrations/stdlib.py", line 102, in getresponse
    rv = real_getresponse(self, *args, **kwargs)
  Fi...

Describe the solution you'd like

The best solution would be to be compatible by default with Sentry if it's there (I presume this is one of the most used exception reporting solutions for Python).

Another solution would be to provide a hook into the exception handler, to give us the opportunity to call Sentry.

Describe alternatives you've considered

I looked into subclassing PartialSQSProcessor and overriding _process_record, which is what I might do if you don't think that's worth looking into.

@charlax charlax added feature-request feature request triage Pending triage from maintainers labels Feb 18, 2021
@charlax
Copy link
Author

charlax commented Feb 19, 2021

I was actually pretty simple, although it would still be nice to have a no-code integration:

from aws_lambda_powertools.utilities.batch import PartialSQSProcessor
from sentry_sdk import capture_exception


class SQSProcessor(PartialSQSProcessor):
    def failure_handler(self, record: Event, exception: Tuple) -> Tuple:  # type: ignore
        capture_exception()
        logger.exception("got exception while processing SQS message")
        return super().failure_handler(record, exception)  # type: ignore

Btw: I took the existing types, Tuple misses its type parameters.

@to-mc
Copy link
Contributor

to-mc commented Feb 19, 2021

Hi @charlax, thanks for raising an issue! The functionality surrounding the SQSBatchProcessingError is a bit tricky for 2 reasons:

  1. We have to execute _process_record multiple times, and multiple exceptions can be raised during a single execution. There's no good way (I'm aware of) to raise from multiple exceptions, so we wrap everything into the SQSBatchProcessingError. The "root" exceptions should be logged though since we fixed Exception handling in batch processing doesn't work as expected #275 in v1.10.2.
  2. By default, logging statements from within the powertools package are suppressed - as per best practices for libraries. This means we can't log the exceptions individually as they happen without users needing to manually set the package logger.

I think the solution you've found is a good one, arguably cleaner than passing hooks/callbacks around. We could always add something to the docs for folks wanting to implement similar logic, do you think that would help?

@to-mc to-mc removed the triage Pending triage from maintainers label Feb 19, 2021
@to-mc to-mc self-assigned this Feb 19, 2021
@charlax
Copy link
Author

charlax commented Feb 19, 2021

Yes, this was actually very simple with the failure_handler, adding this example to the doc would be enough I think! Thanks for your answer!

@heitorlessa
Copy link
Contributor

Hey @charlax Added an example - Could you tell us whether this suffice?

image

We're also refreshing the navigation it should be easier to find it - Full PR: #308

@charlax
Copy link
Author

charlax commented Mar 4, 2021

Yes, this is great! Thanks a lot!

@heitorlessa heitorlessa added the pending-release Fix or implementation already in dev waiting to be released label Mar 4, 2021
@heitorlessa
Copy link
Contributor

This is now available in 1.11.0 - Literally just launched ;) 🎉

@heitorlessa heitorlessa removed the pending-release Fix or implementation already in dev waiting to be released label Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request feature request
Projects
Development

No branches or pull requests

3 participants