Pinging @elastic/es-core-infra (Team:Core/Infra)

Can fallbacks be implemented on top of aliases, so we don't have two separate concepts that do kinda the same thing but in different ways?

Can fallbacks be implemented on top of aliases, so we don't have two separate concepts that do kinda the same thing but in different ways?

I've been wondering about this myself, fair question. Fallbacks and Aliases are certainly fairly similar, but there's differences:

• the fallback setting approach (using a separate setting instance) unfortunately doesn't work for AffixSettings as it is based on default value functions, which are not applicable in that case. That's the main reason I decided to follow @stu-elastic's approach for affix settings in Metrics: Agent settings prefix telemetry.agent preferred over tracing.apm.agent #104345 and generalize that.

• alias keys are rewritten in AbstractScopedSettings.updateDynamicSettings to ensure update consumers are triggered.

• Setting.exits doesn't consider a fallback, but it should consider an alias.

• Setting.get using primary and secondary settings (used by ClusterSettings.get(Setting<?>)) evaluates fallbacks in a different order.

A few thoughts without having an ultimate answer:

• The code required to support fallbacks is minimal, actually it's mostly just a default value function. The trouble arises from the high number of permutations of the various setting factories having both fallbacks and aliases.

• Fallback settings are often shared as default by various other more specialised settings. The current implementation captures that dependency in a more explicit way. With the aliasKey any such dependency would be fairly lose just based on repeated usage of the same alias key.

I share Simon's concerns about supporting both aliases and fallbacks. While it seems like more work to have a consistent, single concept, I fear adding yet another concept to settings is going to complicate them too much.

Some of the things you mention seem like bugs in the same way that fallbacks are not considered today for updates. For example:

Setting.exists doesn't consider a fallback, but it should consider an alias.

That seems like another bug. Fallbacks are supposed to be what you have framed as aliases. While they are clunky today (eg so many method variants to take fallbacks), we should find a way to make them work consistently (and maybe that means changing the concept from fallbacks to aliases, but not having both).

Setting.exists doesn't consider a fallback, but it should consider an alias.

That seems like another bug. Fallbacks are supposed to be what you have framed as aliases.

Interestingly that's one of the important differences and actually required so that fallbacks work as intended. Reasons is

public final T get(Settings primary, Settings secondary) {
    if (exists(primary)) {
        return get(primary);
    }
    if (exists(secondary)) {
        return get(secondary);
    }
    if (fallbackSetting == null) {
        return get(primary);
    }
    if (fallbackSetting.exists(primary)) {
        return fallbackSetting.get(primary);
    }
    return fallbackSetting.get(secondary);
}

which is called from ClusterSettings.get(Setting) using the last settings applied as primary and the initial node settings as secondary.

Another feature fallbacks support is a chain of fallbacks. I think I've seen at least one place using multiple levels of fallbacks.

With the above in mind I'll give this another look to see if I can actually do the opposite, implement the alias behavior on top of fallbacks. I expect that AffixSettings would remain different in that case (and continue using the "alias" behavior Stu implemented) but it might be possible to remove any such logic if rewriting setting keys both during dynamic updates (as currently done in this PR) and additional rewriting the initial node settings as well.

they are clunky today (eg so many method variants to take fallbacks)

The variants are definitely a hassle regardless if alias or fallback. I think it would greatly improve things if moving away from the current factory methods capturing everything in all the possible combinations to a more fluent configuration style starting off a few factories with a basic, common usage pattern. Anyways, that's a much simpler and separate thing to look at.

For simple & list settings this can be re-implemented on top of fallbacks without much effort.
The alias behavior would be triggered based on a new property Alias on the fallback.

When looking into affix settings I ran into #106284. Also, I wouldn't be surprised to find more bugs there.
IMHO, before making any changes on affix settings, we'd have to increase test coverage to better understand the status quo.

My proposal would be to:

• Implement aliases on top of fallbacks for simple and list settings. AffixSettings don't support the same fallback mechanism, so this doesn't apply there.
• Independently, improve test coverage for affix settings (particularly in combination with a fallback prefix) and fix related issues. Then re-evaluate next steps from there.

Any thoughts on above @thecoop and @rjernst ?

Do more complex settings still need to be kept separate between fallbacks and aliases? What's the plan to combine those at some point in the future, or does that need the more exhaustive tests first?

Do more complex settings still need to be kept separate between fallbacks and aliases?

• The fallback implementation of AffixSetting in main currently corresponds more closely to aliases than fallbacks (both, in terms of behavior and implementation). So yes & no, it's different, but only this single implementation exists (and will exist).
• GroupSetting doesn't support fallback (nor alias).

What's the plan to combine those at some point in the future, or does that need the more exhaustive tests first?

Most of all, the implementation is buggy and needs more through testing to identify the gaps first.
But as mentioned above, there's just a single implementation for AffixSetting.

That looks sensible to me - implementing one on top of the other, and increasing test coverage, should get us to a good solution

Closed, in favor of #106997