-
Notifications
You must be signed in to change notification settings - Fork 184
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
Enable advanced monitoring for SQLServer & Postgres. #300
Conversation
src/main/java/com/oltpbenchmark/api/collectors/monitoring/DatabaseMonitor.java
Outdated
Show resolved
Hide resolved
src/main/java/com/oltpbenchmark/api/collectors/monitoring/DatabaseMonitor.java
Outdated
Show resolved
Hide resolved
src/main/java/com/oltpbenchmark/api/collectors/monitoring/DatabaseMonitor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly this file could use some unit tests on the expected file rotation behavior and whatnot.
Could also mark it as future work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this comment. There's no unit tests right now, writing to file is tested as part of the pipeline but the content is not checked currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
End to end test works for me too.
src/main/java/com/oltpbenchmark/api/collectors/monitoring/README.md
Outdated
Show resolved
Hide resolved
src/main/java/com/oltpbenchmark/api/collectors/monitoring/SQLServerMonitor.java
Outdated
Show resolved
Hide resolved
…baseMonitor.java Co-authored-by: Brian Kroth <[email protected]>
…ME.md Co-authored-by: Brian Kroth <[email protected]>
…enheid/benchbase into add-sqlserver-monitoring
src/main/java/com/oltpbenchmark/api/collectors/monitoring/README.md
Outdated
Show resolved
Hide resolved
src/main/java/com/oltpbenchmark/api/collectors/monitoring/README.md
Outdated
Show resolved
Hide resolved
src/main/java/com/oltpbenchmark/api/collectors/monitoring/README.md
Outdated
Show resolved
Hide resolved
@ranaalotaibiMS here's the PR I was mentioning. Could potentially extend this to optionally support plan capture as well. |
.github/workflows/maven.yml
Outdated
@@ -423,13 +423,19 @@ jobs: | |||
- name: Run benchmark | |||
# Note: user/pass should match those used in sample configs. | |||
run: | | |||
java -jar benchbase.jar -b ${{matrix.benchmark}} -c config/sqlserver/sample_${{matrix.benchmark}}_config.xml --create=true --load=true --execute=true --json-histograms results/histograms.json | |||
java -jar benchbase.jar -b ${{matrix.benchmark}} -c config/sqlserver/sample_${{matrix.benchmark}}_config.xml -im 1000 -mt advanced --create=true --load=true --execute=true --json-histograms results/histograms.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extend to postgres as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Postgres needs a plugin installed before this works, is that even possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be. Can you point me at the extension we need? Maybe include it in some documentation with the PR too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had added a reference in the README that's in the monitoring folder. This is the name: pg_stat_statements
Let me know if/how enabling it is possible and I'll add the appropriate tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#510 should handle part of this, and I posted a partial PR against your branch to enable the others:
anjagruenheid#3
… CI pipeline (#510) For testing support in #300 Note, do to lack of support for adjusting CMD args in service container launching (actions/runner#2139), we basically have to manage starting the container via `docker compose` ourselves. Luckily there are scripts for that already, and this way the CI environment should match the local dev experience as well.
… into add-sqlserver-monitoring
Add PG tests for advanced monitoring feature
This PR introduces advanced monitoring (per query and system stats) for SQL Server and Postgres. Anyone currently using monitoring will not be impacted as advanced monitoring will need to be enabled with a new CL option (mt=advanced).
In essence, this code addition allows users to run queries against system tables (SQL Server) or plugins (Postgres) which allows us to extract system and query usage statistics while benchmarking (query plans, query text, pattern execution counts, cache hits etc.).