Repository navigation
feat(database): add a database checks page - #1129
Closed
karlitschek wants to merge 9 commits into
Closed
karlitschek wants to merge 9 commits into
karlitschek wants to merge 9 commits into
Conversation
The Monitoring page had grown to fourteen sections on one long scroll, with the background job tables — three of them, each potentially long — sitting in the middle of the hardware readings. Checking whether cron is alive meant scrolling past the CPU and memory charts every time. Add a navigation at the top of the page that switches between a "Status" page, which keeps everything that describes the machine, and a new "Background jobs" page, which the background jobs section moves to unchanged. The page body is split into StatusView and BackgroundJobsView so that SettingsAdmin is left as a shell that fetches the data and picks a view. The two are toggled with v-show rather than v-if on purpose: the CPU and memory charts accumulate their history in the component, so v-if would reset both graphs every time the admin glanced at the job tables. The switch is an NcRadioGroup of NcRadioGroupButton, so it renders real radio inputs with a group label rather than click handlers on divs. Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Monitoring app reports what the database is, never whether it is configured well, and the answer to that already exists in Nextcloud's DB Doctor app. This ports its check engine into a third page, between Status and Background jobs. What came across: - the rule engine (Rule, RuleResult, RuleSet, the expression evaluator and the advisor) and the probe that snapshots the server - the MySQL/MariaDB rule set, ported in DB Doctor from phpMyAdmin's advisor and carrying its GPL-2.0-or-later attribution, plus the PostgreSQL and Nextcloud schema rules - the dialog for pointing the checks at a separate connection, so they can run as a user with rights Nextcloud itself does not need. It stores under serverinfo's own app config, with the password in the credentials manager - the live connections, throughput and latency boxes What deliberately did not: - the score, grade, mascot, history and trend charts. This is a page about what is wrong, not a gamified dashboard - everything that writes. Failing checks offer the config snippet to paste into my.cnf or postgresql.conf, but nothing applies a value: a bundled monitoring app has no business holding write access to the database The checks run once when the page is first opened rather than on a button, and the page keeps its results when you switch tabs and come back. Two departures from how DB Doctor does it. Its dashboard polls /insights/metrics every three seconds and /check/ping once a second; here both are one endpoint that returns the metrics and times its own round trip, because the request cycle costs orders of magnitude more than the queries inside it. And both live endpoints close the PHP session before doing their work: holding it for the length of a five-second check made the Status page's two-second poll queue up behind it, far enough to exhaust the web server's workers. The charts reuse the chart.js setup the CPU and memory graphs already use, rather than porting DB Doctor's own sparkline and latency chart, and they poll only while the database page is the one on screen and the tab is visible. Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The database page reports what is wrong with the database configuration, but only to an admin who knows to open it. Settings > Overview is where they already look for things that need attention, so a failing check now says so there, linking straight to the findings. Running fifty-odd rules against a live server on every Overview load is not reasonable, so the outcome of a run is recorded in app config and the setup check reads that. Opening the database page records a fresh result, which means acting on a finding clears the warning without waiting for anything; a server nobody has looked at for a day re-runs the checks once and records that instead. The link is passed as a rich object parameter rather than through SetupResult's linkToDoc: the Overview does not render linkToDoc at all -- of the twenty-two checks on a stock install, the only one showing a link builds it the way this now does. Following that link needs the page to be addressable, so the admin settings page now reads its view from the URL fragment (#status, #database, #background-jobs) and keeps it in sync with history.replaceState, which leaves the back button alone while switching between them. Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems the database page introduced. A rule whose formula cannot be evaluated was logged as a warning. That is not an exceptional condition: a variable that is not numeric on this particular server is ordinary — MariaDB reports concurrent_insert as AUTO where MySQL reports an integer — and it happens on every run. The effect was a warning written to nextcloud.log every time the page was opened, which then inflated the "Errors in the log" setup check on the same Overview page the database warning was added to. Verified on a MariaDB instance: three runs used to add three lines, and now add none. Nothing is lost by dropping the level. The rule already reports itself as skipped with the reason attached, which is where an admin should read it; the log line was only ever diagnostic detail. The apply-descriptor failure next to it stays a warning, because that one means a rule is written wrong rather than that the server is different. The other problem was two event listeners registered with anonymous handlers and never removed: visibilitychange in the live charts and hashchange on the settings page. Both are now named and taken off on unmount, which is what useLiveData already does. Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat(database): warn about failing database checks on the overview page
fix(database): stop logging per page load and leaking two listeners
Twenty of the fifty-seven ported checks are removed, leaving thirty-seven. Nextcloud already reports missing indices, columns and primary keys on Settings > Overview, so repeating them here only gave an admin two places to read the same finding, and the transaction isolation check went with them. The query cache and MyISAM groups describe engines and features that a current Nextcloud does not use — the query cache was removed outright in MySQL 8.0 — so on a supported server they either skip or advise tuning something nothing touches. The rest are checks whose finding is not actionable: how long the server has been up, whether it has served enough queries to be worth analysing, slow_launch_time, skip_name_resolve, and the two binary-log ones that only matter with replication configured. Removing the schema-integrity rules pays for itself twice over. They were the only consumers of the code that dispatches AddMissingIndicesEvent and friends to every installed app and diffs the result against the live schema through SchemaWrapper, so that work no longer runs on a page load, and the app no longer reaches into OC\DB\SchemaWrapper -- one fewer private server class for a bundled app to depend on. The psalm baseline shrinks with it. It also removes the rule behind the MariaDB log noise fixed in the previous commit, so that fix now has nothing left to suppress on MariaDB. On a MariaDB test instance the page goes from 57 checks with 12 failing to 37 with 5, and the categories MyISAM, QueryCache, Security and Server disappear entirely. Signed-off-by: Frank Karlitschek <frank.karlitschek@nextcloud.com> Assisted-by: Claude Code:claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat(database): drop the checks that do not earn their place
…database-page # Conflicts: # css/serverinfo-main.css # js/serverinfo-main.mjs # js/serverinfo-main.mjs.map # lib/Config/ConfigLexicon.php # src/components/SettingsNavigation.vue # src/types.ts # src/views/SettingsAdmin.vue # src/views/StatusView.vue
Member
Author
|
Closing as redundant: #1123 was merged into master while I was preparing this, and I have verified the merge came out correct — master keeps #1122's failure notices and the database page in The only remaining difference between this branch and master was the ordering of two entries in I also checked master directly: the committed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The whole database feature in one branch, now that #1124, #1125 and #1128 have been merged into it: the page, the Overview setup check, the regression fixes and the rule pruning.
What it adds
A Database page between Status and Background jobs, running the checks ported from Nextcloud's DB Doctor:
my.cnforpostgresql.conf, but nothing writes to the database. A bundled monitoring app has no business holding write access to what it monitors#database, cached so it does not re-run fifty rules on every page loadThe phpMyAdmin attribution and GPL-2.0-or-later provenance travel with the ported rule set.
What it deliberately leaves behind
The score, grade, mascot, history and trend charts — this is a page about what is wrong, not a dashboard to score. And 20 of the original 57 checks: the schema-integrity ones Nextcloud already reports on Overview, everything about the query cache (removed in MySQL 8.0) and MyISAM (Nextcloud runs on InnoDB), and findings an admin cannot act on such as short uptime or
slow_launch_time.Dropping the schema rules removed the only caller of the code that dispatches
AddMissingIndicesEventand friends to every installed app — by far the most expensive thing a check run did — and with it the dependency onOC\DB\SchemaWrapper, one fewer private server class for a bundled app to rely on.Conflict resolution, for review
Four files needed real merging against current master, and it is worth knowing which side won:
StatusView.vuestaticErrorhandling; the database work never touched itSettingsNavigation.vue,types.tsConfigLexicon.phpcached_boot_timeentries pluscached_db_checkSettingsAdmin.vueBuilt bundles were regenerated rather than merged.
Verification
Deployed to a live NC 35 instance with everything merged together:
/database/checkreturns 200 with 37 rules, 5 failing, and the page renders 5 cardsNaN,-1024,1.0 Bor(-1 threads)anywhereeslint,php-cs-fixerand psalm clean; phpunit green (37 tests inLinuxTestalone); the committedjs/cssreproduce from a fresh build.Still worth a maintainer's view
This duplicates DB Doctor's rule engine inside a bundled app, so the two copies will drift. I kept the ported files structurally identical and preserved upstream rule ids so a re-sync stays mechanical, but if you would rather serverinfo deferred to DB Doctor when it is installed, that is a better conversation to have now than after they diverge.