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: widen types for Logger arguments #1777

Open
1 of 2 tasks
dreamorosi opened this issue Nov 1, 2023 · 3 comments
Open
1 of 2 tasks

Feature request: widen types for Logger arguments #1777

dreamorosi opened this issue Nov 1, 2023 · 3 comments
Labels
discussing The issue needs to be discussed, elaborated, or refined feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility need-customer-feedback Requires more customers feedback before making or revisiting a decision revisit-in-3-months Blocked issues/PRs that need to be revisited

Comments

@dreamorosi
Copy link
Contributor

dreamorosi commented Nov 1, 2023

Use case

The current implementation of our Logger is fairly stringent when it comes to types and objects allowed in the logs when compared with the Python implementation.

For instance, in Python it’s possible to log a wide number of objects:

from dataclasses import dataclass
from aws_lambda_powertools import Logger

logger = Logger()


@dataclass
class Stuff:
    def __init__(self, name):
        self.name = name


def main():
    logger.info("Hello world!")
    logger.info(3)
    logger.info(True)
    logger.info(Stuff("foo"))
    try:
        raise Exception("Something went wrong")
    except Exception as e:
        logger.info(e)
    logger.info({"foo": "bar"})
    logger.info(None)
    logger.info([1, 2, 3])


if __name__ == "__main__":
    main()

which produce the following logs:

{"level":"INFO","location":"main:14","message":"Hello world!","timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:15","message":3,"timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:16","message":true,"timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:17","message":"Stuff()","timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:21","message":"Something went wrong","timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:22","message":{"foo":"bar"},"timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:23","timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}
{"level":"INFO","location":"main:24","message":[1,2,3],"timestamp":"2023-10-31 10:42:27,011+0100","service":"service_undefined"}

On TypeScript instead, the current typing allows the first argument to only be a string, and the second a limited amount of types (mainly strings, objects, and errors).

A condensed version of the current types, and its limitation can be found in this TS Playground.

We should aim at expanding the type of arguments of the logger to more closely align with the Python counterpart.

Solution/User Experience

You can find the proposed types in this other TS playground, but essentially after the changes, it should be possible to log all the following:

logger.info('hi');
logger.info('hi', { bob: '' });
logger.info('hi', { bob: 1 });
logger.info('hi', { bob: true });
logger.info('hi', { bob: undefined });
logger.info('hi', { bob: BigInt(1212) });
logger.info('hi', { error: new Error('ol'), bob: 1 });
logger.info('hi', { stuff: new Error('ol') });
logger.info('error', new Error('bummer'));
logger.info(3);
logger.info(undefined);
logger.info(true);
logger.info(null);
logger.info([1, 2, 3]);
logger.info([1, 2, 3, 'a']);
logger.info({ foo: 'bar', message: 'string' });
logger.info({ foo: 'bar', message: 2 });
logger.info({ foo: 'bar' });
logger.info(new Error('hi'));
// @ts-expect-error - second argument must be an object, otherwise we don't know where to put it on the top level
logger.info('hello', 2);
// @ts-expect-error - we limit types to non serializable ones (maybe a bad idea)
logger.info('hi', { bob: new Map([[1, 2]]) });
// @ts-expect-error - you cannot override the `message` key when already specifying the first argument  
logger.info('hi', { message: 'bar' });

Alternative solutions

No response

Acknowledgment

Future readers

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

@dreamorosi dreamorosi added logger This item relates to the Logger Utility feature-request This item refers to a feature request for an existing or new utility confirmed The scope is clear, ready for implementation labels Nov 1, 2023
@dreamorosi dreamorosi self-assigned this Nov 1, 2023
@dreamorosi dreamorosi added this to the Version 2.0 milestone Nov 1, 2023
@dreamorosi dreamorosi moved this from Working on it to Next iteration in Powertools for AWS Lambda (TypeScript) Jan 30, 2024
@dreamorosi dreamorosi removed their assignment Jan 30, 2024
@dreamorosi dreamorosi moved this from Next iteration to Ideas in Powertools for AWS Lambda (TypeScript) Jan 30, 2024
@dreamorosi dreamorosi removed this from the Version 2.0 milestone Jan 30, 2024
@dreamorosi dreamorosi added need-customer-feedback Requires more customers feedback before making or revisiting a decision discussing The issue needs to be discussed, elaborated, or refined and removed confirmed The scope is clear, ready for implementation labels Jan 30, 2024
@dreamorosi
Copy link
Contributor Author

We would like to hear more feedback from customers and understand how you use the logger as well as which limitations you have encountered before moving forward.

@OffensiveBias-08-145
Copy link

I have not encountered a situation where a message is not provided in a log statement.

IMO, logs should tell you something and have enough context for one to understand.
Having logs that simply return null or true gives the reader no context.

For cases such as the one below, I can see the usage intent. However, this then leads down a path of determining what is json serializable and where it specifically should go in each log item.

This would mean a prescribed prop would need to be used to determine where such data is to be placed in the log. This would then conflict with the current understanding of the library and any usage of a custom formatter.

// @ts-expect-error - second argument must be an object, otherwise we don't know where to put it on the top level
logger.info('hello', 2);

@dreamorosi
Copy link
Contributor Author

IMO, logs should tell you something and have enough context for one to understand.
Having logs that simply return null or true gives the reader no context.

That's a fair statement and the above is just a proposal for now.

I have not encountered a situation where a message is not provided in a log statement.

In production environments this is definitely the case, but especially with debug logs sometimes I have found myself just wanting to log a value and not necessarily wanting to provide a message.

@dreamorosi dreamorosi added the revisit-in-3-months Blocked issues/PRs that need to be revisited label Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussing The issue needs to be discussed, elaborated, or refined feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility need-customer-feedback Requires more customers feedback before making or revisiting a decision revisit-in-3-months Blocked issues/PRs that need to be revisited
Projects
Development

No branches or pull requests

2 participants