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: Support Logging Incoming Event Without Use of Middy or Class #1904

Closed
1 of 2 tasks
bestickley opened this issue Jan 12, 2024 · 7 comments · Fixed by #2924
Closed
1 of 2 tasks

Feature request: Support Logging Incoming Event Without Use of Middy or Class #1904

bestickley opened this issue Jan 12, 2024 · 7 comments · Fixed by #2924
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility

Comments

@bestickley
Copy link

bestickley commented Jan 12, 2024

Use case

I want the incoming log event to be automatically logged for me when POWERTOOLS_LOGGER_LOG_EVENT is set to true and I'm not using middy or a handler class. Middy and handler class usage is demonstrated in docs here.

Solution/User Experience

Conditionally log incoming event based on POWERTOOLS_LOGGER_LOG_EVENT within Logger.addContext. I know this may seem strange to put "log incoming event logic" into the addContext method, but that's how it's implemented with middy (injectLambdaContext) and class decorator (logger.injectLambdaContext).

With this solution, the below function would log incoming event. Currently it does not.

import { Logger } from "@aws-lambda-powertools/logger";
process.env.POWERTOOLS_LOGGER_LOG_EVENT = "true";
const logger = new Logger();
export function handler(event, context) {
  logger.addContext(context);
  // do work
}

Alternative solutions

No response

Acknowledgment

Future readers

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

@bestickley bestickley 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 12, 2024
@am29d
Copy link
Contributor

am29d commented Jan 15, 2024

Hi @bestickley , thanks for the feature request. This is indeed a missing feature for customer who are not using middy or decorator. I checked the implementation and from the first impression it looks like we could move event logging into the addContext method. injectLambdaContext calls addContext before logging the event.

I think the tricky part is to keep the call clean as you did your example logger.addContext(contex). In this case, we have no chance to grab the event object without explicitly passing it to the function. So it is most likely to have the signature logger.addContext(context, event) which looks weird because the parameters are reversed.

The feature request is valid and will bring value, we just need to agree on the implementation details.

@am29d am29d added logger This item relates to the Logger Utility discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait labels Jan 15, 2024
@bestickley
Copy link
Author

@am29d, I agree logger.addContext(context, event) is a little weird but I'm not opposed. You could make event optional as second argument. Alternatively you could overload the function to allow for logger.addContext({ context, event }). I don't have a strong preference.

@dreamorosi
Copy link
Contributor

It's true that we have overloaded the meaning of the injectLambdaContext decorator and Middy middleware making it do multiple things, this was done for two reasons: 1/ injecting the context in each log entry is the main feature, and 2/ consistency with other Powertools versions (mainly Python).

The overloading came also from a place of necessity. Both the Middy middleware and decorator wrap the handler and have access to its arguments (aka event and context), so adding other features made sense versus adding a different decorator or middleware for each minor feature.

In this case the logger.addContext() method both in name and implementation is exclusively dedicated to inject the context so in my opinion adding a second unrelated argument is not the way to go.

Starting from the assumption that you can already log the event by just calling logger.info('event', event); anywhere in your function scope, the real value of this feature (and potentially the root cause of the request) revolves around the ability to control whether the event is logged or not using the POWERTOOLS_LOGGER_LOG_EVENT environment variable without having to modify your code.

If this is true I would be more inclined to go in a different direction:

  • short-term: add a dedicated logger.logEvent(event); method which emits the log based on the value of POWERTOOLS_LOGGER_LOG_EVENT
  • medium-term: come up with a new high-level equivalent for the decorator and Middy middleware that doesn't require you to use decorators (which are experimental and require TS as well as to opt in to a specific way of structuring your code) nor Middy middleware (which requires you to add and trust an external dependency) - this is a discussion that has come up other times with the most recent being in Feature request: Add pure functional middleware (willing to contribute) #1833

@am29d am29d moved this from Triage to Backlog in Powertools for AWS Lambda (TypeScript) Jan 16, 2024
@am29d
Copy link
Contributor

am29d commented Jan 16, 2024

I agree on having a separate method logEvent. I have seen many customers trying various ways to log an event, including JSON.stringify() inside logger.info.

@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Aug 14, 2024
@dreamorosi dreamorosi self-assigned this Aug 14, 2024
@dreamorosi dreamorosi moved this from Backlog to Working on it in Powertools for AWS Lambda (TypeScript) Aug 14, 2024
@dreamorosi dreamorosi moved this from Working on it to On hold in Powertools for AWS Lambda (TypeScript) Aug 14, 2024
@dreamorosi
Copy link
Contributor

Hi both, while doing some maintenance on the Logger I noticed that we already have a mostly undocumented public method called logEventIfEnabled() that does exactly what we are proposing in one of the comments above.

The method would be used like this:

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

const logger = new Logger();

export const handler = async (event: unknown) => {
  logger.logEventIfEnabled(event); // (1)
  // ... your handler code
};

It takes in account the value of POWERTOOLS_LOG_EVENT and besides that, behaves the same as the usage with Middy.js middleware or class method decorator.

For now I have opened a PR to add documentation and examples for this method and will be closing the issue once the PR is merged, since the first low hanging fruit has been addressed.

In regards to a better and less verbose DX for users who don't want to use Middy.js nor class method decorators, we can continue the discussion in #1833.

@dreamorosi dreamorosi moved this from On hold to Pending review in Powertools for AWS Lambda (TypeScript) Aug 14, 2024
@github-project-automation github-project-automation bot moved this from Pending review to Coming soon in Powertools for AWS Lambda (TypeScript) Aug 16, 2024
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.

@github-actions github-actions bot added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Aug 16, 2024
@dreamorosi dreamorosi moved this from Coming soon to Shipped in Powertools for AWS Lambda (TypeScript) Sep 16, 2024
Copy link
Contributor

github-actions bot commented Oct 8, 2024

This is now released under v2.9.0 version!

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped 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

Successfully merging a pull request may close this issue.

3 participants