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

fully unroll params in log message, don’t include hidden props #420

Merged
merged 2 commits into from
Dec 10, 2014

Conversation

bkw
Copy link
Contributor

@bkw bkw commented Dec 1, 2014

The current log messages do not include values for some nested structures because of the arguments chosen for util.inspect limit the depth.
In the case of DynamoDB.updateItem this leads to ExpressionAttributeValues being masked by [Object], for instance.

This PR changes the call to util.inspect to not include shadow properties (arg2 false, why would we want this?) and also set the depth to unlimited (arg3 null).

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9665d17 on bkw:betterLogs into 2b463ce on aws:master.

@lsegal
Copy link
Contributor

lsegal commented Dec 9, 2014

I'm okay with setting the depth to unlimited, but is there a reason why we wouldn't want to show non-enumerable properties? Keep in mind the SDK might be serializing these properties even if they are non-enumerable (there is no contract in the SDK to ignore non-enumerable properties when serializing a request), so in those cases we would not be logging data that was being serialized, which could be confusing. The opposing argument is that users could take advantage of non-enumerable properties to explicitly hide values from logging, but I'm not sure that's a pattern currently being used in the SDK.

I would opt to leave the enumerable behavior the way it is unless there's a reason to change it.

@bkw
Copy link
Contributor Author

bkw commented Dec 10, 2014

agreed, I just reenabled it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) when pulling bc10498 on bkw:betterLogs into 2b463ce on aws:master.

lsegal added a commit that referenced this pull request Dec 10, 2014
fully unroll params in log message, don’t include hidden props
@lsegal lsegal merged commit 772df68 into aws:master Dec 10, 2014
@lsegal
Copy link
Contributor

lsegal commented Dec 10, 2014

Thanks!

AdityaManohar added a commit that referenced this pull request Dec 12, 2014
@lock
Copy link

lock bot commented Sep 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants