twosigma / satellite

Satellite monitors, alerts on, and self-heals your Mesos cluster.
Apache License 2.0
143 stars 18 forks source link

yaml configuration options #33

Closed tnn1t1s closed 8 years ago

tnn1t1s commented 8 years ago

Users who are not comfortable w/ Clojure have asked for a yaml or json based configuration file option satellite. we can discuss design and details here.

leifwalsh commented 8 years ago

Would they be satisfied with just edn (with just primitive types like 60 rather than (-> 60 t/seconds))?

Here are some options I've found:

dgrnbrg commented 8 years ago

We should probably just marshal ourselves--there's only minimal data type enrichment necessary, and by doing the conversion ourselves we can write more user-friendly error messages. On Tue, Nov 3, 2015 at 2:10 PM Leif Walsh notifications@github.com wrote:

Would they be satisfied with just edn (with just primitive types like 60 rather than (-> 60 t/seconds))?

Here are some options I've found:

— Reply to this email directly or view it on GitHub https://github.com/twosigma/satellite/issues/33#issuecomment-153505397.

mforsyth commented 8 years ago

Assuming we provide a way to configure Satellite using a static data file, is there still a reason to maintain the .clj format as another option? It seems simpler to offer 1 consistent configuration format.

tnn1t1s commented 8 years ago

@leifwalsh a format w/ primitive types would be acceptable. The biggest complaint was parentheses and the time example you mention.

mforsyth commented 8 years ago

@tnn1t1s @dgrnbrg @leifwalsh Based on this conversation, would a good first step be to offer only .edn as an option for the config (in addition to the current .clj)?

With .edn configuration, we would need to replace comet definitions in the file like (satellite-slave.recipes/load-average 30 (-> 60 t/seconds)) with e.g. ["satellite-slave.recipes/load-average" 30 60] Is there a common need to produce a Period using a unit other than seconds?

The .edn configuration might be more readable if we made each comet a map like this: {:recipe ""satellite-slave.recipes/load-average" :params [30] :period 60} Can we assume that every recipe function will take the period as the last param (as all of the examples currently do)?

Also, it doesn't seem like inline comets (like the one at the bottom of satellite-slave/config/sample.clj) are worth supporting in an .edn format. We could allow arbitrary code to be in included in an .edn as a string, but If someone wants clojure code in their config, it's reasonable to ask them to use the option for .clj configuration, right?

mforsyth commented 8 years ago

It strikes me that it will be cleaner to support both .clj and .edn forms of configuration (and, eventually, other forms of config), if we expect the last form in a .clj config to evaluate to a map, which core.clj then merges into the default config map, rather than having myconfig.clj update the "settings" var as a convention (which is what we have now). Obviously, any form of data-based config isn't going to be able to update a var in a namespace, and this might remove some noise from .clj config files as well (they should just be code that evaluates to settings which is then treated like any other config).

A potential problem here is that it would be backwards incompatible with existing configuration files, which don't "return" a map. Should I be sensitive to this, and make sure that any existing config.clj will continue to work, at the expense of a bit of complexity?

mforsyth commented 8 years ago

Proposed sample master config edn file: https://gist.github.com/mforsyth/5936df35aad3b8605e7c

Proposed sample slave config edn file: https://gist.github.com/mforsyth/ee092675d1d6dce35e0a

dgrnbrg commented 8 years ago

I think we need to review whether there's a benefit from using edn over just removing the need for convenience functions to configure satellite's clj file. On Tue, Nov 17, 2015 at 3:52 PM Matthew Forsyth notifications@github.com wrote:

Proposed sample master config edn file: https://gist.github.com/mforsyth/5936df35aad3b8605e7c

Proposed sample slave config edn file: https://gist.github.com/mforsyth/ee092675d1d6dce35e0a

— Reply to this email directly or view it on GitHub https://github.com/twosigma/satellite/issues/33#issuecomment-157502481.

mforsyth commented 8 years ago

@dgrnbrg by "convenience functions" do you mean the ones in recipes.clj e.g. "free-memory", or the function "every"? Or the actual output parsing functions associated with comets?

mforsyth commented 8 years ago

We've got the configuration process reading from a static JSON file which contains many of the actual configuration values. Can combine static config and clojure config for e.g. custom comets as needed.