Conf int with reload

[kylebrandt]

wip:

# kbrandt at grove in ~ on git:master x [16:19:15]
$ curl -XPOST 'http://localhost:8080/api/config/alert' -d "{
    \"Name\" : \"meowNew\", \"AlertText\" : \"alert meowNew {\n\twarn = 0 + 6\n}\", \"Hash\": \"\"
}"
reloaded%
# kbrandt at grove in ~ on git:master x [16:19:12]
$ curl -XDELETE 'http://localhost:8080/api/config/alert/meowNew'
meowNew deleted%

[kylebrandt]

#705

[kylebrandt]

example file save hook https://gist.github.com/kylebrandt/c4c9099118bea6e4ab793e6b657a1c2c

[kylebrandt]

Example sys conf

Hostname = "bosun.example.com"
HTTPListen = ":8080"
TimeAndDate = [ 202, 75, 179, 136 ]
ShortURLKey = "aKey"
CommandHookPath = "/Users/kbrandt/src/hook/hook"
RuleFilePath = "/Users/kbrandt/src/testProdRepo/prod.conf"

[OpenTSDBConf]
    Host = "ny-tsdb01:4242"
    Version = 2.2
    ResponseLimit = 25000000

#Test comment

[ElasticConf]
    Hosts = ["http://ny-lselastic01.example.com:9200", "http://ny-lselastic02.example.com:9200","http://ny-lselastic03.example.com:9200"]

[DBConf]
    RedisHost = "localhost:6389"

[SMTPConf]
    EmailFrom = "bosun@example.com"
    Host = "mail.example.com"

[captncraig]

I think the interface adds complexity here we don't really need. Why not just a struct without Getter Functions? Are we afraid some code will change it without going through the conf package? I'm not really super worried about that.

[kylebrandt]

I'm okay with removing the systemconf interface.

[captncraig]

I'm not sure about this check. What if someone is only using post notifications?

[kylebrandt]

bad logic, the previous was ||, but what I mean was "if one, than the other". Will fix

[captncraig]

Why is this wrong? I think its pretty cool.

[captncraig]

Set/Delete belong on RuleConfWriter, but you may already know that.

[captncraig]

No GetMacros or GetNotifications? Are those only looked at when an alert references them?

[kylebrandt]

GetMacro isn't actually used anywhere. Will remove for now

[captncraig]

I'm ok leaving this for now, but the rawText abstraction may get broken if/when we add an alternate backend

[captncraig]

I think this is the correct behavior for now. But we will need to think about what happens if a config provider fails. All of those methods will likely need to return an error in the future.

[captncraig]

Is this your TODO?

[kylebrandt]

Before it was non exported, nodePair, Pairs []nodePair, so this was a shortcut to get it working when I moved the rule stuff to a different package

[captncraig]

What value does LocationType add? Wondering if we just leave it out for now.

[captncraig]

I'm a little shaky on calling this package rule. Not sure what would be better though. Maybe alerts?

[captncraig]

Or is this really the file provider? Not sure.

[captncraig]

Why does the ruleProvider need to be given the save hook. Can't the reload function below just execute it?

[kylebrandt]

The file is saved, and then the hook is called. If the hook fails, the orginal file is restored:

if c.saveHook != nil {
        err := c.callSaveHook(c.Name, user, message, args...)
        if err != nil {
            sErr := c.SaveConf(c)
            restore := "successful"
            if sErr != nil {
                restore = sErr.Error()
            }
            return fmt.Errorf("failed to call save hook: %v. Restoring config: %v", err, restore)
        }
    }
    err = c.reload()

Also the hook is passed arguments that reload doesn't need to be aware of. But maybe there is a better way?

[captncraig]

fmt

[captncraig]

parens here please. I can never remember priorities.

[captncraig]

or how about:

hasHost := sc.GetSMTPHost() != ""
hasFrom := sc.GetEmailFrom() != ""
if hasFrom != hasHost{ERROR!}

[captncraig]

all of these comments are really helpful btw. Thanks.

[captncraig]

I love the emoji, but these incantations are repeated a few times. How about a helper on the Location type?

textBefore(originalRaw, l)
textAfter(originalRaw, l)

may not be worth it.

[captncraig]

I still feel like some of these fields should be in the rule conf. Is there a technical reason they are here, or is it a choice?

CheckFrequency DefaultRunEvery MinGroupSize PingDuration(maybe) and UnknownThreshold all seem like good candidates for rule config.

[kylebrandt]

That was my original thought as well. The reason I put them in system was more of a permissions. This is because editing those will change the way all alerts in Bosun will run. So having those in the system conf seemed safer to me.

[captncraig]

Can we reduce duplication here? Maybe LoadFile uses ioutil.ReadFile then calls this method?

[captncraig]

Trying to reason about orphaned goroutines here. I think this chan should be buffered with size 1.

[captncraig]

buffer of 1

[captncraig]

I think we should go back to precalculating runevery per alert. This also looks to me like a reason default run every and check freq should be moved to rule conf again.

[captncraig]

I feel like there should be a timeout here, but I'm not sure what we would do. I don't like the idea of a long running expression being unkillable. Maybe I'm worrying too much?

[kylebrandt]

The expressions are cancelled while running. So the only thing that we wait for to complete is the run history.

[captncraig]

I hate panics. Return an err maybe? Return nil maybe? Is this only called from web requests?

[captncraig]

That's all I have for now.

[kylebrandt]

@captncraig Do you see any potential issues with dispatchNotifications which is run from schedule.Run() in alertRunner.go? I'm wonder if on reload (and currently in a shutdown) notifications could potentially be lost and never sent - that would be bad.

[captncraig]

Lets go ahead and delete this at this point.