Improve notification query performance by reducing db calls #2470

Tirokk · 2020-10-05T12:15:51Z

Authored by roschaefer
Dec 10, 2019
Merged Dec 10, 2019

🍰 Pullrequest

We had plenty of database calls for our notification query. That could very well be the source of our recent performance issues in the backend.

Just don't ask for stuff that you don't want to show anyways

- update incorrect variables

- we removed it, prevent null pointer error

Tirokk · 2020-10-07T05:58:53Z

Authored by cypress[bot]
Dec 10, 2019

Test summary

50 • 0 • 0 • 0

Run details


Project	Human-Connection
Status	Passed
Commit	a1930121e3
Started	Dec 10, 2019 6:11 PM
Ended	Dec 10, 2019 6:23 PM
Duration	12:08 💡
OS	Linux Ubuntu Linux - 16.04
Browser	Chromium 78

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Tirokk · 2020-10-07T05:59:49Z

backend/src/schema/resolvers/reports.js

-      const readTxPromise = session.readTransaction(async tx => {
-        const allReportsTransactionResponse = await tx.run(
+      const readTxPromise = session.readTransaction(async transaction => {
+        const reviewedReportsTransactionResponse = await transaction.run(
          `
            MATCH (resource)<-[:BELONGS_TO]-(report:Report {id: $id})<-[review:REVIEWED]-(moderator:User)
            RETURN moderator, review


Authored by mattwr18
Dec 10, 2019

Outdated (history rewrite) - original diff

@@ -153,14 +155,14 @@ export default { const { id } = parent let reviewed const readTxPromise = session.readTransaction(async tx => { - const allReportsTransactionResponse = await tx.run( - ` - MATCH (resource)<-[:BELONGS_TO]-(report:Report {id: $id})<-[review:REVIEWED]-(moderator:User) - RETURN moderator, review - ORDER BY report.updatedAt DESC, review.updatedAt DESC - `, - { id }, - ) + const cypher = ` + MATCH (resource)<-[:BELONGS_TO]-(report:Report {id: $id})<-[review:REVIEWED]-(moderator:User) + RETURN moderator, review + ORDER BY report.updatedAt DESC, review.updatedAt DESC + ` + const params = { id } + const allReportsTransactionResponse = await tx.run(cypher, params) + log(allReportsTransactionResponse)

same question as your other PR @roschaefer
why so much refactoring just to add log??

Authored by roschaefer
Dec 10, 2019

I want to see the database calls being made when I set DEBUG=human-connection:neo4j:*. So I implemented this helper function which outputs the cypher statement and/or the database call statistics, the counters. We should always call the log method from now on when we implement database calls.

Authored by mattwr18
Dec 10, 2019

that part, I get... I don't understand how why that needed so much refactoring
I have refactored it and pushed it up.
Would this somehow not work with the code you added...
It would be nice to know how to use as well
If I run DEBUG=human-connection:neo4j:* yarn dev it throws this error

No matches for wildcard “DEBUG=human-connection:neo4j:*”. See `help expand`. env DEBUG=human-connection:neo4j:* yarn dev

so I'm assuming the wildcard needs to be replaced by something, but what? Ahh
it takes either human-connection:neo4j:cypher or human-connection:neo4j:stats??

Authored by roschaefer
Dec 10, 2019

On fish shell:

env DEBUG="human-connection:neo4j:*" yarn run dev

if you don't escape the * your shell believes it's a file glob pattern.

Authored by mattwr18
Dec 10, 2019

great thanks @roschaefer
then run a query and it will give me some crazy feedback?

Authored by mattwr18
Dec 10, 2019

ahhh, ok here are the result of running a reports query

2019-12-10T16:04:20.346Z human-connection:neo4j:cypher MATCH (report:Report)-[:BELONGS_TO]->(resource) WHERE (resource:User OR resource:Post OR resource:Comment) WITH report, resource, [(submitter:User)-[filed:FILED]->(report) | filed {.*, submitter: properties(submitter)} ] as filed, [(moderator:User)-[reviewed:REVIEWED]->(report) | reviewed {.*, moderator: properties(moderator)} ] as reviewed, [(resource)<-[:WROTE]-(author:User) | author {.*} ] as optionalAuthors, [(resource)-[:COMMENTS]->(post:Post) | post {.*} ] as optionalCommentedPosts, resource {.*, __typename: labels(resource)[0] } as resourceWithType WITH report, optionalAuthors, optionalCommentedPosts, reviewed, filed, resourceWithType {.*, post: optionalCommentedPosts[0], author: optionalAuthors[0] } as finalResource RETURN report {.*, resource: finalResource, filed: filed, reviewed: reviewed } ORDER BY report.createdAt DESC 2019-12-10T16:04:20.348Z human-connection:neo4j:cypher {} 2019-12-10T16:04:20.349Z human-connection:neo4j:stats StatementStatistics { _stats: { nodesCreated: 0, nodesDeleted: 0, relationshipsCreated: 0, relationshipsDeleted: 0, propertiesSet: 0, labelsAdded: 0, labelsRemoved: 0, indexesAdded: 0, indexesRemoved: 0, constraintsAdded: 0, constraintsRemoved: 0 } } 2019-12-10T16:04:20.349Z human-connection:neo4j:stats { resultConsumedAfter: 2, resultAvailableAfter: 56 }

Tirokk · 2020-10-07T06:00:46Z

backend/src/schema/resolvers/reports.js

-        if (!txResult[0]) return null
-        filed = txResult.map(reportedRecord => {
+        const filedReports = await readTxPromise
+        filed = filedReports.map(reportedRecord => {
          const { submitter, filed } = reportedRecord
          const relationshipWithNestedAttributes = {
            ...filed,


Authored by roschaefer
Dec 10, 2019

Outdated (history rewrite) - original diff

@@ -83,35 +82,35 @@ export default { params.offset && typeof params.offset === 'number' ? `SKIP ${params.offset}` : '' const limit = params.first && typeof params.first === 'number' ? `LIMIT ${params.first}` : '' - const reportReadTxPromise = session.readTransaction(async tx => { - const cypher = ` - MATCH (report:Report)-[:BELONGS_TO]->(resource) - WHERE (resource:User OR resource:Post OR resource:Comment) - ${filterClause} - WITH report, resource, - [(submitter:User)-[filed:FILED]->(report) | filed {.*, submitter: properties(submitter)} ] as filed, - [(moderator:User)-[reviewed:REVIEWED]->(report) | reviewed {.*, moderator: properties(moderator)} ] as reviewed, - [(resource)<-[:WROTE]-(author:User) | author {.*} ] as optionalAuthors, - [(resource)-[:COMMENTS]->(post:Post) | post {.*} ] as optionalCommentedPosts, - resource {.*, __typename: labels(resource)[0] } as resourceWithType - WITH report, optionalAuthors, optionalCommentedPosts, reviewed, filed, - resourceWithType {.*, post: optionalCommentedPosts[0], author: optionalAuthors[0] } as finalResource - RETURN report {.*, resource: finalResource, filed: filed, reviewed: reviewed } - ${orderByClause} - ${offset} ${limit} - ` - const allReportsTransactionResponse = await tx.run(cypher) + const reportReadTxPromise = session.readTransaction(async transaction => { + const allReportsTransactionResponse = await transaction.run( + ` + MATCH (report:Report)-[:BELONGS_TO]->(resource) + WHERE (resource:User OR resource:Post OR resource:Comment) + ${filterClause} + WITH report, resource, + [(submitter:User)-[filed:FILED]->(report) | filed {.*, submitter: properties(submitter)} ] as filed, + [(moderator:User)-[reviewed:REVIEWED]->(report) | reviewed {.*, moderator: properties(moderator)} ] as reviewed, + [(resource)<-[:WROTE]-(author:User) | author {.*} ] as optionalAuthors, + [(resource)-[:COMMENTS]->(post:Post) | post {.*} ] as optionalCommentedPosts, + resource {.*, __typename: labels(resource)[0] } as resourceWithType + WITH report, optionalAuthors, optionalCommentedPosts, reviewed, filed, + resourceWithType {.*, post: optionalCommentedPosts[0], author: optionalAuthors[0] } as finalResource + RETURN report {.*, resource: finalResource, filed: filed, reviewed: reviewed } + ${orderByClause} + ${offset} ${limit} + `, + ) log(allReportsTransactionResponse) return allReportsTransactionResponse.records.map(record => record.get('report')) }) try { - const txResult = await reportReadTxPromise - if (!txResult[0]) return null - reports = txResult + const reports = await reportReadTxPromise + if (!reports) return []

How could reports ever be not an array?

Authored by roschaefer
Dec 10, 2019

If we run into an error I would prefer HTTP 500

Authored by mattwr18
Dec 10, 2019

So, revert it to return null?

Mogge

great job @roschaefer !! this is really cool
now we need to make sure that we are adding logging and know how to read the output!

roschaefer and others added 7 commits December 10, 2019 18:09

Better debugging

b2ccc1b

Reduce database statements in notifications

3beef5e

Improve notification query performance

deefc4e

Just don't ask for stuff that you don't want to show anyways

Convert to transaction function/refactor

12236c9

- update incorrect variables

Follow @roschaefer suggestion/remove guard clauses

0a50a02

Remove disable from query request

4f0df2f

- we removed it, prevent null pointer error

Fix backend tests

cc7cd6b

Tirokk commented Oct 7, 2020

View reviewed changes

Mogge approved these changes Oct 8, 2020

View reviewed changes

Mogge closed this Oct 8, 2020

Mogge added the Github Import label Oct 8, 2020

ulfgebhardt deleted the pr2470head branch January 7, 2021 04:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve notification query performance by reducing db calls #2470

Improve notification query performance by reducing db calls #2470

Tirokk commented Oct 5, 2020

Tirokk commented Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Tirokk Oct 7, 2020

Mogge left a comment

Improve notification query performance by reducing db calls #2470

Improve notification query performance by reducing db calls #2470

Conversation

Tirokk commented Oct 5, 2020

🍰 Pullrequest

Tirokk commented Oct 7, 2020

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mogge left a comment

Choose a reason for hiding this comment