feat(parameters): AppConfigProvider

[shdq]

Description of your changes

Related issue #1177

How to verify this change

Related issues, RFCs

Issue number: #1177

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

• My changes meet the tenets criteria
• I have performed a self-review of my own code
• I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding changes to the examples
• My changes generate no new warnings
• The code coverage hasn't decreased
• I have added tests that prove my change is effective and works
• New and existing unit tests pass locally and in Github Actions
• Any dependent changes have been merged and published
• The PR title follows the conventional commit semantics

Breaking change checklist

• I have documented the migration process
• I have added, implemented necessary warnings (if it can live side by side)

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[shdq]

@dreamorosi I need help with types for AppConfigProvider.

TL;DR; BaseProvider get method name argument and options? argument which is optional. But name and two more arguments are required to retrieve a configuration.

To retrieve configuration two API two calls are necessary. First to StartConfigurationSession three properties are required:

1. ConfigurationProfileIdentifier – The configuration profile ID or the configuration profile name. This is name argument from the get method of BaseProvider.
2. ApplicationIdentifier – The application ID or the application name.
3. EnvironmentIdentifier – The environment ID or the environment name.

When you make the first call to API you get initial token that encapsulates all these fields. Then you make the next API call with the only one initial token to GetLatestConfiguration and get the actual configuration and new token for the next call.

I placed the last two properties into sdkOptions property of options.

interface AppConfigGetOptionsInterface
  extends Omit<GetOptionsInterface, 'sdkOptions'> {
  clientConfig?: AppConfigDataClientConfig
  sdkOptions: {
    application: string
    environment: string
  }
}

Right now, application and environment are required to create an instance of AppConfigProvider. I add Partial<AppConfigGetOptionsInterface> to BaseProvider get method or it would break BaseProvider tests. As the result, I had to provide default values to application and environment which is not correct.

It also makes sense to "reset" the token, when you receive SDK options to overwrite in get method. Or it will make an API call with the old token completely ignoring new options. That's why I opened an issue about logic.

Another way is not to require options for a new instance of the class, but raise an error for the first call of the get method if it was made without required SKD options for StartConfigurationSession.

Sorry for the delay with the changes. We can discuss it in discord to speed up the process.

[dreamorosi]

Hey Sergei, thanks for the message and please no need to apologise for the times, you're doing great - no complaints at all here 😄

Let me pull the code and play with it in my IDE. Before giving an answer I need to better understand it and also wrap my head around the AppConfig flow.

Doing this now, you can expect an answer in the next couple of hours tops.

[shdq]

Doing this now, you can expect an answer in the next couple of hours tops.

Thanks 👍
At the same time, I'll try to draw a diagram with the flow to help us figure this out.

[shdq]

I think this would fix the logic.

[dreamorosi]

Thanks for the diagram, I like the idea of being able to retrieve multiple configurations!

I have been playing with the SDK, reading the docs, and making a few tests and just like you proposed I think we should support this use case:

const provider = new AppConfigProvider( {
  sdkOptions: {
    application: 'TestApplication',
    environment: 'Lambda',
  }
});

const config = await provider.get('TestFeatureFlag');
const otherConfig = await provider.get('TestFreeForm');

Based on the way the SDK & service work, and as you also figured out, each session token is tied to a set of:

1. ApplicationIdentifier (for us application)
2. EnvironmentIdentifier (for us environment)
3. ConfigurationProfileIdentifier (for us name)

This means that while 1 & 2 stay constant, if we want to retrieve two different configurations we should request a new session token.

At least based on my tests, if you use a valid token requested for a configuration from the same application/environment pair to retrieve a second configuration, the operation will succeed but return an empty response. This is a little confusing, but I'm sure there are reasons as for why the service was implemented this way although I'll refrain from commenting on it since I don't know much about it.

However, for the purpose of our use case, this means that we need a new token for each set of application, environment, name.

I came up with an implementation that would allow this before seeing your diagram, however I'm open to discuss and converge on either.

AppConfigProvider.ts

class AppConfigProvider extends BaseProvider {
  public client: AppConfigDataClient;
  private application: string;
  private environment: string;
++  private identifiersTokens: Map<string, string> = new Map(); // TODO: find a better name
  private latestConfiguration: Uint8Array | undefined;
--  private token: string | undefined;

then, same file, but inside the _get method:

++ if (!this.identifiersTokens.has(name)) {
-- if (!this.token) {
...
-- this.token = session.InitialConfigurationToken;
++ if (!session.InitialConfigurationToken) throw new Error('Unable to retrieve the configuration token');
++ this.identifiersTokens.set(name, session.InitialConfigurationToken);
...
const getConfigurationCommand = new GetLatestConfigurationCommand({
--  ConfigurationToken: this.token,
++  ConfigurationToken: this.identifiersTokens.get(name),
});
...
-- this.token = response.NextPollConfigurationToken;
++ if (response.NextPollConfigurationToken) {
++  this.identifiersTokens.set(name, response.NextPollConfigurationToken);
++ } else {
++   // This will force a new session to be created in the next execution
++  this.identifiersTokens.delete(name);
++ }

See full method implementation below:

protected async _get(
    name: string,
    options?: AppConfigGetOptionsInterface
  ): Promise<Uint8Array | undefined> {
    /**
     * The new AppConfig APIs require two API calls to return the configuration
     * First we start the session and after that we retrieve the configuration
     * We need to store the token to use in the next execution
     **/
    if (!this.identifiersTokens.has(name)) {
      const sessionOptions: StartConfigurationSessionCommandInput = {
        ConfigurationProfileIdentifier: name,
        EnvironmentIdentifier: this.environment,
        ApplicationIdentifier: this.application,
      };

      if (options?.sdkOptions) {
        Object.assign(sessionOptions, options.sdkOptions);
      }

      const sessionCommand = new StartConfigurationSessionCommand(
        sessionOptions
      );

      const session = await this.client.send(sessionCommand);

      if (!session.InitialConfigurationToken) throw new Error('Unable to retrieve the configuration token');

      this.identifiersTokens.set(name, session.InitialConfigurationToken);
    }

    const getConfigurationCommand = new GetLatestConfigurationCommand({
      ConfigurationToken: this.identifiersTokens.get(name),
    });
    const response = await this.client.send(getConfigurationCommand);

    if (response.NextPollConfigurationToken) {
      this.identifiersTokens.set(name, response.NextPollConfigurationToken);
    } else {
      // This will force a new session to be created in the next execution
      this.identifiersTokens.delete(name);
    }

    const configuration = response.Configuration;

    if (configuration) {
      this.latestConfiguration = configuration;
    }

    return this.latestConfiguration;
  }

This logic is pretty similar to the one in your diagram, however it would allow us to handle & cache the tokens of multiple configurations at the same time. What do you think?

[dreamorosi]

I hope that my comments clarify all the open points we had. I realise that the communication/conversation is a bit all over the place due to the distributed nature of the comments, so if there's any point that we should discuss further, or that I have missed please don't hesitate to let me know.

To sum up:

• application and environment should be set by the user when instantiating the provider
  • application can be set via constructor argument OR via environment variable - if none is set we throw an error.
  • environment can be set exclusively via constructor argument and as such it's mandatory
• Each set of application, environment, and configuration requires its own session token. With the assumption that an instance of AppConfigProvider handles exclusively a pair of application and environment defined during instantiation, we can handle multiple config by keeping track of the configuration and token pairs.

[shdq]

This logic is pretty similar to the one in your diagram, however it would allow us to handle & cache the tokens of multiple configurations at the same time. What do you think?

I like it, I mentioned something like that in the python issue. But I see a potential problem with the latest configuration. Quote from the issue:

Another option is to use dict to store NextPollConfigurationToken tokens with name as the key. But it increases complexity, cause in this case you should also store last_returned_value for each token. I think flush and starting a new session is the better option.

Let's use your example:

const provider = new AppConfigProvider( {
  sdkOptions: {
    application: 'TestApplication',
    environment: 'Lambda',
  }
});

const config = await provider.get('TestFeatureFlag');

We got the configuration for TestFeatureFlag and saved the token in the map and this.latestConfiguration = "latest TestFeatureFlag configuration".

const otherConfig = await provider.get('TestFreeForm');

Now the map has two tokens and this.latestConfiguration = "latest TestFreeForm configuration".
Now, lets call the TestFeatureFlag again:

const config = await provider.get('TestFeatureFlag');

We hit the cache and used the token for TestFeatureFlag and since we already got the latest configuration API will return an empty response which lead to

if (configuration) {
      this.latestConfiguration = configuration;
    }

But latestConfiguration has the latest TestFreeForm configuration. I assume this behavior, but I can't test it.

At least based on my tests, if you use a valid token requested for a configuration from the same application/environment pair to retrieve a second configuration, the operation will succeed but return an empty response. This is a little confusing, but I'm sure there are reasons as for why the service was implemented this way although I'll refrain from commenting on it since I don't know much about it.

I think it is intended to reduce end-user costs because AWS charges for each API call and received configuration

With AWS AppConfig, you pay each time you request configuration data from AWS AppConfig via API calls, as well as each time your requesting target receives configuration data in response to that request.

A target refers to the host that receives the application configuration (this could be an Amazon EC2 instance, on-premises server, container, AWS Lambda function, mobile app, IoT device, etc.).

Configuration requests via API Calls cost $0.0000002 per configuration request
Configurations Received cost $0.0008 per configuration received

If this potential problem with the latest configuration is real, seems like we should store configurations as well as tokens. Or flush the token if it was called with a different name and start over with a new session. I think in this case the response won't be empty (didn't test it) if it was made with InitialToken.

What do you think?

[shdq]

Sorry for the lengthy discussion around that.

Thanks for the help and clarification. I appreciate that. I will adjust the implementation and write more tests for different use cases while we decide on the caching tokens.

[dreamorosi]

No, this is a great discussion and it's worth having it I think.

I realised that I might not have been very clear in my previous message, so I have recorded a short screen share where I explain my implementation idea & also show a debugger session to explain what happens at each point, I hope it helps:

2022-12-30.16-10-42.mp4

In regards to why this is needed, you are correct, the reason is mainly cost savings but also performances (which in the case of Lambda almost directly translates to additional cost savings since shorter execution = lower cost).

Adding the storage of the tokens would be a second layer of caching. The first one is the storage of the configurations, which as you pointed out, is needed and we are in fact doing.

If you check the implementation of BaseProvider.get (here) which is the caller of the _get method you are implementing, we are in fact checking the cache before hitting the API. This means that we'll call the _get method only if the config is not cached or is present in the cache but has expired (as defined by the maxAge param - default 5 seconds).

Given this first level of caching, we can assume that a number of retrievals will be covered by the main cache. However the function's environment lasts longer and the cache expires, or the user wants to fetch an updated version (via the forceFetch parameter), we need to be able to call the AppConfig API as quickly as possible, and this means that having the token we can do so with one less request.

[dreamorosi]

After our conversation on Discord I'd like to amend part of my understanding of how the AppConfig service works and acknowledge that you were correct in your understanding.

Looking at this section of the docs (which I think you were referring to), there's this statement:

Warning
The GetLatestConfiguration response includes a Configuration section that shows the configuration data. The Configuration section only appears if the system finds new or updated configuration data. If the system doesn't find new or updated configuration data, then the Configuration data is empty.

This means that regardless of our implementation / caching, if users want to call the API they need to be ready to handle empty responses in their code if the config hasn't changed in the meanwhile.

Additionally, there's another case that we haven't mentioned, but that doesn't affect the implementation: the response returns a NextPollIntervalInSeconds which can be tweaked when starting the session via the RequiredMinimumPollIntervalInSeconds parameter (minimum 15 seconds). If you poll too quickly the service will throw an error Request too early.

In the video I posted above, I mistakenly assumed that the last config was retrieved from the service, while in fact it must have been retrieved from the cache since it should have been empty. I was able to confirm this by re-running the same sample this time using forceFetch and confirm that there response was indeed empty since the config hadn't changed.

[dreamorosi]

After merging #1206 I'm having some issues with the types on the get & getMultiple methods in the specific providers and in relation to the overloads in the BaseProvider.

Not sure how to move ahead yet, but you probably will encounter the same type issues I think.

Fixed in #1214 (details in #1213)

[shdq]

Hey @dreamorosi! Some comments for the PR. If you have any questions, I'm happy to answer.

As I get it from our conversation, a user should handle the cases below:

• the too-early request (RequiredMinimumPollIntervalInSeconds)
• empty GetLatestConfigurationresponse when configuration didn't change since the last retrieval (I assume it tracked using tokens)
• BadRequestException when the token expires in 24 hours (unlikely with lambda)

So I return it as-is, so all the errors will be caught in BaseProvider.

For the utility function, I combined interfaces in getAppConfigCombinedInterface, since the function has one argument, but inside I need to provide options for both new AppConfigProvider(options) and DEFAULT_PROVIDERS.appconfig.get(name, options)

Future thoughts for the next cycles:

• If forceFetch option is provided, start a new configuration session and get the latest configuration anyway
• Configuration returns Uint8Array and the configuration itself can be in json format, so it may require a double transformation

[dreamorosi]

As I get it from our conversation, a user should handle the cases below:

• the too-early request (RequiredMinimumPollIntervalInSeconds)
• empty GetLatestConfigurationresponse when configuration didn't change since the last retrieval (I assume it tracked using tokens)
• BadRequestException when the token expires in 24 hours (unlikely with lambda)

So I return it as-is, so all the errors will be caught in BaseProvider.

Correct.

For the utility function, I combined interfaces in getAppConfigCombinedInterface, since the function has one argument, but inside I need to provide options for both new AppConfigProvider(options) and DEFAULT_PROVIDERS.appconfig.get(name, options)

I understand why this is necessary, however I'm on the fence about this.

My main question mark is on whether we should restrict the type to avoid passing clientConfig to the utility function, i.e.:

interface getAppConfigCombinedInterface
   extends Omit<AppConfigProviderOptions, 'clientConfig>,
   AppConfigGetOptionsInterface {}

The reason I suggest this is for coherence with the other providers. None of the other providers allows to customize the AWS SDK when using the utility functions. That should be a behavior (and a reason to use) for the class providers.

Future thoughts for the next cycles:

• If forceFetch option is provided, start a new configuration session and get the latest configuration anyway

That's an interesting proposal, I hadn't thought about that and I can see the value. I am however concerned about implicitly overloading the meaning of the forceFetch which across providers would normally mean "1/ skip cache, 2/ fetch value from API" while for this provider would mean "1/ skip cache, 2/ discard token, 3/ fetch value from API".

On the other hand, if someone wanted to have a break-glass solution to forcefully fetch the value no matter the cost / timeout values, there's no way.

I think it's worth discussing this, but I'd like to do so across all versions of Powertools, not just us.

• Configuration returns Uint8Array and the configuration itself can be in json format, so it may require a double transformation

Very true, I've been thinking about this a lot as well and I don't love it. However, given that (like the point above) it's a new behavior/feature, I'd like to discuss this with the larger Powertools ecosystem. I'm in the process of writing a RFC which I'll post at a later time, but my current stance is that this is not something that should be there in v1 of the Parameters utility and could come afterwards.

Basically I'd like to focus our limited resources/cycles on laying out solid foundations and then having people start using the library before adding/perfecting features.

Hope it makes sense.

[dreamorosi]

Looking great, I have one last comment/point about the getAppConfigCombinedInterface interface that I'd like to hash out.

After that I'm looking forward to merge this asap!

[shdq]

Basically I'd like to focus our limited resources/cycles on laying out solid foundations and then having people start using the library before adding/perfecting features.

Sure. I just want to let you know and save my thoughts for future discussions.