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

Logging: print message as JSON, instead of wrapping into a string #1508

Closed
mriccia opened this issue Nov 8, 2023 · 14 comments
Closed

Logging: print message as JSON, instead of wrapping into a string #1508

mriccia opened this issue Nov 8, 2023 · 14 comments
Assignees
Labels
priority:2 High - core feature or affects 60% of the users status/staged-major-release This change will go with the next major release v2 Version 2
Milestone

Comments

@mriccia
Copy link
Contributor

mriccia commented Nov 8, 2023

Is your feature request related to a problem? Please describe.

The Powertools Logging functionality automatically wraps the messages into string, regardless of whether they are strings or objects.
Looking through the logs with CloudWatch logs is harder with the message being printed as a single string, and printing as JSON will also unlock other options (e.g. using CloudWatch insights to automatically discover fields).

Describe the solution you'd like

Describe alternatives you've considered

Additional context

@mriccia mriccia added the triage label Nov 8, 2023
@scottgerring scottgerring added priority:2 High - core feature or affects 60% of the users and removed triage labels Nov 9, 2023
@jeromevdl
Copy link
Contributor

I think this is a breaking change ==> v2

@jeromevdl jeromevdl added the v2 Version 2 label Nov 10, 2023
@jeromevdl jeromevdl moved this from Triage to Backlog in Powertools for AWS Lambda (Java) Nov 10, 2023
@scottgerring
Copy link
Contributor

This feels quite important. Is it really breaking and not a bug?
OTOH we are redoing the logging in v2 anyway, so there's a natural synergy there.

@jeromevdl
Copy link
Contributor

@jdoherty I'm taking this one. I'm working on the logging v2 anyway. Not sure that make sense you spend time on a pure log4j solution as I'm using slf4j now.

@jeromevdl jeromevdl assigned jeromevdl and unassigned jdoherty Nov 24, 2023
@jeromevdl
Copy link
Contributor

@jeromevdl
Copy link
Contributor

jeromevdl commented Nov 27, 2023

will be part of #1435, using the PowertoolsResolver in log4j and some custom code in JsonUtils for logbak

@jeromevdl
Copy link
Contributor

Discussion with Log4J: apache/logging-log4j2#2013

@scottgerring
Copy link
Contributor

@jeromevdl we should probably update this with resolution once you're done 👼

@jeromevdl
Copy link
Contributor

Based on the discussions with log4j guys, on this issue, and after thinking a bit more about this, we won't log messages as JSON. Instead, we'll support advanced object as additional arguments (not using MDC which is limited to Strings), and log them as JSON (when that makes sense).

Example:

Product product = new Product("id123456", "Name of the product", 234.5);

LOGGER.info("Some message", StructuredArguments.entry("key", product)); 

// can also use static import to make it lighter
LOGGER.info("Some message", entry("product", product));

will produce:

{
    "message": "Some message",
    "product": {
        "id": "id123456", 
        "name": "Name of the product", 
        "price": 234.5
    }
}

Note there was no {} in the message, if you do this:

LOGGER.info("My super product={}", entry("product", product));

will produce:

{
    "message": "My super product=[...]", // will use the object.toString()
    "product": {
        "id": "id123456", 
        "name": "Name of the product", 
        "price": 234.5
    }
}

Also working on other StructuredArguments:

  • entries that will take a complete Map
  • array that will take an Object... objects
  • json that will take a raw json string

For the event and response, we'll use this instead of putting the event in the message.

@scottgerring
Copy link
Contributor

@jeromevdl what do we need to do this - is it simply a matter of documenting this practice? I gather the user's interface here will be through SLF4j APIs.

@jeromevdl
Copy link
Contributor

No I need to implement some stuff. The StructureArguments thing and how they interpreted. It is supported with the logback-logstash-encoder but that's another library to add to the project...

@mriccia
Copy link
Contributor Author

mriccia commented Dec 14, 2023

My thoughts on this implementation are:
Yes we are achieving the objective of printing out objects as JSON structures in the logs, which is the ultimate goal.
However the onus is now on the library user to ensure that they log this way every time they intend to log an object.

What will I do if I only want to log an object, without a string message?
Will it be LOGGER.info("{}", entry("product", product));?

@jeromevdl
Copy link
Contributor

You cannot log without a message. Or you can just log an empty message, but messages are always there (it's the basic of the loggers).

you should do LOGGER.info("", entry("product", product)); but you will have an empty message in the logs...

@jeromevdl
Copy link
Contributor

jeromevdl commented Dec 14, 2023

we could actually check if the message is empty and not adding it to the log... but a log without a message is a bit weird

@jeromevdl jeromevdl mentioned this issue Dec 15, 2023
6 tasks
@scottgerring scottgerring added this to the v2 milestone Dec 21, 2023
@mriccia
Copy link
Contributor Author

mriccia commented Dec 21, 2023

I am OK with this (would like to hear other views), even though I don't love the syntax.

we could actually check if the message is empty and not adding it to the log... but a log without a message is a bit weird

Agree that it would be weird to print a log message, without any message

@scottgerring scottgerring moved this from Backlog to Coming soon in Powertools for AWS Lambda (Java) Feb 29, 2024
@jeromevdl jeromevdl added the status/staged-major-release This change will go with the next major release label Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:2 High - core feature or affects 60% of the users status/staged-major-release This change will go with the next major release v2 Version 2
Projects
Status: Coming soon
Development

No branches or pull requests

4 participants