-
Notifications
You must be signed in to change notification settings - Fork 154
Feature request: Change ordering of default log formatter #1568
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
Comments
I'm happy to submit a PR for this if the team would be okay to merge it. Also open to hearing feedback on the ordering. My rationale for putting log level first is that it makes it easy to visually scan and see which log entries are for errors vs info vs debug. After that, the message is the most important attribute, IMO. The request and trace ids are then the most useful bits for pulling additional context around a given log line. |
Hi @bilalq thank you for taking the time to open an issue. As you mentioned, we can't guarantee the order of the keys so if my memory serves me correctly, we simply ordered the keys alphabetically when creating the formatter. I'm okay with changing the ordering as you suggested in your second code example and have As a side note, the Python version of Powertools has |
One correction from the issue description: |
This resolves aws-powertools#1568. The problem raised in that issue is that the log formatter outputs log lines with keys written in alphabetical order. In practice, that means they start off with the cold start flag, the `function_arn`, `function_name`, `function_memory_size`, and a whole bunch of other stuff that is not usually of interest. The actual log level and message are buried and need log lines expanded to be seen. Even when expanded, they're hard to visually spot at a glance. When viewing a log group in Cloudwatch or some other UI that truncates log lines, you effectively see no useful information at all (see screenshot in aws-powertools#1568 for example). This change addresses the problem by surfacing log properties in order of usefulness. This is a subjective decision, and opinions may vary, but the order submitted in this PR should be preferable for the vast majority of users. The "at-a-glance" view of logs should now be actionable and help with log-diving and debugging. The actual information recorded is unchanged and log insight queries should still work as before. The serialization order is well-defined in modern ECMAScript engines: Serialization From the MDN docs on `JSON.stringify`: > Only enumerable own properties are visited. This means `Map`, `Set`, > etc. will become `"{}"`. You can use the replacer parameter to > serialize them to something more useful. Properties are visited using > the same algorithm as `Object.keys()`, which has a well-defined order > and is stable across implementations. For example, `JSON.stringify` on > the same object will always produce the same string, and > `JSON.parse(JSON.stringify(obj))` would produce an object with the > same key ordering as the original (assuming the object is completely > JSON-serializable). > > Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#description And the note about what that order is: > The traversal order, as of modern ECMAScript specification, is > well-defined and consistent across implementations. Within each > component of the prototype chain, all non-negative integer keys (those > that can be array indices) will be traversed first in ascending order > by value, then other string keys in ascending chronological order of > property creation. > > Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
This resolves aws-powertools#1568. The problem raised in that issue is that the log formatter outputs log lines with keys written in alphabetical order. In practice, that means they start off with the cold start flag, the `function_arn`, `function_name`, `function_memory_size`, and a whole bunch of other stuff that is not usually of interest. The actual log level and message are buried and need log lines expanded to be seen. Even when expanded, they're hard to visually spot at a glance. When viewing a log group in Cloudwatch or some other UI that truncates log lines, you effectively see no useful information at all (see screenshot in aws-powertools#1568 for example). This change addresses the problem by surfacing log properties in order of usefulness. This is a subjective decision, and opinions may vary, but the order submitted in this PR should be preferable for the vast majority of users. The "at-a-glance" view of logs should now be actionable and help with log-diving and debugging. The actual information recorded is unchanged and log insight queries should still work as before. The serialization order is well-defined in modern ECMAScript engines: Serialization From the MDN docs on `JSON.stringify`: > Only enumerable own properties are visited. This means `Map`, `Set`, > etc. will become `"{}"`. You can use the replacer parameter to > serialize them to something more useful. Properties are visited using > the same algorithm as `Object.keys()`, which has a well-defined order > and is stable across implementations. For example, `JSON.stringify` on > the same object will always produce the same string, and > `JSON.parse(JSON.stringify(obj))` would produce an object with the > same key ordering as the original (assuming the object is completely > JSON-serializable). > > Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#description And the note about what that order is: > The traversal order, as of modern ECMAScript specification, is > well-defined and consistent across implementations. Within each > component of the prototype chain, all non-negative integer keys (those > that can be array indices) will be traversed first in ascending order > by value, then other string keys in ascending chronological order of > property creation. > > Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
Having the same functionality in typescript would be awesome ! I'd be willing to contribute with some guidance. |
Hi @mmeylan thank you for offering to contribute to the project. As a first stop, and if you haven't already done so, I would recommend to take a look at the contributing document for the repo. In this doc you should be able to find the main info about how to contribute & setup your dev environment. Since you have already identified an opportunity for contributing and an issue, you're already more than halfway there. After that, in case you want to get more details about the repo structure or other conventions, I would recommend reading also the pages under the "Contributing" section in our documentation: https://docs.powertools.aws.dev/lambda/typescript/latest/contributing/setup/ Once you feel ready to contribute, you can fork the repo and create your working branch starting from the In terms of implementing the feature, the main reference and North Star in terms of developer experience we are looking for is the one found in the Python version of Powertools. To that extent, I imagine that we will have to add a new property to the Then, we'll likely have to handle this new option in the constructor method of the Logger and perhaps pass it down to the Once that's done, we should modify the implementation of the In doing so, we should keep in mind a couple of things:
If any of the above is not clear or you'd like to discuss any of it further please don't hesitate to ask. Before staring the implementation it'd be great if you could lay out a rough plan of how you plan on implementing the feature so that we can try catching potential issues with it early on. Feel free to continue the discussion under this issue, we'll make sure to reply as soon as we can. For a closer collaboration, or if you prefer to have a more real-time conversation (time zone allowing it), you can also join our Discord server, you'll find me & the other maintainers of this repo, as well as other members of the community in the |
I will be glad to help with this issue if still need some help :) |
Hi @willpeixoto, yes, help is always welcome! Thank you for offering, I'll assign the issue to you. If you have any questions now or during development, please let me know. We're happy to help either here or on Discord if less async communication is needed. |
Sensacional hein monstrão @willpeixoto! :D We are so happy to have your contribution to the Powertools. If you have any questions, let us know and we'll help! |
Hi @willpeixoto - just wanted to check in to see if you were still working on this or if you needed any help. |
Putting back the issue on the backlog, if anyone is interested in picking this up please leave a comment. |
@dreamorosi I have made a draft WIP PR with some questions. Please have a look whenever you are available. |
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. |
Use case
I know that users can provide their own LogFormatter instance, but the I believe the default should still be reasonably well-suited for most people.
The problem today is that the output frontloads a bunch of noise that isn't useful and makes the CloudWatch console log stream view useless without expanding and hard to read/skim even when you do.
At a glance, all you see is whether or not an invocation was a cold start and the ARN. Actually interesting information like log level and message are buried.
Solution/User Experience
Technically, V8 and every other JS engine I'm aware of don't guarantee ordering of JS object literals when serializing/deserializing. In practice, when the object key counts are not high, the order as written does get preserved.
If we look here:
https://github.com/aws-powertools/powertools-lambda-typescript/blob/main/packages/logger/src/formatter/PowertoolLogFormatter.ts#L19-L33
Changing:
To something like this:
Would likely be a lot more usable. This brings the log level, message, and execution ids to the forefront.
With regards to backwards compatibility: No promised API or contractual behavior changes. Technically, if a user had regex based parsing of log lines, that may break. However, I'd argue that given the JS engine already doesn't guarantee order preservation, such a dependency never had guarantees to begin with and should not be considered breakage of backwards compatibility.
Alternative solutions
No response
Acknowledgment
Future readers
Please react with 👍 and your use case to help us understand customer demand.
The text was updated successfully, but these errors were encountered: