-
Notifications
You must be signed in to change notification settings - Fork 153
fix(logger): logger throws TypeError when log item has BigInt value #1201
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
Changes from 2 commits
05b5974
1041c4e
0dbf5e6
49b4d73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
import pickBy from 'lodash.pickby'; | ||
import merge from 'lodash.merge'; | ||
import { LogItemInterface } from '.'; | ||
import { LogAttributes } from '../types'; | ||
|
@@ -31,7 +30,14 @@ class LogItem implements LogItemInterface { | |
} | ||
|
||
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; | ||
} | ||
Comment on lines
32
to
41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Firstly, I wrote a declarative one-liner:
But for the sake of readability, I stick to the verbose version. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if nested objects could be in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you give an example of what do you mean by nested objects? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to support nested objects though. Here's an example of logs emitted by Logger 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes, it's just go through the first level There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Then later on, if there's demand we can explore extending the behavior to empty strings and/or There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great then! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I definitely support/agree with the choice of using a more verbose version for the sake of readability. |
||
|
||
public setAttributes(attributes: LogAttributes): void { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new name, thanks for making the change