feat: add slow query logging #7867
Conversation
Thanks for opening this pull request!
|
Codecov Report
@@ Coverage Diff @@
## alpha #7867 +/- ##
==========================================
- Coverage 94.18% 85.00% -9.18%
==========================================
Files 182 183 +1
Lines 13617 13641 +24
==========================================
- Hits 12825 11596 -1229
- Misses 792 2045 +1253
Continue to review full report at Codecov.
|
| body: req.body, | ||
| query: req.query, |
There was a problem hiding this comment.
Here i'm wondering about security, i can suggest some little improvements (inspired from Sentry):
- If body size is huge the _SlowQuery collection could be heavy, sentry currently limit the body size, may be we can splice the body at a defined size after a JSON.stringify (this size option could be added in the config).
- As i understand this hook will be triggered on all endpoints (create, update, get, find, login, signup etc...), here it's required to clean the body and remove sensitve data (like password, etc...), if i'm not wrong the
loggeralready has this feature. May be a little package exists.
| it('needs masterKey for slow queries', async () => { | ||
| await reconfigureServer({ | ||
| slowQuery: { | ||
| enable: true, | ||
| threshold: 300, | ||
| log: true, | ||
| }, |
There was a problem hiding this comment.
Is it possible to add a test to check that Parse.Cloud.beforeSave("_SlowQuery") works correctly. It could help developers to manipulate the body and remove sensitive data (we should add documentation suggestion about that)
I think we need to ensure/cover that this is supported.
|
@dblythy nice work 🚀 |
|
Yes I was thinking that too, it's obviously going to be problematic to store |
|
As i understand the current implementation rely on a middleware for all app endpoints. Benefits: Easily cover all endpoints By design it will be hard to add some future options to helps developers in production to disable for example a specific class slow query tracking for security purpose (health data, and more). In other hand in dev env it's not a problem, but this kind of feature is also really useful on production env. i can may be suggest to implement this feature in RestWrite, RestQuery system to be able to give some config options to disable SlowQuery on sensitive classes. A trigger should also added before cloud functions. What do you think about this @dblythy ? |
|
Thanks for your feedback @Moumouls! Personally I think that your suggestion is the end goal of slowQuery logging. I would in-vision that one day we would have logs of SlowQueries as well as logs of how long each async task took, so users could diagnose what caused the slow query (e’g whether it was a beforeSave, afterSave, auth). However, I think before that we should refactor the codebase (RestRead/RestWrite$ to async/await so the new SlowQuery code can be nice and clean. I think for a v1 of a new feature, recording path and body should be enough for users to replicate the slow query, and then we can evolve it in the future to include more analytics. I do agree though that this is more of a production feature, so we would have to ensure that we clean the output. |
|
See #7038 (comment) |
|
@mtrezza you are right i think in this case, some 3rd party services are may be more appropriate. Here i think it's a good idea to redirect developers to just plug their 3rd party lib on top of express (like this implementation). For example sentry has some example (i know that they have a performance utility): https://docs.sentry.io/platforms/node/guides/express/ here @dblythy i'm scared about security/performance issue of the system. Listening also the full express app, in case of graphql queries/mutations, it will be hard to replace sensitive informations. In another hand i understand the fast/YAGNI/POC methodology, but we saw also for example on the GraphQL implementation that we need to check widely the scope/standards/common used tools , (in graphql case: Relay API). The first implementation was ok, but after a quick check, it led us to rewrite/migrate all the system to Relay spec. Here a good idea is may to add some interface to allow developers to plug their favorite performance tool. Last thinking: This feature will be also useless in some case when developers use mongo atlas. They provide a good performance monitoring tool :) |
|
I will comment in #7038 (comment) to give some conceptual suggestions. |
parse-github-assistant
bot
removed
the
type:feature
New Pull Request Checklist
Issue Description
There's no way to know if your Parse Server has slow endpoints without using a third party tool. Slow Queries was a feature in the original Parse.com, and in fact the component still exists in Parse Dashboard
Related issue: #7038
Approach
Add an express listener to detect when routes take longer than the threshold. Pretty simple really.
TODOs before merging