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

Slow query log #1052

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Slow query log #1052

merged 2 commits into from
Apr 17, 2024

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Apr 9, 2024

Description of Changes

Closes #1071

Log as tracing::warn! a query if it exceeds the threshold.

Note: Currently the default is burned here.

Note: The settings are not persited neither are transactional.

Is possible to query and set it at runtime:

🪐quickstart>SHOW slow_query_threshold;
 slow_query_threshold
----------------------
 100
 (1 row)
Time: 12.11ms

🪐quickstart>SET slow_query_threshold = 1
Time: 6.59ms
OK

The current settings:

slow_query_threshold = 100 ms
slow_incremental_updates_threshold = None
slow_subscriptions_threshold = None

Expected complexity level and risk

1

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • test_slow_queries
  • test_runtime_config
  • Check with the sql cli that SET slow_query_threshold TO 1 and running a query print the warning

Don't have a way to check for stdout.

@mamcx mamcx added release-any To be landed in any release window release-alpha-nice labels Apr 9, 2024
@mamcx mamcx requested a review from joshua-spacetime April 9, 2024 17:07
@mamcx mamcx self-assigned this Apr 9, 2024
}

/// Creates a new `SlowQueryConfig` with [THRESHOLD_QUERIES_MILLIS] for `queries` and the rest set to [None].
pub fn with_defaults() -> Self {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to confirm which values will be nice for bitcraft and how to read them.

@mamcx mamcx force-pushed the mamcx/slow-queries branch from 11fb6fb to 4659ba9 Compare April 10, 2024 18:58
@mamcx mamcx marked this pull request as ready for review April 10, 2024 19:07
@mamcx mamcx force-pushed the mamcx/slow-queries branch from 4659ba9 to 279596b Compare April 12, 2024 16:03
@mamcx mamcx requested a review from joshua-spacetime April 12, 2024 16:04
@mamcx mamcx force-pushed the mamcx/slow-queries branch from 279596b to f1ec5b2 Compare April 12, 2024 16:46
Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

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

This looks great @mamcx! Just a few more asks, but it should be ready to merge after that.

crates/core/src/db/relational_db.rs Outdated Show resolved Hide resolved
crates/core/src/util/slow.rs Outdated Show resolved Hide resolved
crates/core/src/util/slow.rs Outdated Show resolved Hide resolved
crates/core/src/util/slow.rs Show resolved Hide resolved
crates/core/src/config.rs Outdated Show resolved Hide resolved
@mamcx mamcx force-pushed the mamcx/slow-queries branch from f1ec5b2 to 6a17bf7 Compare April 15, 2024 15:58
Comment on lines 181 to 183
ReadConfigOption::SlowQueryThreshold => "slow_ad_hoc_query_threshold",
ReadConfigOption::SlowIncrementalUpdatesThreshold => "slow_incremental_query_threshold",
ReadConfigOption::SlowSubscriptionsThreshold => "slow_subscription_query_threshold",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Final naming suggestion :)

Suggested change
ReadConfigOption::SlowQueryThreshold => "slow_ad_hoc_query_threshold",
ReadConfigOption::SlowIncrementalUpdatesThreshold => "slow_incremental_query_threshold",
ReadConfigOption::SlowSubscriptionsThreshold => "slow_subscription_query_threshold",
ReadConfigOption::SlowQueryThreshold => "slow_ad_hoc_query_ms",
ReadConfigOption::SlowIncrementalUpdatesThreshold => "slow_tx_update_ms",
ReadConfigOption::SlowSubscriptionsThreshold => "slow_subscription_query_ms",

@mamcx mamcx force-pushed the mamcx/slow-queries branch from 6a17bf7 to b62bcca Compare April 16, 2024 19:11
@mamcx mamcx force-pushed the mamcx/slow-queries branch from b62bcca to dd35841 Compare April 17, 2024 13:36
@mamcx mamcx enabled auto-merge April 17, 2024 13:39
@mamcx mamcx added this pull request to the merge queue Apr 17, 2024
Merged via the queue into master with commit 7d5eb15 Apr 17, 2024
6 checks passed
@Centril
Copy link
Contributor

Centril commented Apr 18, 2024

This seems to have regressed incr-select compared to HEAD~1 but the other benchmarks are unaffected.
On i7-7700K, 64GB RAM:

Benchmarking incr-select: Collecting 100 samples in estimated 5.0
incr-select             time:   [226.69 ns 228.30 ns 230.24 ns]
                        change: [+24.989% +26.097% +27.045%] (p = 0.00 < 0.05)
                        Performance has regressed.

@Centril
Copy link
Contributor

Centril commented Apr 18, 2024

Partially resolved by #1110.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log slow queries
3 participants