Consider using Ficus for reading in configuration

[nsauro]

I see in a lot of places, we are reading in Config values as so:

https://github.com/tumblr/colossus/blob/a48178e11c62df7b7f9e7664c0806e960d83473a/colossus/src/main/scala/colossus/core/ServerSettings.scala#L71

I also noticed that we have a small set of helpers to facilitate this:

https://github.com/tumblr/colossus/blob/5bc3a45a58d4c0faa0d9387b0ff12808f54fc36b/colossus-metrics/src/main/scala/colossus/metrics/ConfigHelper.scala#L10

There is a project called Ficus: https://github.com/iheartradio/ficus which takes care of this type of stuff and provides a pretty good interface and has pretty robust support for reading in scala specific types(Options, Durations, etc). Using this could reduce some boilerplate in a few places.

[benblack86]

If I remember correctly, @DanSimon was on the fence about whether the typesafe config was a good thing. We hardly use it a Tumblr, apart from changing the request timeout/max-size. My problem is that it is easy to make a typo in the namespace and it wouldn't be used.

Not sure why typesafe config was added? Did someone ask for it? Is anyone using it?

[nsauro]

It is def used by colossus, and pulled in transitively probably via Akka. The ConfigHelpers link I included in the description includes references to it.

Not sure about the problem you described, is that something specific to Typesafe config? Elaborate?

[benblack86]

What I'm suggesting is that colossus stop using typesafe config, i.e. this stuff http://tumblr.github.io/colossus/0.9.1/configuration.html (it would still be used by akka). Everything that can be done in config, can be done via code.

For example, lets say I want to set request timeout. I can either set it via code, or add to application.conf:

colossus.service.SERVICE-NAME.request-timeout = 30 second

Except, maybe I make a mistake and write:

colossus.service.SERVICE-NAME.timeout = 30 second
colossus.service.SERVICE-NAME.request.timeout = 30 second
colossus.service.SERVICE-NAME-SPELT-WRONG.request-timeout = 30 second
colossus.SERVICE-NAME.request-timeout = 30 second
just.a.load.of-bull = 5 cats

Or maybe I add it to the wrong file, or misspell the filename, etc.

I would go to bed thinking I set the request timeout, but actually I haven't.

[nsauro]

So, if you were using something like Ficus, in this case, you would have some case class for the values under colossus.service.SERVICE-NAME, which matches the shape of the expected configuration (field names and types).

case class ServiceConfig(timeout: Duration, requestTimeout: Duration...)

If you the read in a config:

val config = someTypeSafeConfigInstance
val myServiceConfig = config.as[ServiceConfig]("SERVICE-NAME")
...

If the field is missing, and not optional, this will throw an error.

[benblack86]

Currently everything is optional.

[DanSimon]

So it's not entirely true that you can do in code everything you can do with typesafe config. The big exception is metrics.

Without typesafe config, there is no way to configure the metrics built-in to Colossus. When you create either a server or client, Colossus looks in specific places in config (based on the namespace of the service) to find config settings to override the default settings for the metrics. This includes being able to enable/disable specific metrics.

Personally I've always been weary of typesafe config, but I think we've almost got it comfortably integrated. We could fix the issue @benblack86 mentioned by just making service config settings all required. Then we also supply a set of defaults in config, and since Typesafe config supports inheritance, it should be easy for users to import a set of defaults and override the settings they want to change:

//defined in reference.conf
service.server.defaults = {
    requestTimeout = 1 second //or whatever
}

// in user's application.conf
myservice_config = $service.server.defaults
myservice_config.requestTimeout = 5 seconds

//in service
val myConfig = ServiceConfig.load("myservice_config")

Slightly more tedious, but it should help prevent those silent config errors.

[nsauro]

So, there are a few things here.

I can't remark on @DanSimon's opinion on typesafe config. If you guys are considering moving away from it wholesale, that is beyond my capacity to influence, and then this ticket is moot. I would strongly disagree with it, as it would then force users to manage configuration in 2 separate places. TypeSafe config would still need to be used to manage Akka and everything in that realm(including the ActorSystem which Colossus runs on), but then Colossus configuration would be managed..someplace else. Via code, or whatever replacement(if any) is chosen. I think if anything, that would introduce an inconvenience and become less than ideal.

"Currently everything is optional". I don't think I follow this? Are you talking about there being defaults that are provided via a reference.conf file, which allow for users to omit config values from the config file?

As for the "doing it all in code". I don't know how much I agree with this, and don't the same possible mistakes apply? You can misconfigure something in code, just as easily as you do in a config file. If you change the wrong value, or change the wrong instance, or any numerous other mistakes that can be made..the compiler won't save you there. If I remember correctly, deeper integration with typesafe config was something we did a while back because the code to manage all of the configurations needed for Colossus, especially metrics, started to become cumbersome and tedious to manage via code.

I don't want to discuss the "handle in code" part too deeply, as I feel like it is a bit off topic, and only am touching on it since you bought it up. I say it is off topic because this ticket was created for discussing introducing ficus as a way of reducing boilerplate from the sites where we read in config values from a typesafe config object.

[nsauro]

Just saw @DanSimon's comment, I didn't realize he wrote it as I was writing mine :)

[benblack86]

I like @DanSimon's suggestion to require the config, although that then makes setting up a colossus service a little more complex. Maybe we should punt on that unless people have strong feelings.

Since there is at least two people in favor of typesafe config, I think it is fine to keep it and tidy up the implementation using ficus.

[nsauro]

So, is this a go, and should I start working on it?

[benblack86]

Sounds good to me.

[DanSimon]

Same here. I know from experience that Ficus is super useful, so I'm not opposed to the extra dependency.

[benblack86]

@nsauro you working on this?

[nsauro]

Started to yea, but have been a bit busy of late. Hope to get some more time on it this week

[benblack86]

What's the status on this? Happy to take this over.

[benblack86]

Closing this since we don't want to add an extra dependency.