-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat: log http POST requests body in http-access log #3214
Conversation
const valKeys = Object.keys(value) | ||
if (valKeys.length > 0) { | ||
// value is an object | ||
return prev + ` body.${curr}=${JSON.stringify(value)}` |
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.
is this going to be extremely long? agree we want it tho
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.
does seem like a risk, yes. This is printing the full body of the commit being sent to js-ceramic, so is users are creating large json documents in their app, this will print the full document, which could be up to 256kb large
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.
um if we do this for every single request on gitcoin i definitely think its a risk of crashing them by filling up the logs. i would like to sample them
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.
lgtm, only slightly worried about really long logs. also assuming you tested the morgan.token code
Not quite sure what you mean by this? The testing I did was running the node, sending http requests to the node, and looking at the log messages generated. |
yeah i meant if there were unit tests, its fine Are we logging every request? i'm still concerned we could fill up disks with this stuff - hopefully we could log this much detail only on error? |
yeah, agreed this is too verbose right now. We should not merge this without some controls to avoid filling the log with literally every document written to Ceramic. |
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.
(we agreed that we need to make it a little lighter/more selective before merge)
do you want these maybe? #3215
This PR needs more work to be ready to merge, and since js-ceramic is dying soon and we've made it this long without this working, going to go ahead and close this unmerged |
Example log line from the POST request to create a new stream with the genesis commit: