Remove "omitempty" from exported config options

[pracucci]

While reviewing the PR #2165 (which adds the /config API endpoint) I realized the omitempty we have across the config are hiding part of the config when exported by the GET /config endpoint.

I would suggest to get rid of all omitempty across the Cortex config structs except for deprecated fields.

[gouthamve]

Wait, but why do we want to have 0s and "" in the exported config? Isn't it cleaner to omit the fields?

[pracucci]

Very legit question.

Wait, but why do we want to have 0s and "" in the exported config?

Because the zero value is not necessarily the default value in our config (the default is registered along with the CLI flags).

If we keep omitempty for field whose default is not the zero value, then the exported config doesn't match the current config for all these field where the config sets the zero value. May look an edge case but it's quite common (ie. 0s to disable something): we also hit this in integration tests (and I realized it thanks to it).

Removing all omitempty looks the safest approach to me. It's required only for field whose default value is different than the zero value but, unless we add a custom linter for that, keeping it correct over the time may be complicated (we'll easily miss this during code reviews). Thus my suggestion to remove all.

Isn't it cleaner to omit the fields?

It depends. The exported config is huge anyway. It's not the few omitempty fields which will reduce it significantly. If you look at the the exported config as the source of truth of the current config than even hiding defaults may be annoying to the user, cause if you wanna check a specific setting you need to look for it in the config and then fallback to the doc for defaults.

[jaybatra26]

I would like to take this up. Please assign it to me.