Improve configurability

ianthetechie commented 1 year ago

:wave: I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.

Here's the reason for this change :rocket:

This PR does not fix any open issues that we're aware of, but it adds the following features which we think are generally useful to upstream ;)

In particular, it makes the Pelias API 1) more configurable via tunables which can be customized per use-case, and 2) improves the correctness of HTTP status codes.

Here's what actually got changed :clap:

Allows easy overriding of request variables in the configuration file. This lets users of the Pelias API customize things like weights to different factors without modifying code, pushing a new container, etc.
Return a 502 HTTP status code when a backend service returns an error, refuses connection, or times out. These previously resulted in 400 Bad Request, which is just not correct ;)
[Incidental change] Make the shebang lines more portable. While Linux systems have historically thrown the kitchen sink into /usr/bin, that isn't the case on other UNIX variants (ex: FreeBSD, which I often develop on), and it's quite likely Apple won't ship bash at some point in the future, so using /usr/bin/env bash will ensure portability.

Here's how others can test the changes :eyes:

The PR contains unit tests demonstrating 502 responses on some real backend errors.

A straightforward approach to testing config overrides didn't immediately come to mind as I don't write JS very often, but here is an example fragment of a pelias.json config demonstrating the use if you want to test locally:

{
  "api": {
    "autocomplete": {
      "default_overrides": {
        "population:weight": 1.2,
        "focus:weight": 10
      }
    }
  }
}

orangejulius commented 1 year ago

Hi there @ianthetechie 👋,

Thanks for these changes. Some thoughts on the different things included in this PR:

config overrides

This has been a frequently discussed topic, and many people have asked for us to allow changing default settings in pelias.json. We've always maintained that there's some sort of conceptual difference between things that are already in pelias.json and those defaults, but it's probably been muddied over time. I definitely see the appeal of the convenience for testing especially.

Despite our past stance on this I think your change is pretty simple and helpful, and so I'd support it being merged as long as we make it clear in the docs that any settings accessed this way are 100% "you break it, you get to buy the pieces". I don't love the idea of having to triage issue tickets only to find out people have ridiculous values for some default settings that they don't even fully understand in their pelias.json. @missinglink what do you think?

Error handling

Yeah, Pelias API error handling has always been bad. The most glaring issues were fixed shortly after we launched Geocode Earth back in 2018 (https://github.com/pelias/api/issues/1108, https://github.com/pelias/api/pull/1231), but there are still a bunch of cases where errors from services like Placeholder and Interpolation can cause Pelias to return a 4xx error when it should return 5xx.

Can you split those changes into a new, clean PR and describe a bit more about exactly what you saw that caused the errors? We'd always want to track those down.

shebangs

I don't think explicit FreeBSD support is going to be a goal of Pelias any time soon, but it seems like a relatively reasonable change. We do in practice need to support Macs since most people will be developing on them. Same thing: new clean PR with just those changes please.

missinglink commented 1 year ago

Hi 👋 yep totally agree with what you said Julian.

I share your concerned that having too many runtime tuneables will make triage and maintenance difficult.

If the settings can be improved then I would prefer to see everyone share their insights from testing & tweaking these settings in the spirit of open-source. It could be that each installation needs a different set of values or it could be that we work together to tune 'ideal' values.

Please split the PR into parts which are logically distinct so the review process is easier and the changelog makes sense.
Sounds reasonable, it would be nice if things 'just work' everywhere.

pelias / api