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: allow custom formatter to formatAttributes on all attributes (not only the base attributes) #1261

Closed
2 tasks done
ghdoergeloh opened this issue Jan 25, 2023 · 11 comments
Assignees
Labels
confirmed The scope is clear, ready for implementation feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility
Milestone

Comments

@ghdoergeloh
Copy link

ghdoergeloh commented Jan 25, 2023

Use case

In my use case there is a compliance that enforces the developer to put some basic attributes in the root level of a log json. All the other additional data like a error or other data to identify the reason or context of that log should be placed under a attribute "data".

Since the formatAttributes function of a Formatter only gets the unformattedBaseAttributes it is not possible to sort or format the other attributes.

Solution/User Experience

I would like to pass any attribute to the logs and enforce a nice ordering of the log in the formatter:

export class MyLogFormatter extends LogFormatter {
  public formatAttributes(attributes: UnformattedAttributes): LogAttributes {
    const {
      environment,
      serviceName,
      sampleRateValue,
      lambdaContext,
      xRayTraceId,
      awsRegion,
      logLevel,
      timestamp,
      message,
      ...data
    } = attributes;
    return {
      id: randomUUID(),
      timestamp: timestamp.getTime(),
      logLevel: logLevel,
      serviceName: serviceName,
      message: message,
      xrayTraceId: xRayTraceId,
      requestId: lambdaContext?.awsRequestId,
      data,
    };
  }
}

export const logger = new Logger({
  logFormatter: new MyLogFormatter(),
});

logger.error('This is my message', { error: new Error('something bad happened'), contextInfo: 1234 })

which should then result in a object like this:

  {
      "id": "...",
      "timestamp": "...",
      "logLevel": "error",
      "serviceName": "...",
      "message": "This is my message",
      "xrayTraceId": "...",
      "requestId": "...",
      "data": {
         "error": { "name":"...", "message": "..."},
         "contextInfo": 1234
    }

Alternative solutions

My current workaround is to extend the Logger Class and do the sorting in the debug,info,warn and error method:

interface LogAttributesWithMessageAndData extends LogAttributesWithMessage {
  data?: LogAttributes;
}

export class MyLogger extends Logger {
  public override debug(input: LogItemMessage, ...extraInput: LogItemExtraInput): void {
    super.debug(this.convertLogAttributes(input, extraInput));
  }

  public override info(input: LogItemMessage, ...extraInput: LogItemExtraInput): void {
    super.info(this.convertLogAttributes(input, extraInput));
  }

  public override warn(input: LogItemMessage, ...extraInput: LogItemExtraInput): void {
    super.warn(this.convertLogAttributes(input, extraInput));
  }

  public override error(input: LogItemMessage, ...extraInput: LogItemExtraInput): void {
    super.error(this.convertLogAttributes(input, extraInput));
  }


  private convertLogAttributes(input: LogItemMessage, extraInput: LogItemExtraInput): LogAttributesWithMessage {
    const attributes: LogAttributesWithMessageAndData = {
      message: '',
      data: {},
    };
    if (typeof input === 'string') {
      attributes.message = input;
    } else {
      const { message, ...data } = input;
      attributes.message = message;
      attributes.data = data;
    }
    extraInput.forEach((item) => {
      const dataAttributes: LogAttributes =
        item instanceof Error ? { error: item } : typeof item === 'string' ? { extra: item } : item;
      merge(attributes.data, dataAttributes);
    });

    if (attributes.data && !Object.keys(attributes.data).length) {
      delete attributes.data;
    }

    return attributes;
  }
}

export const logger = new MyLogger({
  logFormatter: new MyLogFormatter(),
});

logger.error('This is my message', { error: new Error('something bad happened'), contextInfo: 1234 })

Acknowledgment

@ghdoergeloh ghdoergeloh added triage This item has not been triaged by a maintainer, please wait feature-request This item refers to a feature request for an existing or new utility labels Jan 25, 2023
@saragerion
Copy link
Contributor

saragerion commented Jan 27, 2023

Hi @ghdoergeloh, thanks for opening this issue.
I agree that this will be an improvement of the way the current logger works. We'll look into this possibility and get back to you with a more detailed perspective.

@saragerion saragerion added logger This item relates to the Logger Utility enhancement PRs that introduce minor changes, usually to existing features labels Jan 27, 2023
@ghdoergeloh

This comment was marked as resolved.

@dreamorosi

This comment was marked as resolved.

@ghdoergeloh

This comment was marked as resolved.

@dreamorosi dreamorosi added discussing The issue needs to be discussed, elaborated, or refined and removed enhancement PRs that introduce minor changes, usually to existing features triage This item has not been triaged by a maintainer, please wait labels Feb 7, 2023
@dreamorosi
Copy link
Contributor

dreamorosi commented Feb 9, 2023

Thank you for opening this feature request.

One of the key value propositions of the Logger utility is to allow customers to bring their own formatter so that they can comply with their unique requirements, so I definitely see value in your proposal.

However I'd like to reframe/rephrase the feature request slightly.

Context

At the moment, the logic in charge of formatting attributes resides within the private method called createAndPopulateLogItem, which as you noticed, is called before a log is emitted.

By default, Logger comes with an opinionated LogFormatter, however customers can provide a custom one, which is expected to implement the formatAttributes method. This method is currently called only on the standard attributes and context-related ones if present.

As you pointed out, any attribute present in the extraInput is excluded from the formatting. This is true also for any other persistent attributes added to the logger.

The method was designed as such because the formatting done in the PowertoolLogFormatter is largely manual/hardcoded and aimed purely at standardizing the names of certain fields that are always guaranteed to exist and have stable names. All other fields (extraInput and persistentAttributes) are user-provided at runtime and as such we have decided to leave them as they are provided.

Proposed rephrasing/refocus

With this in mind, I think that the path forward should be to consider adding to custom log formatters, aka log formatters that extend the LogFormatter class and that are provided to the Logger at instantiation time via the logFormatter option, the ability to format also attributes provided as extraInput and persistentAttributes.

Proposed solutions / DX

Note
Ideally I would like to address this in a backward-compatible way and avoid a breaking change. However, I'm not sure this is possible so let's not limit ourselves and add this to the already ongoing discussion for Powertools v2.

The formatAttributes method currently has the following signature:

formatAttributes(attributes: UnformattedAttributes): LogAttributes

As discussed for now we are only passing some attributes to it, and excluding any extraInput and persistentAttributes.

Option 1

To give full control to a custom log formatter, all three types of attributes should be passed to it, which would require to change the method signature to something like:

formatAttributes(attributes: UnformattedAttributes, persistentLogAttributes: LogAttributes, extraInput: LogItemExtraInput): LogAttributes

This would allow us to move some of the some of the logic currently in the createAndPopulateLogItem method into PowertoolLogger, as well as potentially simplify the merging of these attributes.

At the same time, and more interestingly, this would give full control to users extending the LogFormatter class to truly apply any formatting and any sorting they like.

This is how Powertools for Python currently behaves, however changing out library's logger now would represent a breaking change and will require us to wait for a major version release.

Option 2

A non-breaking way of allowing some type of formatting, but not sorting would be to extend theLogFormatter interface and add two methods like:

formatPersistentAttributes(persistentLogAttributes: LogAttributes): void

formatExtraAttributes(extraAttributes: LogAttributes): void

which could then optionally overloaded by customers wanting to format one or the other group of attributes. These two methods would be called as part of the createAndPopulateLogItem chain.

While this could be a decent stop-gap solution, I'm not convinced it's the best way forward for the long term and in line with the key value proposition of Logger as an utility - which is to provide full customization of the log format.


What do you think? Would appreciate people's opinion on this + additional / alternative proposals.

@dreamorosi dreamorosi changed the title Feature request: run formatAttributes on all attributes (not only the base attributes) Feature request: allow custom formatter to formatAttributes on all attributes (not only the base attributes) Feb 9, 2023
@ghdoergeloh
Copy link
Author

I do also think that your first solution would be the best way. But if you decide for the second option, you could think about "piping" the log object through these methods and each method can use the existing log object from the earlier called method.

So you would call formatAttributes, take the result an push it together with the persistent attributes to the formatPersistentAttributes method. So you would be able to format and also to sort the attributes in your message object. Same with the formatExtraAttributes.

@quintinchris
Copy link

definitely +1 this feature - as part of a team that just refactored our custom logger to use the powertools logger, i think the expectation with a custom log formatter is that it will format all the attributes within a log item, not just the base or persistent attributes.

also, a potential solution - and apologies if this is naive, still learning the codebase - couldn't you simply call formatAttributes on all the attributes, then set the attributes of the logItem to the formatted attributes?

so within createAndPopulateLogItem, before returning the logItem, you could do something like:

const formattedAttributes = this.getLogFormatter().formatAttributes(logItem.getAttributes());
logItem.setAttributes(formattedAttributes);

as far as i can tell, this should still give complete control to the custom LogFormatter, without introducing a breaking change.

let me know what you all think! i'm happy to help with the implementation of a solution regardless which direction we go 🙂

@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Apr 13, 2023
@dreamorosi
Copy link
Contributor

Hi @quintinchris, thank you for adding to this issue, and also for offering to help!

The solution you propose could work, however the challenge that we are having is due to a decision that was made on our side early on to exposing to the consumer (a.k.a. developers implementing custom log formatters) only a subset of the attributes.

If I'm understanding correctly the suggestion, this would mean that the signature of the formatAttributes attribute method would still need to change to allow not only the current UnformattedAttributes type, but UnformattedAttributes | any (or something along these lines).

This, while patching the problem in the short term, would mean asking developers to handle this unknown type while implementing their version of formatAttributes, making it needlessly complex.


With that said, I want to share that I am adding the feature to our backlog as we are definitely going to implement a version of this (hopefully with help & inputs from the community).

We are currently discussing supporting other observability providers other than CloudWatch. This is still a early stages discussion that is happening in the form of an RFC in our sister project Powertools for Python (aws-powertools/powertools-lambda-python#2014) and that will be ported to this version as well. The main gist of the proposal is that we are going to add support to other providers by extending the LogFormatter by giving it access to all attributes and provide a set of LogFormatters aimed at specific providers (i.e. DatadogLogFormatter or similar).

Implementing this issue specifically, will pave the way for that other feature so we will address it sooner rather than later.

At the moment I don't have an ETA to share, mainly because this feature is included in the version 2 planning (since it's a breaking change), but I should be able to give one in the coming weeks.

@dreamorosi
Copy link
Contributor

Hi everyone, we have started working on a RFC related this topic.

If you're interested in reading the current proposal and leaving your feedback please do so at #1500.

@dreamorosi
Copy link
Contributor

The PR for the RFC mentioned above has been merged several weeks ago. We are actively working on the next major release and plan on start publishing public alpha releases soon.

You can follow the progress of the release here: #1714

@dreamorosi dreamorosi self-assigned this Sep 26, 2023
@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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 logger This item relates to the Logger Utility
Projects
Development

No branches or pull requests

4 participants