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

Improve performance by splitting query details off into seperate table #481

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ErwinJunge
Copy link
Contributor

This PR aims to improve the performance of silk by splitting the query details off into a seperate database table.

Some background information: The current system stores the details of the queries directly inside the query table itself. This leads to a very wide table and consequently few tuples per page, which makes doing aggregates over large tables very expensive in IO since the database has to load all the pages into memory.

What this PR does to fix that is pull the details that aren't used in the overview pages out of the main table and into a seperate table. This decreases the width of the main table, leading to more tuples per page and therefore less IO when generating overviews.

Specifically, in our production environment the sqlquery table has >3M rows. This lead to the overview page taking more than a minute to load and therefore timeout. With these changes, the overview page takes ~9s. Still long, but at least it doesn't timeout anymore.

@ErwinJunge
Copy link
Contributor Author

@auvipy could you hit the button to approve the workflow run?

@@ -0,0 +1,38 @@
# Generated by Django 2.2.24 on 2021-06-24 09:54

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this migration handwritten? or auto generated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code inside the migration was handwritten, the file was autogenerated using ./manage.py makemigrations silk --empty --name fill_query_details

@codecov
Copy link

codecov bot commented Jul 2, 2021

Codecov Report

Merging #481 (ce07da5) into master (7e93d6c) will increase coverage by 1.48%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
+ Coverage   82.83%   84.32%   +1.48%     
==========================================
  Files          50       53       +3     
  Lines        2051     2086      +35     
==========================================
+ Hits         1699     1759      +60     
+ Misses        352      327      -25     
Impacted Files Coverage Δ
silk/migrations/0010_fill_query_details.py 66.66% <66.66%> (ø)
silk/collector.py 90.34% <100.00%> (+10.78%) ⬆️
silk/migrations/0009_sqlquerydetails.py 100.00% <100.00%> (ø)
silk/migrations/0011_auto_20210624_1450.py 100.00% <100.00%> (ø)
silk/models.py 87.44% <100.00%> (+0.54%) ⬆️
silk/views/sql_detail.py 94.33% <100.00%> (ø)
silk/utils/data_deletion.py 100.00% <0.00%> (ø)
silk/__init__.py
silk/urls.py
project/project/__init__.py 60.00% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e93d6c...ce07da5. Read the comment docs.

@Archmonger
Copy link
Contributor

@ErwinJunge This is a candidate for merge if tox tests can be passed.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants