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: Logger - ability to clear state #2337

Open
2 tasks done
rcoundon opened this issue Apr 8, 2024 · 9 comments · May be fixed by #2408
Open
2 tasks done

Feature request: Logger - ability to clear state #2337

rcoundon opened this issue Apr 8, 2024 · 9 comments · May be fixed by #2408
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

Comments

@rcoundon
Copy link

rcoundon commented Apr 8, 2024

Use case

Hi - we're beginning to use Powertools Logger as a canonical logger. By that I mean we append various key/values to the logger as the lambda executes and then flush them to stdout by calling a write method e.g myLogger.info('canonical');

Chatting with @am29d and @dreamorosi on Discord, it was confirmed there's no way to clear the keys that were added using appendKeys(). This means that those keys remain present during the next warm lambda invocation.

Solution/User Experience

Providing a logger method such as clearState() that clears down any user appended keys would solve the problem.

Alternative solutions

The only other solution in user land is to keep track of the appended keys and clear them down using removeKeys

Acknowledgment

Future readers

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

@rcoundon rcoundon 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 Apr 8, 2024
Copy link

boring-cyborg bot commented Apr 8, 2024

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

@dreamorosi dreamorosi added logger This item relates to the Logger Utility confirmed The scope is clear, ready for implementation and removed triage This item has not been triaged by a maintainer, please wait labels Apr 8, 2024
@dreamorosi
Copy link
Contributor

Hi @rcoundon, thank you for opening this issue.

As mentioned on Discord, I think it’d make sense to have this convenience method, so I am going to put the item in our backlog so it can be picked up.

Just to manage expectations I think it’s unlikely it’ll make the cut for this week release, but if not, it’ll be included in the following one (~next week or the following one).

@rcoundon
Copy link
Author

rcoundon commented Apr 8, 2024

That's great, thanks again for accepting it and putting it into the backlog so quickly

@dreamorosi
Copy link
Contributor

I have started looking into this and I might have found a slight inconsistency or rather something that we should address as part of this work.

Right now internally we treat what we call "persistent attributes" and "ephemeral attributes" the same - aka they're placed in the same object. The description of these and the clearState feature is quite ambiguous about this, but even though internally they're the same, the actual current behavior is:

  1. customers can add attributes to a Logger instance in 4 ways: 1/ via the persistentLogAttributes constructor option at instance creation, 2/ via the addPersistentLogAttributes method at runtime, 3/ via the appendKeys method also at runtime, and 4/ via the setPersistentLogAttributes method at runtime - this last method replaces the pre existing values.
  2. when using the class method decorator or Middy.js middleware with the clearState option enabled we keep track of any attribute added during the invocation either via addPersistentLogAttributes or appendKeys and restore the initial state after the invocation. We do this by taking a snapshot of the attribute immediately before the handler is executed and then restoring it right after
  3. there is not equivalent of the behavior described in 2/, which is why this feature request was opened

Below a simplified pseudo-code representation of the current implementation:

// middleware AND decorator
const injectLambdaHandler = (logger: Logger) => {
  let initialAttributes = {};
  const before = () => {
    initialAttributes = logger.getAttributes();
  };

  const after = () => {
    logger.setPersistentLogAttributes(initialAttributes)
  };

  return {
    before,
    after,
  }
}

With the middleware or decorator above, the usage would be like this (showing only middleware but the decorator works the same):

const logger = new Logger({
  persistentLogAttributes: {
    accountId: '01234567891012'
  }
});

export const handler = middy(async (event: unknown) => {
  logger.appendKeys({
    event
  });

  logger.info('CANONICAL');
}).use(injectLambdaHandler(logger, { clearState: true }));

The result of this would be a canonical log that contains:

  • the accountId key with the same value for all invocations
  • the event key with a value corresponding to the payload for each invocation

The above is the behavior documented in the docs, which is fine. What is not included in the docs however is that this is also possible:

const logger = new Logger({
  persistentLogAttributes: {
    accountId: '01234567891012'
  }
});

export const handler = middy(async (event: unknown) => {
  logger.appendKeys({
    event
  });

  if (condition) {
    logger.addPersistentLogAttributes({ foo: 'bar' });
  }

  logger.info('CANONICAL');
}).use(injectLambdaHandler(logger, { clearState: true }));

Because of the names of the methods, and especially the "persistent" word, I would expect the foo key added via the corresponding method to not be cleared after the invocation. This however is not true and instead all keys added during the invocation are cleared regardless of the method used.

Based on the implementation and meaning of the names, I think this should not be the case and persistent attributes should be treated as such and thus only cleared after customers remove them explicitly (i.e. removeKeys).

To this end, I propose to consider the current behavior as unintended effect (aka bug) and rectify the implementation so that both the new clearState method and the existing corresponding option in the middleware and decorator would only clear the keys added via the appendKeys.

Additionally, I would also:

  • mark the setPersistentLogAttributes method as deprecated and to be removed in the next major version.
  • mark the persistentLogAttributes constructor option and addPersistentLogAttributes method as deprecated and add respectively a persistentKeys option and appendPersistentKeys method - this would be just a renaming for consistency and the old naming ones would be removed in the next major version
  • persistent keys would be removable only by calling the removeKeys method
  • improve the documentation to clarify the behaviors described above

@heitorlessa
Copy link
Contributor

commenting later today :)

@heitorlessa
Copy link
Contributor

Great discussion and a nice improvement to reduce cognitive overload to everyone! Answering in line, with only one suggestion and one caveat on deprecation method.

mark the setPersistentLogAttributes method as deprecated and to be removed in the next major version.

+1, I’d suggest deprecating now but removing in the 2nd major version (e.g., 4.x) to give ample time. Logger is widely used in nearly if not all customers.

mark the persistentLogAttributes constructor option and addPersistentLogAttributes method as deprecated
+1.

and add respectively a persistentKeys option
and appendPersistentKeys method
this would be just a renaming for consistency and the old naming ones would be removed in the next major version
persistentLogAttrs > persistentKeys

If you allow me, I'd like to suggest a different approach:

  1. Keep only the constructor level. Give one option to customers to persistent data across state flushes. It'll be less confusing and aligned with other languages.
  2. Create a new feature request. Since the method already exists but it's undocumented, we can gauge solid use cases for having a method to persist data across state flushes.

To the best of my knowledge, customers persist keys across state in two ways only: (1) Logger Constructor for simple company standard use cases, and (2) Logger Custom Formatter for complex and dynamic company standards. This means whoever set those standards have access to the constructor and often export an instance of a Logger. Whoever uses it, can still use appendKeys / removeKeys as part of the transaction if they wish to have the key as early as possible.

improve the documentation to clarify the behaviors described above

+1. Keeping one option for persisting across state will also simplify the documentation and make it less ambiguous.

@dreamorosi
Copy link
Contributor

Thank you for weighing in Heitor, appreciate it.

Let me recap what actions need to be taken so that contributors or maintainers can pick up the issue:

  • mark the setPersistentLogAttributes method as deprecated
  • mark the persistentLogAttributes constructor option as deprecated
    • add a new persistentKeys constructor option that replaces the persistentLogAttributes above - to do this extend the init logic for Logger so that both values are checked. The options should be typed so that they're mutually exclusive, and unit tests should be added
  • change internal logic so that persistent attributes and temporary attributes are handled separately
  • add new clearState method that clears any of the temporal attributes - aka they ones added via appendKeys but not the ones marked as persistent
  • make sure that both types of attributes are propagated when creating a child logger

I'm going to mark this issue as help-wanted since its scope is now fairly defined. If anyone is interested in picking it up, please leave a comment here.

@dreamorosi dreamorosi added the help-wanted We would really appreciate some support from community for this one label Apr 15, 2024
@shdq
Copy link
Contributor

shdq commented Apr 15, 2024

I would like to pick this one up. Thanks for the clear description of the issue.

@dreamorosi dreamorosi removed the help-wanted We would really appreciate some support from community for this one label Apr 15, 2024
@dreamorosi
Copy link
Contributor

Great to see you again @shdq, we're here or on Discord if you need anything!

@shdq shdq linked a pull request Apr 22, 2024 that will close this issue
9 tasks
@dreamorosi dreamorosi linked a pull request Apr 22, 2024 that will close this issue
9 tasks
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
Status: Working on it
Development

Successfully merging a pull request may close this issue.

4 participants