Introduce behavior-change configs as an alternative to feature configs

[eric-maynard]

Polaris currently has the PolarisConfiguration type as a catch-all for behavior that can be altered via the application.properties. When I refactored it in #97 I imagined that we would only use these flags for "features" -- things a typical user will want to enable/disable depending on their deployment, and things that the project should support for a long time.

It's becoming apparent that we'll also want to use flags to protect behavior changes that aren't strictly features. For example, a bugfix that may cause a regression for users relying on buggy behavior, or a controversial change like the one in #490 where theoretical performance concerns could cause a regression.

To better support these cases, this PR refactors PolarisConfiguration into FeatureConfiguration and BehaviorChangeConfiguration. In the future, changes that are perceived to be high-risk or to have a high chance to cause a regression should be gated behind behavior change flags.

[eric-maynard]

It seems that there's some conflict between the two ConfigMappings that Quarkus is not able to resolve. Currently, tests that rely on configOverrides do not pass. If I comment out the new mapping, Quarkus tests pass as expected:

@StaticInitSafe
// @ConfigMapping(prefix = "polaris.behavior-changes")
public interface QuarkusBehaviorChangesConfiguration extends FeaturesConfiguration {

[eric-maynard]

For now I've found that duplicating code between FeatureConfiguration and QuarkusBehaviorChangeConfiguration allows Quarkus to parse the configs correctly, though ideally QuarkusBehaviorChangeConfiguration should just extend FeatureConfiguration. Added a comment to this effect.

[dimas-b]

Could we quickly discuss this in the community sync tomorrow?

[eric-maynard]

Good idea @dimas-b, let's plan to discuss there

[adutra]

My general feeling about FeatureConfiguration is that it's too schemaless. I don't think this PR addresses that, but I think it's something that needs to be addressed soon.

A typical configuration class in a Quarkus application would like like this:

@ConfigMapping(prefix = "polaris.feature")
interface FeatureConfiguration {
  boolean configX();
  Optional<String> configY();
  List<String> configZ();
}

However, we currently have this:

@ConfigMapping(prefix = "polaris.feature")
interface FeatureConfiguration {
  Map<String, String> defaults();
}

Where the map values are dynamically parsed as json, then each map entry is collated against the fields in PolarisConfiguration:

public class PolarisConfiguration<T> {
  public static final PolarisConfiguration<Boolean> CONFIG_X = ...
  public static final PolarisConfiguration<String> CONFIG_Y = ...
  public static final PolarisConfiguration<List<String>> CONFIG_Z = ...
}

This makes the code quite brittle, as it's not possible to statically compile it against an existing method like FeatureConfiguration.configX().

I understand that the goal is to be able to override things on different levels, by realm for instance – and I think that's a great idea. But there are other ways to achieve that while preserving a strongly-typed configuration mechanism.

Let's discuss this at the next meeting, but I would suggest to refactor this by grouping the configuration options into logical groups inside one or many configuration classes.

[eric-maynard]

@adutra I agree there. Actually, I am hoping that when we refactor FeatureConfiguration to make it more structured, we can also resolve the issue where QuarkusBehaviorChangeConfiguration and QuarkusFeatureConfiguration don't share a type. I also think it's a little odd that we have the logic sort of split between e.g. PolarisFeatureConfiguration and then QuarkusFeatureConfiguration, so perhaps we can fix that as well.

With all that being said, I see adding a new type of config as not strictly related to these changes. Potentially, we could establish the existence of these configs here and then later refactor the FeatureConfiguration types

[adutra]

I also think it's a little odd that we have the logic sort of split between e.g. PolarisFeatureConfiguration and then QuarkusFeatureConfiguration, so perhaps we can fix that as well.

That split is rather for technical reasons, since polaris-service-common does not depend on Quarkus at all. But I'm open to revisit that as well.

[eric-maynard]

since polaris-service-common does not depend on Quarkus at all. But I'm open to revisit that as well.

I see. Yeah, I am also open to revisiting that if it simplifies things.

For now, what do you think is the best way to proceed? Should we introduce the flags with something like this change, and then try to refactor PolarisConfiguration etc. after? I'm a little worried about trying to sort out the refactor now (especially something potentially controversial like adding a Quarkus dependency in polaris-sevice-common) since other PRs like #1068 that should be gated behind a flag will be blocked.

[collado-mike]

Do we really need different types to support this? I think probably adding a field to PolarisConfiguration would be sufficient

[eric-maynard]

Strictly speaking we don't need a new type, but I chose to add one because I think it's valuable to separate the actual config instances from one another. Besides just having them clearly separated & organized, it means the caller is forced to recognize which type they are relying on (e.g. BehaviorChangeConfiguration.UNSTABLE_FLAG vs FeatureConfiguration.ACTUAL_FEATURE). It also allows us to override e.g. catalogConfig which shouldn't be valid for these flags.

[collado-mike]

It also allows us to override e.g. catalogConfig which shouldn't be valid for these flags.

That seems reasonable, but given that we use a builder, can't we just enforce that at construction? I do like having the different config types housed in different namespaces (as in your examples BehaviorChangeConfiguration.UNSTABLE_FLAG vs FeatureConfiguration.ACTUAL_FEATURE)...

I guess I would just like to ensure that the call sites that are checking for config values don't need to know what type the configuration is at runtime. I can just imagine cases where someone accidentally references one type and the config field is another or someone tries to write some generic code that only works for one config type but not the other...

[eric-maynard]

checking for config values don't need to know what type the configuration is at runtime.

Oh, I think the opposite. We should want the caller to have to acknowledge when they write logic that depends on an unstable flag.

I can just imagine cases where someone accidentally references one type and the config field is another

Can you say more about this? Are you talking about a situation where the user misconfigures the application.properties?

[collado-mike]

I was thinking of cases where a person writes generic code like (as a very simplistic example)

public static <T> T wrapWithConfigCheck(PolarisConfiguration<Boolean> config, Supplier<T> code, T defaultVal) {
    if (configurationStore.getConfiguration(polarisCallCtx, config)) {
        return code.get();
    }
    return defaultVal;
}

You can imagine code like this being used to provide bean implementations, to gate certain API implementations, to guard new features, ... if the generic code becomes type-specific, or if it has to downcast to pass the configuration value on to a Consumer<BehaviorChangeConfiguration>, we could end up having to write that code twice - or possibly have to change everything to accept <? extends PolarisConfiguration>.

As I mentioned in the previous comment, I do like having BehaviorChangeConfiguration as a namespace, so that the feature gates and the risky fix changes are separated. But I think the code that works with PolarisConfiguration types should be agnostic to the existence of those subtypes.

[eric-maynard]

Looking at that example, I think it functions the way we should want it to.

I created a test with a consumer like this:

  private static class PolarisConfigurationConsumer {
      ...
      public <T> T consumeConfiguration(PolarisConfiguration<Boolean> config, Supplier<T> code, T defaultVal) {
          if (configurationStore.getConfiguration(polarisCallContext, config)) {
              return code.get();
          }
          return defaultVal;
      }
  }

Where it's used like:

  @Test
  public void testBehaviorAndFeatureConfigs() {
      ...
      FeatureConfiguration<Integer> featureConfig =
          PolarisConfiguration.<Integer>builder()
          ...
              .buildFeatureConfiguration();

      BehaviorChangeConfiguration<Boolean> behaviorChangeConfig =
          PolarisConfiguration.<Boolean>builder()
          ...
              .buildBehaviorChangeConfiguration();

      consumer.consumeConfiguration(behaviorChangeConfig, () -> 21, 22);
      consumer.consumeConfiguration(featureConfig, () -> 21, 22);
  }

As expected, that last line won't compile because the method expects a PolarisConfiguration<Boolean>. If featureConfig is changed to a FeatureConfiguration, everything works as expected. I'll commit the test in that state now, but let me know how we can adjust it to better suit the use case you had in mind.

[collado-mike]

That doesn't really address the case I was talking about, but it's fine. As there aren't any concrete issues right now, I'm happy to move forward. The minute I see someone write <? extends PolarisConfiguration>, though, I'm out ;)

I'll defer to @adutra's review above.

[flyrain]

Maybe we should be more explicit about how we are going to remove them. I'm not concerned about the removal of any configurations, more concerned about they are not removed. Any time may mean a long time.

[flyrain]

We should have some rules for removing them, while exceptions are allowed as well per request. For example, if community said we will keep one configuration for one more release, we should allow that. The default behavior would be to remove them after 1 or 2 releases.

[eric-maynard]

I think that's a good idea. I updated this comment to be more detailed about my proposal, but maybe there's an additional place (the contributing guidelines? Is there like a release guidelines?) where we can define this. Let me know what you think.

This is also just my initial proposal, and I think it's expected that there will be exceptions or changes to how we handle these flags.

[sfc-gh-ygu]

LGTM. Thanks @eric-maynard!