SimplyStaking / panic

PANIC Monitoring and Alerting For Blockchains
Apache License 2.0
82 stars 31 forks source link

Nikhil/add peering alert #380

Closed nivasan1 closed 1 year ago

nivasan1 commented 1 year ago

In this PR

dillu24 commented 1 year ago

Hey @nivasan1 should we review this? I am asking because you closed it.

nivasan1 commented 1 year ago

Hey @nivasan1 should we review this? I am asking because you closed it.

Hi! Yes, this PR is ready for review, please let me know if you all would like more context on mev-tendermint / why the alerts added are relevant. Thanks!

itsciccio commented 1 year ago

Have all tests passed?

itsciccio commented 1 year ago

Have you ran PANIC with docker-compose up --build -d? Did it run without errors?

itsciccio commented 1 year ago

Have you ran PANIC with docker-compose up --build -d? Did it run without errors?

Is the new alert configurable in the UI? (When setting a cosmos node, in the alerts configuration step, are the new alerts shown in the list, and are you able to be configure it (ex: change severity)?

itsciccio commented 1 year ago

Great work on this PR! Thank you for abiding by our code standards.

itsciccio commented 1 year ago

Okay, I just noticed that the data store is not updated to cater for these new metrics. Kindly have a look at the Cosmos Node Store code and amend it to cater for these new metrics being added.

The AlertStore might also need to be amended for the new alerts. Have a look at the codebase, although it might be catered for automatically.

itsciccio commented 1 year ago

Would be great if you can update the relevant docs

nivasan1 commented 1 year ago

Have all tests passed?

Yes, I have also modified the testing suites to accomodate the newly added alerting logic

nivasan1 commented 1 year ago

Have you ran PANIC with docker-compose up --build -d? Did it run without errors?

Is the new alert configurable in the UI? (When setting a cosmos node, in the alerts configuration step, are the new alerts shown in the list, and are you able to be configure it (ex: change severity)?

I have been able to run the alerter with both a mev-tendermint / non-mev-tendermint node, both the node_is_peered_with_sentinel and validator_is_peered_with_sentinel alerts are configurable in the panic UI. I have also tested that the regular alerts / new alerts were emitted at the expected times. Please let me know if there is anything else I should be looking into further

dillu24 commented 1 year ago

unless there is anything else from @itsciccio side all there is left for this to be merged is the changes suggested above

itsciccio commented 1 year ago

Super! We can proceed with the merge. Thank you for resolving all the threads which I opened.

Will need to check something with the team prior to merge. But all okay from your end.

itsciccio commented 1 year ago

One last thing: Please update the changelog here, in the Unreleased section: https://github.com/SimplyVC/panic/blob/master/docs/CHANGE_LOG.md

Just so we can keep track of these community contributions.