search

coralogix / coralogix-operator

Coralogix Operator for integration with Kubernetes clusters.
https://coralogix.com
Apache License 2.0
0 stars 7 forks source link

support setting integration name for alert #167

Open alexandermarston opened 1 week ago

alexandermarston commented 1 week ago

We'd like to start pushing some of our PrometheusRule evaluations through Coralogix, but at the moment we can't seem to natively setup the notification integration without using a Coralogix specific Kubernetes resource.

It would be nice if we could add an additional annotation to our already existing PrometheusRules to specify the name of the Notification Integration we would like to use.

Looking for some feedback on this approach.

CLAassistant commented 1 week ago

CLA assistant check
All committers have signed the CLA.

OrNovo commented 1 week ago

Hi @alexandermarston. Thank you for your contribution. Please take into account that this may create a conflict with future support of alert-manger (ie alert-manager controller will override the logic you suggested). Please resolve the conflict and I'll approve your PR.

alexandermarston commented 1 week ago

Hi @alexandermarston. Thank you for your contribution. Please take into account that this may create a conflict with future support of alert-manger (ie alert-manager controller will override the logic you suggested). Please resolve the conflict and I'll approve your PR.

Thank you for the feedback.

Would ignoring the cxNotificationName annotation if the PrometheusRule is labelled with coralogix.com/managed-by-alertmanger-config be enough?

Just as some added context, the Alert Manager integration looks neat but as it currently stands it wouldn't be compatible with our Alert Manager implementation. We use the Alertmanager resource, but define our routes and receivers in a configuration secret not in the resource itself.

OrNovo commented 1 week ago

Hi @alexandermarston. Thank you for your contribution. Please take into account that this may create a conflict with future support of alert-manger (ie alert-manager controller will override the logic you suggested). Please resolve the conflict and I'll approve your PR.

Thank you for the feedback.

Would ignoring the cxNotificationName annotation if the PrometheusRule is labelled with coralogix.com/managed-by-alertmanger-config be enough?

Just as some added context, the Alert Manager integration looks neat but as it currently stands it wouldn't be compatible with our Alert Manager implementation. We use the Alertmanager resource, but define our routes and receivers in a configuration secret not in the resource itself.

Yes. But we will deal with it separately.

Got it. Guess that will be handled separately when we get all the necessary information.

alexandermarston commented 1 week ago

Thanks for the approval @OrNovo . I am still happy to add the additional check now, if you'd like?

I can see some tests are failing, but I don't believe they're related to my change.

assafad1 commented 5 days ago

@OrNovo regarding failing tests, I see on the e2e-test logs:

2024/10/13 15:21:42 Warning: teamLevelAPIKey was not provided. Some functionality will not be available.
2024/10/13 15:21:42 Warning: userLevelAPIKey was not provided. Some functionality will not be available.

I think there is no access to the repo secrets, because the tests run from @alexandermarston's fork. I think we can merge this PR.

assafad1 commented 5 days ago

@alexandermarston can you please sign your commit?