powertools-lambda-typescript icon indicating copy to clipboard operation
powertools-lambda-typescript copied to clipboard

Feature request: Enable reuse across code files/modules

Open humanzz opened this issue 2 years ago • 6 comments

Description of the feature request

Problem statement

Powertools should make it easy to implement logging, metrics and tracing for code bases that are split across multiple files and modules. This is in contrast to simplistic examples that have all the example logic needed in a single handler file.

Currently, logger, tracer, and metrics examples all show instantiation in the handler file. It's not clear, from documentation, how developers should handle logging, metrics and tracing in other files

  1. Should different instances be instantiated as needed?
  2. Should the handler-instantiated instances be passed passed around for usage by different pieces of code?

Some example use cases where the current implementation makes them unnecessarily difficult - or not clear - to implement

  1. All log lines - regardless of the originating file - should log some custom attribute (e.g. some field from a request or a header)
  2. All metrics - regardless of the originating file - should log come custom attribute as a property

Additional context

  • Powertools for Java solve these problems
    • For logging, it's built on top of log4j which allows global settings to be applied. It also introduces a LoggingUtils class which can be accessed anywhere in code to manipulate some feature of logging e.g. https://awslabs.github.io/aws-lambda-powertools-java/core/logging/#appending-additional-keys
    • For metrics, they also introduce MetricsUtils class which can be accessed anywhere in code.
  • I found https://github.com/awslabs/aws-lambda-powertools-typescript/pull/767 which seems to be trying to solve an issue of the same theme but only for tracing

Solution(s) I'm not proposing a specific solution, but I'd like for the team to think about it. It seems like Java Powertools and the Tracer PR mentioned above do leverage some variation of singleton so this might be worth exploring more.

Off the top of my head, and based on the logger feature parity intended in https://github.com/awslabs/aws-lambda-powertools-typescript/issues/482, I don't feel that a simple singleton is the solution, but maybe a singleton root logger, and then child loggers for other files/modules https://awslabs.github.io/aws-lambda-powertools-typescript/latest/core/logger/#using-multiple-logger-instances-across-your-code.

In an example application I've started testing Powertools in, I introduced a powertools.ts file which instantiates/exports logger, metrics, tracer. My handler file imports those instances and configures the handler with the relevant middy middleware. Other files import those instances as needed as well (this is essentially me introducing the singletons).

humanzz avatar Apr 17 '22 18:04 humanzz

Thanks for the issue. DX is high priority for us and we appreciate this kind of feedback.

Powertools should make it easy to implement logging, metrics and tracing for code bases that are split across multiple files and modules. This is in contrast to simplistic examples that have all the example logic needed in a single handler file.

We had a discussion about this. We're considering scraping the singleton implementation for tracing (#767 ). Instead of enforcing this (potentially dangerous) pattern, it would be better to let user control this (like introducing powertools.ts as you suggested (example). I think that's a better fit for TypeScript than having another Util class.

Our examples mislead people to think that we should instantiate one utility per handler. I'm thinking about these two changes:

  1. Update the doc to promote this pattern. We can have a section for Recommend practices for topics like this and link to it from the main utility docs.
  2. Update code in the examples folder to follow the powertools.ts style.

What do you think?

Some example use cases where the current implementation makes them unnecessarily difficult - or not clear - to implement

  1. All log lines - regardless of the originating file - should log some custom attribute (e.g. some field from a request or a header)
  2. All metrics - regardless of the originating file - should log come custom attribute as a property

I do not understand this part. Could you elaborate more and point me to the example with these issues?

ijemmy avatar Apr 21 '22 10:04 ijemmy

Thanks @ijemmy and @dreamorosi for looking into this.

What do you think?

I do like the Recommended practices and updating the examples. As a Powertools user, this could've helped me start more quickly.

I'm coming from a Java background mostly, and have been using Powertools there. With a recent TypeScript project where I knew Powertools would be a good option to drive consistency/good practices in my team's Lambda functions, I wanted to check Powertools TypeScript but from the documentation, the answers to how to do logging/metrics/tracing in non-handler file/function was not clear.

I do not understand this part. Could you elaborate more and point me to the example with these issues?

I'll mention a use case and draw some parallels to Java/Powertools.

Assume that the Lambda function is servicing requests by human users (e.g. lambda is a backend for a frontend). I'd like to add a userId property to all log lines and EMF metrics lines. Currently, with the powertools.ts approach we can do something like

  • metrics: metrics.addMetadata('userId', 'xx') anywhere in code (handler file or others)
  • logs: potentially using persistent log attributes
    • We need request-scoped rather than persistent log attributes. The ability to remove keys from persistent log attributes might provide the ability to cleanup via a middy middleware for instance after the request. For the use case described above, and without the cleanup, there's potential bugs waiting where values are carried across requests.
    • with https://github.com/awslabs/aws-lambda-powertools-typescript/issues/482, the concept of a logger name + other mentions of custom keys are what is making me wonder how will that be handled - even with the powertools.ts approach.
      • One approach might be the child loggers of the powertools.ts-instantiated one which should be configured in the handler/middy. Those child loggers might get their own loggerName. But what about the lambdaContext, or other custom keys like my hypotheticaluserId.
        • In Java, with LoggingUtils.appendKeys and the lambda context injected, when you use any logger, in any file, they get those extra keys/properties as those rely on a global log4j MDC. This means, that appendKeys can be called in any file, and from that point onwards, any logger will be outputting those extra properties.
        • In TypeScript, do you want to imitate that behaviour, or should child loggers get the state of their parent (kept in sync, or copied). Regardless of the choice, documentation must be really clear about this.

humanzz avatar Apr 21 '22 15:04 humanzz

Are the Logger/Tracer/Metrics classes computationally expensive to instantiate? It seems really awkward to force consumers to essentially build their own wrapper export around them for instance access. Why not instead export default instances that are ready to be used? Consumers can then have a consistent way to import these instances across all their files/modules. This can be done without forcing singleton usage. The majority of users will be best served using the default instances and the minority with more specialized requirements can still create separate instances as needed. This is the pattern today used by the popular lambda-log library.

If default instances are expensive to instantiate, then they can be published under a separate file that gets imported only by those who want them. Unless that import comes up in the handler path, bundlers will just exclude the code that instantiates default instances. Best of both worlds. Here's an example of what such default imports could look like:

// Easy imports for most use-cases
import { logger } from '@aws-lambda-powertools/logger/defaults'
import { metrics } from '@aws-lambda-powertools/metrics/defaults'
import { tracer } from '@aws-lambda-powertools/tracer/defaults'

// If a hypothetical single wrapper package for all of these existed:
import { logger, metrics, tracer } from '@aws-lambda-powertools/powertools/defaults'

Separate from the topic of reuse is the importance of request scoped log attributes. The clearState flag on the injectLambdaContext options kind of works, but it's annoying that all state is cleared rather than just request specific state. I think a better UX can be achieved by having an API called appendRequestScopedKeys on the Logger class. All it needs to do is keep track of the awsRequestId from the Lambda context and clear away its data if it detects that it has changed.

Would love to hear your thoughts on this matter, @dreamorosi, @ijemmy, @humanzz. These two pain points are the most annoying parts of using Powertools in Node today.

bilalq avatar Jun 29 '22 02:06 bilalq

Hi @bilalq, thanks for engaging with this issue.

The classes for the utilities that we support at the moment are not expensive to instantiate, the only logic that is run at init time is configuring the instances according to the options/params that can be passed by env variables or constructor.

At the moment we don't provide instances of the utilities instantiated with default because we wanted to give developers the ability to initialise the classes and customise their behaviour directly. Even using the method you propose you'd be essentially be saving one line but you'd be losing the chance to configure the class via constructor parameters.

We simply don't have enough feedback from the community yet to determine whether the default settings would be fine for most use cases like you suggest, but if that's the case we can consider providing these already-instantiated utils. I'd like to defer the decision to a later date after more members from the community and users have also given their opinion. Would you be so kind to open a dedicated discussion essentially porting the content of your comment, so that we can have a period of time to gather more opinions? If after that we see enough interest we'll definitely consider this.


Regarding the second topic, I'm not sure I understand the issue you are describing. When using clearState only the keys that have been added to the logger from within the scope of the handler are cleared between invocation (this is assuming you're actually instantiating the logger outside of the handler).

So for instance this code:

import middy from "@middy/core";
import { Logger, injectLambdaContext } from "@aws-lambda-powertools/logger";
import axios from "axios";

const logger = new Logger({
  persistentLogAttributes: {
    foo: "bar",
  },
});

export const handler = middy(async (event: any = {}): Promise<any> => {
  logger.info("Hello World");

  try {
    await axios.get("https://httpbin.org/status/200");
  } catch (error) {
    logger.error("error:catch", { error });
    throw error;
  } finally {
  }
}).use(injectLambdaContext(logger, { clearState: true }));

Will have the "foo": "bar" key-value in all logs across different invocations.

Let's now take a second example:

import middy from "@middy/core";
import { Logger, injectLambdaContext } from "@aws-lambda-powertools/logger";
import axios from "axios";

const logger = new Logger({
  persistentLogAttributes: {
    foo: "bar",
  },
});

let isColdStart = true;

export const handler = middy(async (event: any = {}): Promise<any> => {
  if (isColdStart) {
    isColdStart = false;
    logger.appendKeys({
      another: "key",
    });
    logger.addPersistentLogAttributes({
      other: "key",
    });
  }

  logger.info("Hello World");

  try {
    await axios.get("https://httpbin.org/status/200");
  } catch (error) {
    logger.error("error:catch", { error });
    throw error;
  } finally {
  }
}).use(injectLambdaContext(logger, { clearState: true }));

In the above I'm using a trivial logic to run the portion of code that calls logger.appendKeys & logger.addPersistentLogAttributes only for the first invocation, but it's just a contrived example to convey the point that this portion of code runs only during the 1st invocation..

In this case, the "foo": "bar" key-value will again be present in all logs and across execution. On the other hand "another": "key" and "other": "key" will be present only in the logs of the first execution, this is because we are passing the clearState flag equals to true.

So to sum up, the behaviour you're describing is already happening: only request specific state is cleared while persistent attributes set outside of the handler are maintained. This, by the way, is an advantage of having you - the developer - instantiate the Logger class; by doing so you can pass configuration parameters that will be shared across invocations of the same execution environment.

If you want to continue the conversation on this second topic, please open a dedicated issue.

dreamorosi avatar Jun 30 '22 11:06 dreamorosi

Discussion created over here as requested: https://github.com/awslabs/aws-lambda-powertools-typescript/discussions/1012

bilalq avatar Jul 03 '22 04:07 bilalq

This issue has not received a response in 2 weeks. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

github-actions[bot] avatar Feb 28 '23 00:02 github-actions[bot]