Skip to content
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

Treat notification as an object instead of a string #20

Open
sjung-stripe opened this issue Feb 28, 2018 · 3 comments
Open

Treat notification as an object instead of a string #20

sjung-stripe opened this issue Feb 28, 2018 · 3 comments

Comments

@sjung-stripe
Copy link
Contributor

Currently, the notifications associated with a detector rule are stored as a list of strings:

rule {
    description = "maximum > 60 for 30m"
    severity = "Critical"
    detect_label = "Processing old messages 30m"
    notifications = ["Email,foo-alerts@bar.com"]
}

I would like to propose storing them as objects instead:

rule {
    description = "maximum > 60 for 30m"
    severity = "Critical"
    detect_label = "Processing old messages 30m"
    notification {
        type = "Email"
        email = "foo-alerts@bar.com"
    }
}

or perhaps even:

rule {
    description = "maximum > 60 for 30m"
    severity = "Critical"
    detect_label = "Processing old messages 30m"
    email {
        email = "foo-alerts@bar.com"
    }
}

Motivation for this change: I am planning to add support for the signalfx teams api to signalform, and teams support a notificationlist which (to my understanding) uses the same object as detector rules. It would be useful to have the same structure in both places. In addition, this approach would more closely reflect the underlying API's data model, and would allow for more types of notifications to be supported. (For example, signalform does not currently support sending notifications to a team, which could have its own notification configuration.)

What are your thoughts on this change? I am happy to implement it myself if this is something you'd be willing to accept.

@poros
Copy link
Contributor

poros commented Mar 1, 2018

This will require quite a bit of refactoring on our side, since we have countless detectors and a couple terraform modules relying on this being a string. @dichiarafrancesco is part of the team maintaining our internal repos, so any non-backward compatible change to the API must be approved by him or someone else on the Metrics Infra team.

Anyway, I think that your change makes sense; I don't know what's the reason why this was implemented as a string in the first place. Not sure which one of the two approaches you proposed is better, though. And team support for SignalFx is something we always had in our backlog, so that is definitely exciting!

@dichiarafrancesco
Copy link
Contributor

Will give it a look on Monday. I need sometime to plan the eventual shift from our current way to-do-things and the new one. hope it's not a big blocker for you.

@joshu-stripe
Copy link
Contributor

Would it be an acceptable workaround if, for the time being, we supported both? We could have notifications as the keyword for the list of strings, and notification as the keyword for the more thorough HCl description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants