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

feat(database): Add user_id and dttm composite index to Log model. #19532

Merged
merged 6 commits into from
Jul 7, 2022

Conversation

xneg
Copy link
Contributor

@xneg xneg commented Apr 5, 2022

SUMMARY

When opening welcome page (/superset/welcome/) it took between 10 and 20 seconds to show first results. This is because of /superset/recent_activity endpoint and query against logs table inside it. To speed up this query I suggest to add composite index from user_id and dttm to this table.

ADDITIONAL INFORMATION

  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested

@xneg xneg requested a review from a team as a code owner April 5, 2022 10:44
@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #19532 (4e305bf) into master (1eef923) will decrease coverage by 12.71%.
The diff coverage is n/a.

❗ Current head 4e305bf differs from pull request most recent head 1201780. Consider uploading reports for the commit 1201780 to get more accurate results

@@             Coverage Diff             @@
##           master   #19532       +/-   ##
===========================================
- Coverage   66.60%   53.88%   -12.72%     
===========================================
  Files        1678     1678               
  Lines       64246    64246               
  Branches     6539     6539               
===========================================
- Hits        42788    34616     -8172     
- Misses      19763    27935     +8172     
  Partials     1695     1695               
Flag Coverage Δ
hive 52.70% <ø> (ø)
mysql ?
postgres ?
presto 52.55% <ø> (ø)
python 56.50% <ø> (-25.90%) ⬇️
sqlite ?
unit 47.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/dashboard_import_export.py 0.00% <0.00%> (-100.00%) ⬇️
superset/key_value/commands/upsert.py 0.00% <0.00%> (-89.59%) ⬇️
superset/key_value/commands/update.py 0.00% <0.00%> (-89.37%) ⬇️
superset/key_value/commands/delete.py 0.00% <0.00%> (-85.30%) ⬇️
superset/key_value/commands/delete_expired.py 0.00% <0.00%> (-80.77%) ⬇️
superset/dashboards/commands/importers/v0.py 14.79% <0.00%> (-75.15%) ⬇️
superset/datasets/commands/importers/v0.py 24.82% <0.00%> (-68.80%) ⬇️
superset/databases/commands/test_connection.py 31.42% <0.00%> (-68.58%) ⬇️
superset/datasets/commands/update.py 25.88% <0.00%> (-68.24%) ⬇️
superset/datasets/commands/create.py 30.18% <0.00%> (-67.93%) ⬇️
... and 263 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 1eef923...1201780. Read the comment docs.

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM. Although I do wonder what's the implication of having this index on the write performance for logs.

In the long run, we should probably design two separate tables for different types of logs---one for online display such as auditing logs and recent activities, another for offline analysis such as page loading performance.

@rusackas rusackas merged commit d16f274 into apache:master Jul 7, 2022
dpgaspar added a commit to preset-io/superset that referenced this pull request Jul 7, 2022
dpgaspar added a commit that referenced this pull request Jul 7, 2022
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
…pache#19532)

* feat: Added user_id and dttm composite index to Log model.

* fix: Refactored migration to a general view.

* fix: Changed down revision.

* fix: Formatted with black.

* fix: Removed odd Index import.
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants