search

vimeo / graph-explorer

A graphite dashboard powered by structured metrics
http://vimeo.github.io/graph-explorer/
Apache License 2.0
1.06k stars 93 forks source link

Move configuration to a non-Python config file, specified on the commandline #103

Closed hollobon closed 10 years ago

hollobon commented 10 years ago

As discussed in pull request 100.

hollobon commented 10 years ago

Let me know what you think.

preferences.py is a bit more awkward.

Dieterbe commented 10 years ago

i merged this with current master, which adds some fixes and a few new anthracite config options. see https://github.com/vimeo/graph-explorer/tree/pr-103-updated

my thoughts: this is kindof a weird config format. no quotes around strings. lists should have their elements "on an indented new line" ? can't imagine how that would look like for the metric_plugin_dirs setting.

also the benefit of python configs is that if you change the config, the server would automatically reload. sucks that we lose that advantage, although i can see benefits in a more plain config file, but maybe we should use a more traditional ini syntax or toml.

another thing that happens now when an option is missing in your cfg, you get this stacktrace.

Traceback (most recent call last):
  File "/home/dieter/workspaces/eclipse/graph-explorer/ge-dev/bin/run_graph_explorer.py", line 10, in <module>
    execfile(__file__)
  File "/home/dieter/workspaces/eclipse/graph-explorer/bin/run_graph_explorer.py", line 41, in <module>
    sys.exit(main())
  File "/home/dieter/workspaces/eclipse/graph-explorer/bin/run_graph_explorer.py", line 17, in main
    config.init(args.configfile)
  File "/home/dieter/workspaces/eclipse/graph-explorer/graph_explorer/config.py", line 31, in init
    config.anthracite_host = parser.get("anthracite", "host") or None
  File "/usr/lib64/python2.7/ConfigParser.py", line 618, in get
    raise NoOptionError(option, section)
ConfigParser.NoOptionError: No option 'host' in section: 'anthracite'

it used to be that missing values would show up nicely in the config errors output along with bad values.

Another problem I have is:

anthracite_port 
    Number must be between 0 and 65535.

1) this setting doesn't exist. ("port" in the "anthracite" section does exist, so maybe we should clarify this difference) 2) the cfg contains

[anthracite]
port = 9200

and graph_explorer/config.py contains:

config.anthracite_port = parser.get("anthracite", "port") or 9200

so why is it still complaining?

Dieterbe commented 10 years ago

never mind what i said about the weird format. no quotes is fine, and i see now how the newline thing works, because there's no native support for lists

Dieterbe commented 10 years ago

ok i fixed that anthracite_port had to use getint(); i also made it ignore NoOptionError and NoSectionError so that validation can do its job. see https://github.com/vimeo/graph-explorer/tree/pr-103-updated one interesting i noticed though, is that config.init() gets called twice (with the same argument), but the second time the parser can't see any sections in the config (wtf). verified this by patching ConfigParser.py in get() print self._sections.keys(). so this still causes the thing to break. also didn't used to happen (before i did the patch)

Dieterbe commented 10 years ago

i pushed a workaround for that last issue, and merged it. feel free to have a look at the commits to see if you think you can make the code better.

thanks for your work :)

hollobon commented 10 years ago

Cheers - will take a look. Sorry for late reply, been away on holiday.