fix(logger): logger throws TypeError when log item has BigInt value #1201

shdq · 2022-12-28T12:24:20Z

Description of your changes

The replacer function used for JSON.stringify converts BigInt values to strings.

Added tests and removed external dependency for removeEmptyKeys() function.

How to verify this change

Related issues, RFCs

Issue number: #1117

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

Breaking change checklist

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.

…by dependency

shdq · 2022-12-28T12:36:14Z

packages/logger/src/log/LogItem.ts

  public removeEmptyKeys(attributes: LogAttributes): LogAttributes {
-    return pickBy(attributes, (value) => value !== undefined && value !== '' && value !== null);
+    const newAttributes: LogAttributes = {};
+    for (const key in attributes) {
+      if (attributes[key] !== undefined && attributes[key] !== '' && attributes[key] !== null) {
+        newAttributes[key] = attributes[key];
+      }
+    }
+
+    return newAttributes;
  }


Firstly, I wrote a declarative one-liner:

public removeEmptyKeys(attributes: LogAttributes): LogAttributes { return Object.fromEntries(Object.entries(attributes).filter(([, value]) => value !== undefined && value !== '' && value !== null)); }

But for the sake of readability, I stick to the verbose version.

I don't know if nested objects could be in the logItem, but it wasn't and isn't supported by this function.

Could you give an example of what do you mean by nested objects?

logger.info("my message", { value: 42, nested: { value: "nested value", emptyValue: '', undefinedValue: undefined, nullValue: null } });

I think we need to support nested objects though.

Here's an example of logs emitted by Logger v1.5.0 (latest):

from this code:

import { Logger, injectLambdaContext } from "@aws-lambda-powertools/logger"; import middy from "@middy/core"; const logger = new Logger({}); export const handler = middy(async () => { logger.info("my message", { value: 42, nested: { value: "nested value", emptyValue: "", undefinedValue: undefined, nullValue: null, }, }); }).use(injectLambdaContext(logger, { logEvent: true }));

this was the event payload passed:

{ "key1": "value1", "key2": "value2", "key3": "value3", "nested": { "some": "key", "other": 1 } }

We need to support nested objects both for the logEvent functionality (also present in the unit tests) and arbitrary objects

Oh after re-reading I realize I might have misunderstood your comment.

Were you referring to supporting the removal of undefined, null, etc. from nested objects?

yes, it's just go through the first level

I see, apologies for the misunderstanding!

I think that at the moment we should strive for maintaining the same exact functionality level as the current version without breaking changes. Which based on the test I did above, seems to at least remove undefined values.

Then later on, if there's demand we can explore extending the behavior to empty strings and/or null.

undefined values are filtered somewhere else 🙃

Great then!

And I definitely support/agree with the choice of using a more verbose version for the sake of readability.

dreamorosi · 2022-12-28T13:22:51Z

I see that there's no package-lock.json in the file diff, if you have done it differently, could you please remove the dependency by running npm remove lodash.pickby -w packages/logger?

This should make the necessary updates to the lock file as well as removing it from the package.json of the package. Note that in order to successfully be able to run the command you might have to temporarily revert the change to package.json, otherwise npm will think the two files are out of sync.

Could you please also remove @types/lodash.pickby from devDependencies in the same package while at it?

dreamorosi · 2022-12-28T13:28:01Z

Also, I see that the branch you're working on is behind main.

Normally this would be fine, but in this case I would strongly advise to rebase/bring it in line with main.

The reason why I say this is that as part of the migration to Node 18 we have updated all the dependencies and added the runtime to the unit & integration tests. For the integration tests to pass we need the CDK version that is currently present on main but not in your branch, which causes the integration tests to fail.

dreamorosi · 2022-12-28T13:29:51Z

packages/logger/src/Logger.ts

+   * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify#exceptions
+   * @private
+   */
+  private getReplacer(): (key: string, value: LogAttributes | Error | bigint) => void {


I like the new name, thanks for making the change

…t-values

dreamorosi · 2022-12-28T14:18:50Z

Thank you for syncing the branch & updating the dependencies & lock file.

I can confirm that the integration tests are passing: https://github.com/awslabs/aws-lambda-powertools-typescript/actions/runs/3794471678

dreamorosi

Thanks a lot @shdq for picking this up and for the quick PR.

I appreciate the choices made throughout.

dreamorosi · 2022-12-28T14:19:53Z

packages/logger/src/log/LogItem.ts

  public removeEmptyKeys(attributes: LogAttributes): LogAttributes {
-    return pickBy(attributes, (value) => value !== undefined && value !== '' && value !== null);
+    const newAttributes: LogAttributes = {};
+    for (const key in attributes) {
+      if (attributes[key] !== undefined && attributes[key] !== '' && attributes[key] !== null) {
+        newAttributes[key] = attributes[key];
+      }
+    }
+
+    return newAttributes;
  }


And I definitely support/agree with the choice of using a more verbose version for the sake of readability.

shdq · 2022-12-28T14:23:39Z

Thanks a lot @shdq for picking this up and for the quick PR.

I appreciate the choices made throughout.

I'm happy to help! Thanks for the fast feedback loop @dreamorosi!

github-actions · 2022-12-28T14:25:42Z

📊 Package size report `0.2%↑`

File	Before	After
`aws-lambda-powertools-logger-1.5.0.tgz`	`24.9 kB`	^0.6%↑`25.0 kB`
`logger-bundle.zip`	`25.4 kB`	^0.4%↑`25.5 kB`
Total _{(Includes all files)}	`152.8 kB`	^0.2%↑`153.0 kB`
Tarball size	`151.8 kB`	^0.1%↑`152.0 kB`

Unchanged files

File	Size
`aws-lambda-powertools-commons-1.5.0.tgz`	`10.2 kB`
`aws-lambda-powertools-metrics-1.5.0.tgz`	`17.9 kB`
`aws-lambda-powertools-tracer-1.5.0.tgz`	`22.4 kB`
`commons-bundle.zip`	`10.8 kB`
`metrics-bundle.zip`	`18.4 kB`
`tracer-bundle.zip`	`22.9 kB`

_{🤖 This report was automatically generated by pkg-size-action}
(options hash: c101f9ed32c1999be6905b281c82051e)

shdq added 2 commits December 28, 2022 15:56

fix(logger): fix typeError when a log item has BigInt value, add tests

05b5974

refactor(logger): add a test and rewrite removeEmptyKeys without pick…

1041c4e

…by dependency

pull-request-size bot added the size/L PRs between 100-499 LOC label Dec 28, 2022

shdq commented Dec 28, 2022

View reviewed changes

dreamorosi assigned shdq Dec 28, 2022

dreamorosi self-requested a review December 28, 2022 13:14

github-actions bot added the bug Something isn't working label Dec 28, 2022

dreamorosi linked an issue Dec 28, 2022 that may be closed by this pull request

Bug: logging fails if input contains bigint values #1117

Closed

dreamorosi reviewed Dec 28, 2022

View reviewed changes

shdq added 2 commits December 28, 2022 17:34

Merge branch 'awslabs:main' into 1117-bug-logger-cant-serialize-bigin…

0dbf5e6

…t-values

remove dependencies

49b4d73

dreamorosi self-requested a review December 28, 2022 14:18

dreamorosi approved these changes Dec 28, 2022

View reviewed changes

dreamorosi merged commit a09e4df into aws-powertools:main Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(logger): logger throws TypeError when log item has BigInt value #1201

fix(logger): logger throws TypeError when log item has BigInt value #1201

shdq commented Dec 28, 2022

shdq Dec 28, 2022

shdq Dec 28, 2022

dreamorosi Dec 28, 2022

shdq Dec 28, 2022

dreamorosi Dec 28, 2022 •

edited

shdq Dec 28, 2022

dreamorosi Dec 28, 2022

shdq Dec 28, 2022

dreamorosi Dec 28, 2022

dreamorosi Dec 28, 2022

dreamorosi commented Dec 28, 2022

dreamorosi commented Dec 28, 2022

dreamorosi Dec 28, 2022

dreamorosi commented Dec 28, 2022

dreamorosi left a comment

dreamorosi Dec 28, 2022

shdq commented Dec 28, 2022

github-actions bot commented Dec 28, 2022

fix(logger): logger throws TypeError when log item has BigInt value #1201

fix(logger): logger throws TypeError when log item has BigInt value #1201

Conversation

shdq commented Dec 28, 2022

Description of your changes

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamorosi Dec 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamorosi commented Dec 28, 2022

dreamorosi commented Dec 28, 2022

Choose a reason for hiding this comment

dreamorosi commented Dec 28, 2022

dreamorosi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shdq commented Dec 28, 2022

github-actions bot commented Dec 28, 2022

📊 Package size report 0.2%↑

dreamorosi Dec 28, 2022 •

edited

📊 Package size report `0.2%↑`