code cleanup and refactor

[woodsaj]

There are a number of code cleanup initiatives we want to tackle. Config, logging, plugin management.

The plugin-management changes are needed so that we can build metrictank-enterprise versions with more advanced input, index and store plugins (plus any other future features we want to add).

To be able to do that, we need to overhaul how we initialize and share components/plugins.

I have been looking at the Grafana code base and I really like what they have done with self-registering components and dependency injection. I think that is the model we should follow.

I have built a basic POC of how all of the capabilities work together. I have tried to cover all of the use cases that we have in Metrictank. The highlights are:

• support for multiple input plugins at the same time. (https://github.com/woodsaj/go-server/blob/master/workers/workera/service.go#L31)
• Enforcement of only 1 idx/store plugin (https://github.com/woodsaj/go-server/blob/master/processor/processorbar/service.go#L29)
• Configuration validation at startup after configs have been loaded (https://github.com/woodsaj/go-server/blob/master/workers/workera/service.go#L28)
• component startup ordering (https://github.com/woodsaj/go-server/blob/master/registry/registry.go#L17)
• Handling components that have long startup processes, that other components should wait for. (https://github.com/woodsaj/go-server/blob/master/workers/workera/service.go#L50-L58)

Trying to update everything to match this architecture in one go is going to be a huge PR, that would be hard to review. But i think we should be able to transition to this architecture in small steps.

The most impactful change is going to be updating how we manage configuration. I have really come to like github.com/spf13/viper. Viper doesnt work with INI files, so would need to move to json or yaml. But otherwise i think we can keep the same config names and ENV vars.

[Dieterbe]

Viper doesnt work with INI files, so would need to move to json or yaml

viper supports toml, which is basically a more formally defined (standardized) implementation of ini files.
I think ini (toml) is pretty great, and json/yaml are not a good fit for configs (too extensive).
if we could transition to a better config library such as viper, while keeping compatibility with current config files and env vars (and override rules between them) that would be great.

[woodsaj]

I agree that json isn't pleasant to work with. But yaml can be fine, depending on how you layout the file.

The thing I like most about viper is that it normalizes the config variable names for you.
eg the yaml

---
cassandra:
  addrs: localhost
  consistency: one

is the same as the yaml

---
cassandra.addrs: localhost
cassandra.consistancy: one

which is the same as the toml

[cassandra]
addrs = "localhost"
consistency = "one"

I have played around with trying to get viper to parse an ini file. you cant just treat the ini file as a toml file, as ini does not require strings to be quoted, but toml does.
But i have been successfull in loading in an inifile, then writing it to a buffer as a yaml file, then loading that into viper. Seems to work without issue. see woodsaj/go-server#1

So it looks like we should be able to support a transition period where we expect the ini file format by default, but also allow users to use toml, yaml or json by passing a cmdline flag. Then after the transition period we default to using toml, yaml or json and make ini optional via the cmdline flag. And eventually drop support for ini.

[woodsaj]

@Dieterbe if you are happy with the architecture changes proposed here, then i think we can formulate a plan of attack. I am thinking we should make the following changes, in this order

• move to logrus for logging.
• refactor to use Viper, keeping the current ini config.
  • replace the "ConfigSetup()" funcs in packages with an "init()" that sets defaults.
  • instead of using package variables for config settings, we just get them from viper when needed via viper.GetBool(), viper.GetString(), etc...
  • rename the "ConfigProcess()" funcs to "Init()", inline with our future component registry.
• Add the registry and start moving components to it. Moving all components to the registry would be done over a few PRs

[Dieterbe]

haven't had time yet to look into all of the architecture changes, but we can start with the switch to logrus (#624)

[Dieterbe]

some prior discussion re config in #456

[Dieterbe]

realistically I won't be able to give this the attention it needs for at least another week of 2,
due to various operational issues . is that a problem? are we blocked on this?

[Dieterbe]

came across https://blog.golang.org/wire
if it's not too complicated, looks like an interesting alternative over inject

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.