Create alert merger which dispatches alerts from Dynamo to alert processors

[austinbyers]

to: @ryandeivert
cc: @airbnb/streamalert-maintainers
size: large

Background

This new Lambda function (the Alert Merger) will, as its name implies, eventually be responsible for alert merging. For now, it just dispatches alerts from the alerts Dynamo table and sends them to the alert processor.

The lifecycle of an alert is now as follows:

1. The rule processor generates alerts and saves them in batches to Dynamo
2. The alert merger runs every minute, finding all alerts in Dynamo which have not yet been sent or which need to be retried
3. The alert merger invokes the alert processor once for each alert, providing the Dynamo record as the payload
4. The alert processor deletes the Dynamo record once it is sent successfully, or else updates the list of outputs which need to be retried

Note that this PR does not implement alert merging, it just sets up the last piece of infra required to make it happen

Changes

• Rule processor no longer invokes alert processor
• Add a new Alert Merger Lambda function, which runs every minute
• Update the alert processor with the new invocation format
• Support metric filters in the tf_lambda module
• Add a new AlertAttempts custom metric emitted by the Alert MErger

Testing

• 100% coverage for the alert merger main
• Deploy to a test account and verify alerts end-to-end
• Deploy just the alert merger: ./manage.py lambda deploy -p alert_merger
• Rollback just the alert merger: ./manage.py lambda rollback -p alert_merger
• Try an alert which can't be sent and verify it is retried
• Enable and disable custom metrics

[coveralls]

Coverage decreased (-0.2%) to 95.554% when pulling a53994f on austin-merger into e6bd483 on master.

[ryandeivert]

😮 this is so good!!!! no real requests from me :) well done!

[ryandeivert]

I thought the global keyword was supposed to avoided whenever possible. We could favor class properties (not instance properties) that would keep the client active for the life of the container. I thought we were doing this elsewhere much I'm having trouble finding it.. but basically this:

class Test(object):
    CLS_PROP = None

    def __init__(self):
        if not Test.CLS_PROP:
            Test.CLS_PROP = "now created"

    def get_prop(self):
        return self.CLS_PROP

for i in range(2):
    # when i == 1 the object is already cached
    print 'Class prop before init #%d: %s' % (i+1, Test.CLS_PROP)
    tester = Test()
    print 'Class prop after init #%d: %s' % (i+1, Test.CLS_PROP)
    print 'Instance prop after init #%d: %s' % (i+1, tester.get_prop())

I like this because it still encapsulates the client within the class that actually uses it.

EDIT: Pardon my naivety - I was thinking this was a client wrapper but quickly realized it is not.

[austinbyers]

No, that's a good point. After some discussion, I decided to go with your suggestion and cache the instantiation in the class itself. It makes the handler itself super clean:

def handler(event, context):
    return AlertProcessor.get_instance(context.invoked_function_arn).run(event)

[ryandeivert]

Can you add some context as to why this particular error is okay and only logs, while others will be raised?

[austinbyers]

It's possible (though very unlikely) that the alert processor could delete the entry from the table before the alert merger updates it with its additional state. If that happens, that's totally fine - we want the rows to be deleted when we don't need them anymore.

I removed the logging entirely - it shouldn't even be a warning, it's just an unusual order of operations. I also clarified with some more comments

rebase + new metric == substantial change

[austinbyers]

@ryandeivert Thanks for the feedback, I've addressed it! I also rebased with your recent change (#643 ) and added support for custom metrics in the alert merger. I tested again end-to-end with a new deploy, but I felt these were substantial enough code changes to require another review when you get a chance!

[ryandeivert]

thanks for these changes! 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑 🐑

[ryandeivert]

this is excellent! :) thanks for dealing with my requests!

[ryandeivert]

🎉