search

TOMToolkit / tom_antares

Antares Broker module for the TOM Toolkit
GNU General Public License v3.0
0 stars 5 forks source link

Enhancement: ANTARESBroker uses HTTP API instead of Kafka #7

Closed nicwolf closed 3 years ago

nicwolf commented 3 years ago

Hi folks,

We got a report from someone this morning that was having some issues using ANTARES with the TOMToolkit. Interfacing with Kafka introduces a layer of complexity into the TOMToolkit/ANTARES plugin that I think there isn't a compelling reason to support now that we have a robust HTTP API. In addition to providing a path forwards for more supporting more complex queries in the TOM, the HTTP API also eliminates the requirement for API keys to fetch data from ANTARES.

This PR:

I'd like to get some input from you all about how this looks--there were a couple of things in terms of data structures/types that I was iffy about. Our API also supports fetching sparse responses which could make load times snappier--I wasn't sure what data returned from alert_to_dict was required, for example.

I'm also curious if you could speak to what pagination strategy we should be supporting with queries (if any)? I matched what was happening before this patch, returning the first 20 results from the query.

Looking forward to working with you all on this :smile: Happy new year!

nicwolf commented 3 years ago

Another question somewhat related to pagination: I assume that we want the alerts in the query result sorted from newest to oldest? The opposite is happening right now so I've marked this PR as a draft and will mark as ready once I get some clarity on this and maybe submit another commit to fix.

dmcollom commented 3 years ago

Ah, this is fantastic! When I was getting this module working again, I wanted to move to the HTTP API, but I also just wanted it to work again, as my time was limited--as I'm sure you saw, there are some placeholders for future leveraging of the API.

The pagination strategy is a little odd. For whatever reason, the original implementation was exactly what you've stated--just the first 20. That hasn't caused any major consternation yet, as users are usually filtering down more than that or just care about latest., but the fix is on the backlog. That should answer the ordering question, though--newest to oldest is preferred.

I have a number of other thoughts for this module in the future, and I should add those to the issues soon, but this is a great help. Everything looks good, tests look like they're passing, and the switch from Kafka simplified some of the code. I'm happy to merge this once the ordering is fixed.

nicwolf commented 3 years ago

Thanks @dmcollom :smile:

We've decided that the antares_client should be returning results newest -> oldest anyways, by default. I've pushed some changes there and bumped the dependency version in setup.py. Otherwise no changes, ready to merge when you are!

Thanks for reviewing, we're really excited about the integration with the TOM Toolkit and I'm hoping to have a bit more time in the future to contribute some work.

Hope you're doing well and talk soon.

dmcollom commented 3 years ago

Seems like this is good to go--I'll merge it and get it released by EOD tomorrow. Thanks @nicwolf !

nicwolf commented 3 years ago

Great, thanks David!