Skip to content

Conversation

@bilalq
Copy link

@bilalq bilalq commented Jul 7, 2023

Description of your changes

This resolves #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.

This screenshot shows what the experience is like:

Screenshot 2023-06-29 at 6 58 58 AM

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

Related issues, RFCs

Issue number: #1568

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my change is effective and works
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

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
@bilalq bilalq requested a review from a team July 7, 2023 19:00
@boring-cyborg boring-cyborg bot added logger This item relates to the Logger Utility tests PRs that add or change tests labels Jul 7, 2023
@pull-request-size pull-request-size bot added the size/M PR between 30-99 LOC label Jul 7, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 7, 2023

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

@bilalq
Copy link
Author

bilalq commented Jul 7, 2023

Hey, @dreamorosi. This is the PR related to #1568. Note that I did not check the My changes generate no new warnings box because I was unable to get the lerna build/tests working right on my machine to confirm. It may or may not have new warnings, but since I can't explicitly confirm, I thought it best to leave that box unchecked.

@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jul 8, 2023
@dreamorosi
Copy link
Contributor

Hi @bilalq thanks for the PR, as you can see from the checks above there are a number of tests failing due to the change of order of the fields.

In order to run them locally, I would recommend you to check the contributing document that you can find here.
After setting up the dev environment (npm run setup-local) as described here, you should be able to run the tests for the utility with npm test -w packages/logger or all the unit tests with npm test -ws, as described here.

If there's anything that we can improve in the document please let me know.

Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my previous comment about unit tests.

@am29d
Copy link
Contributor

am29d commented Jul 14, 2023

Hey @bilalq I wanted to follow up on the issue so we can fix the tests and merge your contribution. Is there anything I can help with? Happy to connect in discord of schedule a quick call to walk you through the setup. This will help us to understand the struggles of new contributors and improve the future community work 🙏. Let me known.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
38.6% 38.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@dreamorosi
Copy link
Contributor

Hi @bilalq, we are going to close the PR for now.

If you decide to continue working on it please reopen it.

@dreamorosi dreamorosi closed this Jul 22, 2023
@bilalq
Copy link
Author

bilalq commented Jul 28, 2023

Sorry, I was away on travels these past few weeks. I'll reopen sometime in the next few days with the test fixes (or perhaps just documentation of the issues I've hit when trying to get them to run).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature PRs that introduce new features or minor changes logger This item relates to the Logger Utility size/M PR between 30-99 LOC tests PRs that add or change tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature request: Change ordering of default log formatter

3 participants