Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Provide ability to refernce and modify Log Level Numerical Thresholds #2525

Open
2 tasks done
OffensiveBias-08-145 opened this issue May 14, 2024 · 3 comments
Labels
confirmed The scope is clear, ready for implementation feature-request This item refers to a feature request for an existing or new utility help-wanted We would really appreciate some support from community for this one logger This item relates to the Logger Utility

Comments

@OffensiveBias-08-145
Copy link

OffensiveBias-08-145 commented May 14, 2024

Use case

Currently our logging standard has both the numerical and string representations of the Log Level. The usage of both allow us to have increased efficiency when retrieving or searching logs from storage.

Indexing the numerical value is more efficient that its string counterpart.

{
  level: 'INFO',
  level_index: 12,
}

Currently it is difficult to directly access the prescribed log levels when extending the Logger class or utilizing a custom log format.

Some of the workarounds include:

  • Extending UnformattedAttributes to include the LogLevel Numerical Value (Works but adds another custom interface I then need to manage)
  • Manually mapping LogLevel (str) to its corresponding numerical value (Works but extra code is required)
    • With TS, this also means that we also have to force uppercase matching (Less LOC than a switch) since LogLevel used in UnformattedAttributes allows for upper and lowercase values.'
    • Since one cannot directly access the perscribed LogLevel thresholds, one must copy it manually and later update it should it change.
  class CustomLogFormatter extends LogFormatter {
    private getLogLevelNumberFromName(level: LogLevel): number {
      let found = 0;
      for (const [key, value] of Object.entries({
        DEBUG: 8,
        INFO: 12,
        WARN: 16,
        ERROR: 20,
        CRITICAL: 24,
        SILENT: 28,
      })) {
        if (key.toUpperCase() === level) {
          found = value;
          break;
        }
      }
      return found;
    }

    override formatAttributes(
      attributes: UnformattedAttributes,
      additionalLogAttributes: LogAttributes
    ): LogItem {
      const baseAttributes = {
        timestamp: this.formatTimestamp(new Date()),
        level: attributes.logLevel.toUpperCase(),
        level_index: this.getLogLevelNumberFromName(attributes.logLevel),
  
     //........

If I am missing something evident please let me know.
I would be happy to open an PR for this.

Solution/User Experience

Ideal Solutions:

  • logLevelThresholds changed from private to protected OR utilize a "getter"
  • Add an additional property to UnformattedAttributes or PowertoolsLogData that represents the numerical LogLevel.

This way the numerical log level is easily passed to any Custom log Formatter instead of having to map it using LogAttributes or some other workaround.

Adding this in the other libraries would be beneficial as well!

//Open to feedback on the prop name

type PowertoolsLogData = LogAttributes & {
    environment?: Environment;
    serviceName: string;
    sampleRateValue: number;
    lambdaContext?: LambdaFunctionContext;
    xRayTraceId?: string;
    awsRegion: string;
    logLevelIndex?: number; //OPTION #1 
};
type UnformattedAttributes = PowertoolsLogData & {
    error?: Error;
    logLevel: LogLevel;
    logLevelIndex: number; //OPTION #2
    timestamp: Date;
    message: string;
};

Alternative solutions

Creating a helper function in a Custom Log Formatter to map the string LogLevel to its numerical counterpart.

  class CustomLogFormatter extends LogFormatter {
    private getLogLevelNumberFromName(level: LogLevel): number {
      let found;
      for (const [key, value] of Object.entries({
        DEBUG: 8,
        INFO: 12,
        WARN: 16,
        ERROR: 20,
        CRITICAL: 24,
        SILENT: 28,
      })) {
        if (key.toUpperCase() === level) {
          found = value;
          break;
        }
      }
      return found ?? 0;
    }

    override formatAttributes(
      attributes: UnformattedAttributes,
      additionalLogAttributes: LogAttributes
    ): LogItem {
      const baseAttributes = {
        timestamp: this.formatTimestamp(new Date()),
        level: attributes.logLevel.toUpperCase(),
        level_index: this.getLogLevelNumberFromName(attributes.logLevel),
  
     //........

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@OffensiveBias-08-145 OffensiveBias-08-145 added feature-request This item refers to a feature request for an existing or new utility triage This item has not been triaged by a maintainer, please wait labels May 14, 2024
@dreamorosi
Copy link
Contributor

Hi @OffensiveBias-08-145, thank you for taking the time to open this issue and for writing this thoughtful proposal.

I see the value in supporting this use case and I fully agree on the direction of making this an opt-in feature, however I am still not sure on what would be the best way to do this in a backwards compatible way.

To help me understand better your proposal, could you please clarify if the ideal solutions are meant to be taken together or only one of them?

I'm asking because changing the logLevelThresholds to protected is a very low hanging fruit, so if that's all you'd need to enable this I'd be open to review & merge a PR fairly quickly.

If this is the case, could you share an example of how you'd use it in practice, if it were protected? I guess you're thinking of extending the Logger, but I'm not sure how you'd propagate it to the LogFormatter.

Regarding the other option of adding logLevelIndex to the attributes passed to the formatAttribute function, I think it's a sensible proposal, but one that requires a bit more consideration. It could make sense to pass the value, however I'm not sure if it'll impact existing customers since we don't know how they're handing the attributes in the formatAttribute.

@dreamorosi dreamorosi added logger This item relates to the Logger Utility need-customer-feedback Requires more customers feedback before making or revisiting a decision need-more-information Requires more information before making any calls and removed triage This item has not been triaged by a maintainer, please wait labels May 15, 2024
@OffensiveBias-08-145
Copy link
Author

@dreamorosi I can see how the issue is slightly confusing.
The issue template made more sense in my head when I was doing it.

Hopefully the below provides clarity??

Potential ideal solutions:

In an ideal world I would love to see both 1 and 2 added

1. Change logLevelThresholds to protected and correctly type it to allow the record to be indigestible.

This would not be propgated to LogFormatter. It would only be used for ingestion elsewhere or in other libraries. (Ex: Aligning OpenTelemetry and this logging lib)

EXAMPLE:

//=== UPDATED CLASS PROPERTY ===
class Logger extends Utility implements LoggerInterface {
  //...
  protected readonly logLevelThresholds: Record<Uppercase<LogLevel>, number>;
  //...
}




//=== EXAMPLE USAGE WITH AN EXTENDED LOGGER ===

  export class CustomLogger extends PowertoolsLogger {

    constructor(options: ConstructorOptions = {}) {
      //Override any accidentally passed custom log formatter
      const {logFormatter, rest} = options;
      super({logFormatter: new CustomLogFormatter(), ...rest});
    }

    //getter could also be addeded to the lib itself. 
    public static get logLevelThresholds(): Record<Uppercase<LogLevel>, number> {
      return this.logLevelThresholds;
    }
  }
 //...
}

// === EXAMPLE INGESTION INTO EXTERNAL METHODS ===

    private getLogLevelNumberFromName(level: LogLevel): number {
      let found = 0;
      for (const [key, value] of Object.entries(
        Logger.logLevelThresholds)) {
        if (key.toUpperCase() === level) {
          found = value;
          break;
        }
      }
      return found;
    }

2. Add an optional logLevelIndex to the UnformattedAttributes OR PowertoolsLogData types.

As an optional, this would allow devs the discretion to add the numerical level value to their logs.

By default it can go unused, and if devs want to have it, they can access it by using a custom formatter and using the property itself.

//AN EXAMPLE USAGE WOULD BE AS FOLLOWS:



// === UPDATED TYPES ===
type PowertoolsLogData = LogAttributes & {
    environment?: Environment;
    serviceName: string;
    sampleRateValue: number;
    lambdaContext?: LambdaFunctionContext;
    xRayTraceId?: string;
    awsRegion: string;
};
type UnformattedAttributes = PowertoolsLogData & {
    error?: Error;
    logLevel: LogLevel;
    logLevelIndex?: number; //<-- Could also be moved to PowertoolsLogData
    timestamp: Date;
    message: string;
};


// === USAGE IN A CUSTOM FORMATTER ===

    override formatAttributes(
      attributes: UnformattedAttributes,
      additionalLogAttributes: LogAttributes
    ): LogItem {
      const baseAttributes = {
        timestamp: this.formatTimestamp(attributes.timestamp),
        time: attributes.timestamp.getTime(),
        level: attributes.logLevel.toUpperCase(),
        level_index: attributes.logLevelIndex, //<-- index passed in custom formatter
        message: attributes.message,
        error: attributes.error ? this.formatError(attributes.error) : null,
        //....
        },
      };

      const logItem = new LogItem({attributes: baseAttributes});
      logItem.addAttributes({_data: additionalLogAttributes});
      return logItem;
    }

@dreamorosi dreamorosi removed need-customer-feedback Requires more customers feedback before making or revisiting a decision need-more-information Requires more information before making any calls labels Jul 31, 2024
@dreamorosi
Copy link
Contributor

dreamorosi commented Jul 31, 2024

Apologies for the delayed response, and thank you for the explanation.

I think there's a third option that would allow you to do the same thing with much less code, and without adding any significant change to the public API.

In #2787 we added a new constant for LogLevel that customers can import in their code like this: import { LogLevel } from '@aws-lambda-powertools/logger';. This creates a nice precedent for doing the same but with log level thresholds.

At the moment the map of thresholds resides within the Logger class, but that's the only place it's used and as shown by this issue, it's needlessly hidden within the innards of the Logger.

I suggest we extract the LoggerThresholds object from there and instead put it in a place where it can be exported directly. This would allow you to do what you're trying to achieve just with a custom log formatter:

import { Logger, LogFormatter, LogItem, LogLevelThreshold } from '@aws-lambda-powertools/logger';
import type { LogAttributes, LogLevel, UnformattedAttributes } from '@aws-lambda-powertools/logger/types';

class CustomLogFormatter extends LogFormatter {
  public formatAttributes(
    attributes: UnformattedAttributes,
    additionalLogAttributes: LogAttributes
  ): LogItem {
    return new LogItem({
      attributes: {
        ...attributes,
        logLevel:
          LogLevelThreshold[
            attributes.logLevel.toUpperCase() as Uppercase<LogLevel>
          ],
      },
    }).addAttributes(additionalLogAttributes);
  }
}

const logger = new Logger({
  logLevel: 'info',
  logFormatter: new CustomLogFormatter(),
});

export const handler = () => {
  logger.info('Hello, World!');
};

/**
 * Output:
 * {
 *   "logLevel": 12,
 *   "timestamp": "2024-07-31T08:48:29.157Z",
 *   "message": "Hello, World!",
 *   "serviceName": "service_undefined",
 *   "sampleRateValue": 0
 *  }
 */

I'm going to put this item on the backlog and mark it as open for contributions by adding the help-wanted label.

The changes needed for this task would be roughly the following:

  • Remove LogLevelThresholds object from Logger class here
  • Add & export LogLevelThreshold (notice that it's without s at the end) in the constants file here
  • Add the new constant to the main barrel file
  • In the Logger class, replace usage of this.logLevelThresholds with the imported LogLevelThreshold (example)
  • Remove LogLevelThresholds type from here, as well as its export here
  • Remove any LogLevelThresholds mention from the unit tests in this file

Note

For those interested in contributing, please leave a comment below so that we can assign the issue to you and make sure we don't duplicate efforts. Also, if you have any further questions please don't hesitate to ask here or on our Discord channel.

@dreamorosi dreamorosi added help-wanted We would really appreciate some support from community for this one confirmed The scope is clear, ready for implementation labels Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed The scope is clear, ready for implementation feature-request This item refers to a feature request for an existing or new utility help-wanted We would really appreciate some support from community for this one logger This item relates to the Logger Utility
Projects
Development

No branches or pull requests

2 participants