Maintenance: `undefined` environment variable returning empty string from `EnvironmentVariablesService`

[alan-churley]

Description of the feature request

Problem statement
Currently the EnvironmentVariablesService returns an empty string if an env var is not set, rather than undefined.

Ticket to discuss the pros and cons of this approach and consider changing the logic to use undefined

Summary of the feature

Code examples

Benefits for you and the wider AWS community

Describe alternatives you've considered

Additional context

Related issues, RFCs

This issue should be addressed after:
#484

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dreamorosi]

Would like to pick up again the discussion about this topic to see if we want to change the current behaviour or not. In case we decide to change I'm available to pick up the task.

The outcome of this discussion should be an agreement on whether EnvironmentVariablesService.get() should return an empty string (i.e. '') or undefined when a given environment variable is not set.

Context

The example below is from packages/logger/src/config/EnvironmentVariablesService.ts, but a similar implementation is found in Tracer and Metrics:

/**
 * Class EnvironmentVariablesService
 *
 * This class is used to return environment variables that are available in the runtime of
 * the current Lambda invocation.
 * These variables can be a mix of runtime environment variables set by AWS and
 * variables that can be set by the developer additionally.
 *
 * @class
 * @extends {ConfigService}
 * @see https://docs.aws.amazon.com/lambda/latest/dg/configuration-envvars.html#configuration-envvars-runtime
 * @see https://awslabs.github.io/aws-lambda-powertools-typescript/latest/#environment-variables
 */
class EnvironmentVariablesService extends ConfigService {

  // Reserved environment variables
  private xRayTraceIdVariable = '_X_AMZN_TRACE_ID';

  /**
   * It returns the value of an environment variable that has given name.
   *
   * @param {string} name
   * @returns {string}
   */
  public get(name: string): string {
    return process.env[name]?.trim() || '';
  }

  /**
   * It returns the value of the AWS_REGION environment variable.
   *
   * @returns {string}
   */
  public getAwsRegion(): string {
    return this.get(this.awsRegionVariable);
  }

  /**
   * It returns the value of the POWERTOOLS_SERVICE_NAME environment variable.
   *
   * @returns {string}
   */
  public getServiceName(): string {
    return this.get(this.serviceNameVariable); // <- this.serviceNameVariable is inherited from `ConfigService`
  }
}

EnvironmentVariablesService is used throughout the utilities to influence the behaviour of the instantiated utility.

Logger

For example (link):

/**
   * It adds important data to the Logger instance that will affect the content of all logs.
   *
   * @param {string} serviceName
   * @param {Environment} environment
   * @param {LogAttributes} persistentLogAttributes
   * @private
   * @returns {void}
   */
  private setPowertoolLogData(
    serviceName?: string,
    environment?: Environment,
    persistentLogAttributes: LogAttributes = {},
  ): void {
    this.addToPowertoolLogData(
      {
        awsRegion: this.getEnvVarsService().getAwsRegion(),
        environment:
          environment ||
          this.getCustomConfigService()?.getCurrentEnvironment() ||
          this.getEnvVarsService().getCurrentEnvironment(),
        sampleRateValue: this.getSampleRateValue(),
        serviceName:
          serviceName || this.getCustomConfigService()?.getServiceName() || this.getEnvVarsService().getServiceName() || Logger.defaultServiceName,
        xRayTraceId: this.getEnvVarsService().getXrayTraceId(),
      },
      persistentLogAttributes,
    );
  }

In the example above some keys might end up having an empty string as value, these keys are then filtered out before emitting the log (here) with the following logic:

pickBy(attributes, (value) => value !== undefined && value !== '' && value !== null);

Even if EnvironmentVariablesService.get() returned undefined instead of empty string this logic would stay as it is since it still must be able to filter out values that might come from elsewhere (i.e. user-provided).

Metrics

In Metrics this service is used less heavily and is used only to set service and namespace in packages/metrics/src/Metrics.ts:

private setNamespace(namespace: string | undefined): void {
    this.namespace = (namespace ||
      this.getCustomConfigService()?.getNamespace() ||
      this.getEnvVarsService().getNamespace()) as string;
  }

private setService(service: string | undefined): void {
    const targetService = (service ||
      this.getCustomConfigService()?.getService() ||
      this.getEnvVarsService().getService()) as string;
    if (targetService.length > 0) {
      this.addDimension('service', targetService);
    }
  }

In the case of setService() the if statement would change but remain similar in essence.

In the case of setNamespace() there would be no change except the type casting and the usage of this.namespace elsewhere would stay the same (below current implementation):

return {
      _aws: {
        Timestamp: new Date().getTime(),
        CloudWatchMetrics: [
          {
            Namespace: this.namespace || DEFAULT_NAMESPACE,
            Dimensions: [dimensionNames],
            Metrics: metricDefinitions,
          },
        ],
      },
      ...this.defaultDimensions,
      ...this.dimensions,
      ...metricValues,
      ...this.metadata,
    };

If this.namespace was undefined instead of empty string the this.namespace || DEFAULT_NAMESPACE would be evaluated the same way (DEFAULT_NAMESPACE is picked).

Tracer

In Tracer the EnvironmentVariablesService is used exclusively upon initialisation of the class to set configs and its values are always evaluated after more explicit configs (i.e. passed in constructor or custom config service). For example (here):

/**
   * Setter for `captureResponse` based on configuration passed and environment variables.
   * Used internally during initialization.
   */
  private setCaptureResponse(): void {
    const customConfigValue = this.getCustomConfigService()?.getTracingCaptureResponse();
    if (customConfigValue !== undefined && customConfigValue.toLowerCase() === 'false') {
      this.captureResponse = false;

      return;
    }

    const envVarsValue = this.getEnvVarsService()?.getTracingCaptureResponse();
    if (envVarsValue.toLowerCase() === 'false') {
      this.captureResponse = false;

      return;
    }
  }

If EnvironmentVariablesService returned undefined instead of empty string then the second if statement in the example above would have to include an additional envVarsValue !== undefined but other than that it would remain the same in essence.

To Sum Up

process.env.SOMETHING would return undefined if the SOMETHING environment variable is not set. In our implementation we are instead returning an empty string.

Do we want to leave the implementation as it is or change it and return the actual value? What would be the advantage in doing so?

At the moment I can't think of any good reason to not leave it as it is but I'd like to hear other opinions.

[ijemmy]

Thanks for the thorough context.

Thoughts:

1. I don't have a strong opinion on undefined vs. ''. To me, undefined is the correct choice. The environment variable may intentionally be ''.
2. I'm quite concerned on having 3 similar implementations with the same name in 3 packages instead of only one in common. If they are really different and should be in different packages, shouldn't they have their own names?

The concern stems from duplications and slightly different implementations. As these classes deviate over time (and we have more utilities), it can introduces bugs. undefined and '' is a good example. Contributors who worked on another utility may use the class with incorrect assumption.

[saragerion]

Assigning priority:medium to this issue as we should tackle #484 first.
Note that this issue is still open for triage.