log full body only on slow requests and only 10 per day #3215
Conversation
gvelez17
force-pushed
the
feat/log-slow-requests-daily
branch
from
c978c75
to
759234d
Compare
There was a problem hiding this comment.
hmm... I feel like ideally the slow request log would apply to all types of requests (whereas this handler for "body" only applies to POST requests, not GETs), would cause the query to be logged even when we otherwise aren't logging http requests, and would have a more custom message specifying explicitly that this is a slow query.
Then in the regular case when logging http requests normally, we would still log the body, but maybe not expand sub-objects, so you only get the top-level fields and sub-objects print as [object object] (which they were doing before I added the code to check if the value is an object with multiple keys and if so call JSON.stringify on it).
That said, doing all of that sounds tricky and just doing this is strictly better than where things are today, so I don't think we should hold this up too much to make it perfect.
Maybe one thing we could do is use the logSlowRequest variable to control whether we expand out sub-objects or not, but always include at least the top-level fields. What do you think?
| @@ -2,10 +2,38 @@ import { LoggerProvider } from '@ceramicnetwork/common' | |||
| import { Request, Response } from 'express' | |||
| import morgan from 'morgan' | |||
|
|
|||
| const EXPECTED_RESPONSE_TIME_MS = 2000 | |||
There was a problem hiding this comment.
I think the default threshold for what's considered a "slow" request should probably be higher. Maybe 15 seconds?
Also might be nice to make this threshold configurable.
| @@ -2,10 +2,38 @@ import { LoggerProvider } from '@ceramicnetwork/common' | |||
| import { Request, Response } from 'express' | |||
| import morgan from 'morgan' | |||
|
|
|||
| const EXPECTED_RESPONSE_TIME_MS = 2000 | |||
| const MAX_DAILY_SLOW_REQUESTS_TO_LOG = 10 | |||
There was a problem hiding this comment.
What about only logging one slow query per minute instead? I think that will still be infrequent enough to not flood the logs, while making the slow queries a lot more visible if they are happening often.
Description
to be combined with the PR for logging full body of requests, do so only for slow requests and limit how many
How Has This Been Tested?
Describe the tests that you ran to verify your changes. Provide instructions for reproduction.
PR checklist
Before submitting this PR, please make sure:
References:
Please list relevant documentation (e.g. tech specs, articles, related work etc.) relevant to this change, and note if the documentation has been updated.