open-fresh / bomb-squad

Automatic detection and silencing of high cardinality series in Prometheus.
Apache License 2.0
139 stars 12 forks source link

Implement remaining Prometheus config structs #11

Closed cboggs closed 6 years ago

cboggs commented 6 years ago

The current alpha implementation of Bomb Squad can only properly re-marshal static and k8s SD scrape configs. This was done initially because:

I'm not entirely sure that full (or even partial) re-implementation is necessary, but I've not quite found a better way yet. Reimplementing is easy, if tedious, but I'm concerned with upstream version changes breaking Bomb Squad. Even with a proper import of all the config packages from upstream, we'd likely need a prom-version command-line arg anyways, so maybe it's fine to reimplement and simply add some version isolation for sanity's sake. I don't imagine the Prometheus config will be a very volatile bit of the codebase, and SD features are under moratorium for now, so this is probably fine for the moment.

pcarranza commented 6 years ago

I wouldn't recommend reimplementing the configuration parsing from Prometheus.

In a non-k8s world, we could simply use a single file for the recording rules that are necessary. But I'm not so sure about relabeling.

One possible thing to do is to simply treat the whole configuration of Prometheus as a map of maps and only pay attention to the sections that are necessary, and simply ignore the rest as it will be serialized back again when saving.

Which sections are the ones that are being modified?

cboggs commented 6 years ago

I've tried the approach you suggest (map of maps of ... etc), but anything that isn't explicitly un-marshalled to the proper "depth" will end up re-marshalling as an empty value in the yaml or as a literal string of map[string]interface{}. It's... weird, at best. It may be possible to build up a "dumb struct" that is just a bunch of properly nested map[map[string]string]map[string]string or something, but that feels even worse than reimplementing the actual config. :-\

As for the recording rules, that's already managed in a separate rules file. That file, though, needs to be known by Prometheus, hence it needing to be updated for the bootstrap steps.

In a non-k8s deployment, it might easy-ish to just append rules to an existing rules file, but then we land in a bad spot when config management of some sort ends up overwriting that rules file.

pcarranza commented 6 years ago

Nah, let's keep files separated, it makes sense because then it's also easier to remove things (just rm the file)

I just checked because I didn't know and there is a common library where the configuration is located, it doesn't look like so bad of a dependency.

pcarranza commented 6 years ago

Scratch that, it's only common structs, which means http 😒

pcarranza commented 6 years ago

What if we ask?

Else, copying the structs over to an internal package seems to be the least bad approach.

cboggs commented 6 years ago

So I opened PR #21 to address this. Going to open another shortly when the Prometheus config pacakge(s) pulled in properly and this goes away, I hope.