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

Documentation: reuse across code files/modules #777

Closed
humanzz opened this issue Apr 17, 2022 · 8 comments
Closed

Documentation: reuse across code files/modules #777

humanzz opened this issue Apr 17, 2022 · 8 comments
Assignees
Labels
confirmed The scope is clear, ready for implementation documentation Improvements or additions to documentation

Comments

@humanzz
Copy link

humanzz commented Apr 17, 2022

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

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 #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 humanzz added the triage This item has not been triaged by a maintainer, please wait label Apr 17, 2022
@ijemmy
Copy link
Contributor

ijemmy commented Apr 21, 2022

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 ijemmy added the documentation Improvements or additions to documentation label Apr 21, 2022
@humanzz
Copy link
Author

humanzz commented Apr 21, 2022

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 Feature request: feature parity with Python Powertools logger #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.

@dreamorosi dreamorosi added priority:medium and removed triage This item has not been triaged by a maintainer, please wait priority:medium labels Apr 22, 2022
@bilalq
Copy link

bilalq commented Jun 29, 2022

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.

@dreamorosi
Copy link
Contributor

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.

@bilalq
Copy link

bilalq commented Jul 3, 2022

Discussion created over here as requested: #1012

@dreamorosi dreamorosi added discussing The issue needs to be discussed, elaborated, or refined need-more-information Requires more information before making any calls labels Nov 13, 2022
@dreamorosi dreamorosi changed the title Feature (logger,tracer,metrics): Enable reuse across code files/modules Feature request: Enable reuse across code files/modules Nov 14, 2022
@github-actions

This comment was marked as off-topic.

@github-actions github-actions bot added the pending-close-response-required This issue will be closed soon unless the discussion moves forward label Feb 28, 2023
@dreamorosi dreamorosi removed pending-close-response-required This issue will be closed soon unless the discussion moves forward need-more-information Requires more information before making any calls labels Feb 28, 2023
@dreamorosi dreamorosi changed the title Feature request: Enable reuse across code files/modules Documentation: reuse across code files/modules Jul 29, 2024
@dreamorosi dreamorosi self-assigned this Jul 29, 2024
@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Jul 29, 2024
@dreamorosi dreamorosi moved this from Backlog to Shipped in Powertools for AWS Lambda (TypeScript) Jul 29, 2024
@dreamorosi
Copy link
Contributor

Revisiting this issue, and after some consideration I am inclined to close it

The current implementation, together with Node.js module caching properties already cover the requirement of enabling reuse of instances of our utilities across customer modules.

Relevant excerpt from the Node.js docs:

Modules are cached after the first time they are loaded. This means (among other things) that every call to require('foo') will get exactly the same object returned, if it would resolve to the same file.
Provided require.cache is not modified, multiple calls to require('foo') will not cause the module code to be executed multiple times. This is an important feature.

This means that, today, even without explicitly converting the utilities to singletons customers are able to share instances of the same utility following this patten:

powertools.ts

import { Logger } from '@aws-lambda-powertools/logger';

export const logger = new Logger({
  serviceName: 'product-service',
  logLevel: 'debug'
});

foo.ts

import { logger } from './logger.js';

export const foo = () => {
  logger.appendKeys({ someKey: 'someVal' });
};

index.ts

import { logger } from './logger.js';
import { foo } from './foo.js';

const handler = async () => {
  foo();
  
  logger.info('I am a singleton-ish');
};

which once run yields the following result:

image

Notice how in the logs (bottom of the screen) the log includes the someKey key which was added by another module (foo.ts), which shares the same logger defined in logger.ts.

Examples of this can be found both in our example app in the examples directory and the Powertools for AWS Lambda workshop. Credits for the idea of using this pattern go to @saragerion, from whom I lifted it from here.

Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments 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.

@dreamorosi dreamorosi moved this from Coming soon to Shipped in Powertools for AWS Lambda (TypeScript) Jul 29, 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 documentation Improvements or additions to documentation
Projects
Development

No branches or pull requests

4 participants