Graceful fallback when config is missing a property?

[purew]

It would be useful if a missing property in the config file fell back gracefully to the default value, rather than killing the process with a hard error.

Is there any appetite for such a change? We could potentially specify default values in a json file that the C++ code reads at compile time and the python script valhalla_build_config parses at runtime?

An example would be forgetting to add loki.service_defaults.street_side_tolerance to the config.json. Then the router crashes like

./valhalla_service ~/data/planet.json  1
2021/05/26 15:32:14.244181 [INFO] Tile extract successfully loaded with tile count: 82923
2021/05/26 15:32:14.278646 [WARN] /data/valhalla/elevation/ currently has no elevation tiles
terminate called after throwing an instance of 'boost::wrapexcept<boost::property_tree::ptree_bad_path>'
  what():  No such node (loki.service_defaults.street_side_tolerance)
fish: Job 1, './valhalla_service ~/data/plane…' terminated by signal SIGABRT (Abort)

[CuriOusQuOkka]

At the moment we have a python script valhalla_build_config that creates json and puts the modified custom values ​​into it.
There is a C++ reader in rapidjson_utils, it is used in valhalla_service, valhalla_build_tiles, valhalla_associate_segments, etc.

• I think that a good solution to the original problem might be to use proto2, where you can specify a default value. But using protobuf makes the file not human-readable. (In case it is necessary, will add another script that converts protobufs to jsons)
• Another solution would be to move the default json from valhalla_build_config code to a separate json and implement "merge" functionality in rapidjson_utils logic. The disadvantages include the difficulty of understanding the code.

@kevinkreiser @purew What do you think about it?

But right now I think that this task will only put spokes in our wheels in the future. We will not control the freshness of the data in config.json.
For example, imagine that a user has a config and does not update it for a long time, we replace one of the existing default values, the user does not change the config because it is "good, automatically updated, etc.", he thinks. Time passes and the user has a bug on this basis, it becomes more difficult to find it.

[mandeepsandhu]

Currently, we don't validate the json before using it and thats why we sometime get errors deep within the code when a property is accessed for the first time.

1 easy thing to do would be to put in schema validation before using the config.

Another option is to have a config abstraction built on top of what the user-supplied config json which is then used internally. The internal config representation can make decisions on what default values to use if an attribute is missing (it can simply store the schema-validated json document internally and provide a similar API as the JSON doc). Eg: if you try to get an attribute thats optional, and its not supplied in the user config, the internal abstraction can return a default value.

[purew]

Another option is to have a config abstraction built on top of what the user-supplied config json which is then used internally. The internal config representation can make decisions on what default values to use if an attribute is missing (it can simply store the schema-validated json document internally and provide a similar API as the JSON doc). Eg: if you try to get an attribute thats optional, and its not supplied in the user config, the internal abstraction can return a default value.

I like this idea of parsing the json once and then ending up with something like

struct ValhallaConfig {
  HttpdConfig httpd,
  LokiConfig loki,
...
  // etc
}

But right now I think that this task will only put spokes in our wheels in the future. We will not control the freshness of the data in config.json.
For example, imagine that a user has a config and does not update it for a long time, we replace one of the existing default values, the user does not change the config because it is "good, automatically updated, etc.", he thinks. Time passes and the user has a bug on this basis, it becomes more difficult to find it.

I'm not sure if it could be worse than the current situation. Default values can change today as well with people running older configs.

The benefit of implementing this ticket is that should a default value change, it'd be a seamless upgrade with a new valhalla binary as you'd get the updated value if you haven't specified an explicit value in your existing config file. So from that perspective it'd be better, right?

An example:

I'm only really interested in running large matrix requests, so my config.json really only specifies bigger limits on matrix and nothing else (apart from the basedata). Any changes to default values in newer valhalla binaries would apply to me as soon as I upgrade because I haven't been forced to specify them in the config file already.

[kevinkreiser]

Yeah I swear I wrote an issue about having config loaded/validated once at start up. I hadn't thought about default values though I understand why that would be desirable. I would like to point out that there are a few types of values in the config:

1. ones that aren't required (mjolnir.concurrency)
2. ones that are required but can easily have a default values (service_limits)
3. ones that are required but need a person to choose a value (tile_directory, tile_extract)

for the second type, @CuriOusQuOkka has a good point. it is possible that a person comes to rely on the default but then the default changes and whatever they were doing is now impacted. i dont see a way around this other than people becoming more responsible for configuring their software (which is ok with me, you have to have a clue to some degree about what you are doing).

I find it very difficult to suggest defaults for the third type. the only thing we can do there is maybe not have defaults for those and make those such that the user must supply them? or we could put defaults that basically dont work without user intervention (which is what we do now). the big question would be can you use valhalla without a config at all or must you at least supply a tile_dir/tile_extract?

implementation-wise what are we suggesting here concretely? is it something like the following?

1. take the default config and wrap it in a config object
2. take the validation from the various workers and stages of mjolnir and add those to the config object
3. add a bit of code to the config object to merge in partial user-supplied configs (ie the only have the values the users care about)
4. modify the python config generation to spit out partial configs rather than full ones with defaults

if so i might suggest we do a minor revision release after this to signal a more significant change of functionality

[CuriOusQuOkka]

Currently the scope looks bigger than we expected and the task is not prioritised. Will pause it for now.