PMM-14109 Improve Advisor UX - #5656
Draft
ademidoff wants to merge 137 commits into
Draft
Conversation
- Implemented MarkCheckResultsRead function in the service layer to update the read state of check results. - Added corresponding API endpoint for marking check results as read. - Introduced new parameters and response structures for the API. - Updated settings model to include Advisor notification settings. - Enhanced environment variable parsing to support new Advisor notification feature. - Added tests for the new functionality to ensure proper behavior.
…verity threshold
…d Advisor Notifications
…y Fields - Introduced CheckSeverity and CheckTriggeredBy types to represent the severity levels and initiators of advisor checks. - Updated CheckResult model to include Severity (CheckSeverity) and TriggeredBy (CheckTriggeredBy) fields. - Modified database schema to accommodate new fields in the check_results table. - Adjusted related services and models to handle the new fields, including changes in the execution flow of advisor checks. - Updated API services to support the new fields in requests and responses. - Enhanced tests to cover new functionality and ensure proper handling of severity and triggered by information.
… components - Introduced new fields: environment, cluster, replication_set, and outcome to CheckResultHistoryItem in advisors.proto. - Updated validation logic in advisors.pb.validate.go to include new fields. - Modified JSON representations in list_check_results_history_responses.go and v1.json to accommodate new fields. - Adjusted database schema and models to reflect new fields in CheckResult. - Enhanced tests to validate new fields and ensure proper functionality in AdvisorInsights components.
…, updating labels, and enhancing test coverage
Re-indent the secure_file_priv script and drop its stale commented-out subcategory; remove an empty labels map from the indexes-larger finding.
percona-ui's theme pins MuiIconButton to a fixed 40x40, so the four action icons needed 166px of content box and were clipped by the 150px column whatever their padding. Unpin the button box instead, which collapses each onto its icon and leaves room to spare. Centre the Actions header too: align='center' only sets text-align, and MRT pushes the head cell's actions box right with margin-left:auto, absorbing the free space before justify-content applies - so stretch the label box and centre inside it. Badge the services action with the number of services the check is disabled for, so a check with disablements is distinguishable at a glance; the count doubles as the signal, so the state is not conveyed by colour alone.
The Script field sat at the editor's 160px minimum, roughly half the height of its counterpart in the details pane, leaving the lower half of the form empty. Let it grow into the leftover space, with alignment pinned to the top so a short script is not centred once the box can outgrow its content. Give it a 300px floor as well: the editor's own minimum lives inside an overflow:auto wrapper, so it never reaches the outer box as a min-content floor, and a long queries list would otherwise squeeze the editor away instead of scrolling the form.
Advisor emails are sent by pmm-managed itself over the SMTP server Grafana is configured with, so Grafana was never in the delivery path - it only stored the recipient list in a contact point named 'PMM Advisor Insights'. That indirection required caching the addresses in settings, refreshing the cache on every settings change, and an authenticated request context to reach Grafana, and it failed silently in two ways: no warning when the contact point was missing, and no way to notice when its addresses had changed. Take the recipients directly instead, as a settings field with its own input on the Advisors tab. Addresses are validated as bare parseable emails, capped at 20, and deduplicated. Enabling notifications without a recipient is now rejected up front, since it could only ever fail silently later; the delivery path still logs if it finds no recipients, because PMM_ENABLE_ADVISOR_NOTIFICATIONS bypasses that validation. Drops GetEmailContactPoint and the whole contact-point reconcile/cache. Also reorders the Settings and ChangeSettingsRequest field declarations by field number - the numbers themselves are unchanged.
Category and Sub category were plain text inputs, so authors had to know the existing values by heart and typos silently created near-duplicate categories. Offer the values already in use through a freeSolo Autocomplete, which still accepts anything typed in. autoSelect is required alongside it: freeSolo alone only commits on Enter, so a typed value would be dropped on save. Options come from the advisors query the list page already loaded, so opening the form costs no extra request.
percona-ui's TextInput forwards only sx from formHelperTextProps, so the *-field-error-message testids these 11 call sites passed never reached the DOM. No test referenced them. Drop the helper and its only exporting file rather than leave props that look wired up but are not. Reported upstream as percona/peak-ui#52.
percona-ui's Table uses :has(), and nwsapi composes an internal querySelector
from the matched element while resolving it. When that element carries a MUI id
generated by React's useId (':r23:'), the selector is not valid raw CSS and
nwsapi throws, escaping jsdom's matchesDontThrow and failing the test. It fires
whenever a MUI input renders on a page containing the Table, which so far forced
an explicit id per input or a {hidden: true} escape hatch in each test.
Real browsers treat an unparseable selector as simply not matching, so restore
that in the test env: swallow SyntaxError from matches/closest/querySelector/
querySelectorAll and return the no-match result. Not fixable in MUI - React
colon-wraps useId values on purpose so they cannot be selectors, and
identifierPrefix is inserted inside the colons.
Note the error is built in nwsapi's own realm, so 'instanceof SyntaxError' is
false; the name has to be checked instead.
The report already broke findings down by severity; technology is the other axis readers scan for, so add it alongside. Insights record the service type they ran against rather than a technology, so the counts are keyed by service type and rendered with the technology names used across the UI and API. Every technology is listed, zero included, so a reader can tell 'no findings' apart from 'not covered by this report'. The order comes from a slice rather than map iteration, which would shuffle the lines between runs. Also drops the stale reference to a Grafana contact point from the send function's doc comment.
…pane The insight details pane reported whether the underlying check is currently enabled, which says nothing about the insight itself - it reflects the check's state now, not when the insight was produced. How the run was triggered is the useful fact, and it is already recorded on the insight. Reuses the TRIGGERED_BY_LABEL map that copy-as-text already renders from, so the pane and the clipboard output cannot drift. Drops the checkEnabled prop and the enabled/disabled labels, which had no other consumer.
Editing a check previously required finding its row in the table, so the details pane was a dead end for the most likely next action. Add Edit as the first button in the pane's action row, mirroring Clone: open the form in edit mode and close the pane behind it. The button is disabled rather than hidden on Percona-shipped checks, so the action stays discoverable and the Source field above explains why it is unavailable.
Move the Read/Unread toggle out of its own column into Actions as an envelope icon - a grey open envelope once read, a blue sealed one while unread - so the Insights and Advisor checks panes read the same way. Drop the "Toggle read" menu item, now duplicated by that icon, and add "Copy URL" below "Copy as text". The copied link keeps the current filters and page: the details pane resolves the insight against the current page of results, so a bare ?insight=<id> can land on a page that does not contain the row. Run and disable a check for the insight's own service only; doing either for every service stays on the Advisor checks page. This adds service_ids to StartAdvisorChecksRequest and threads it down to the target lookup. A service-scoped run also needs a service-scoped registry cleanup, since deleting every result for the check name would drop the findings of the services the run never touched.
The formatted timestamp needs 149px, more than the 128px of content width the column had, so every row wrapped. Rather than widen the column to 190px, drop the cell text to 0.85rem and keep the column at 160px. The font size goes on a span inside the cell, not on the cell itself: percona-ui's Table styles '& .MuiTableCell-body' from the table root, and that descendant selector outranks a cell-level sx.
Order the print() prefix as check -> function:line:col so it reads top-down: the Starlark frame already carries the check name as its position filename, it was just paired with the wrong half. Drop the "thread <check name>:" prefix from the Starlark errors. Every caller already names the check - the test pane shows "Failed to execute check '<name>' on service '<service>'" - so the prefix only pushed the real message further to the right. The thread keeps its name for debugging, it just no longer leaks into the message. Label the test results chip "Error" rather than "Failed", which is reserved for checks that ran and found something.
Three failures in the Unit tests job: - checks: funcs_test asserted the "thread <name>:" prefix that the Starlark errors no longer carry. - management: the StartChecks mock was still set up for one argument after the call gained serviceIDs, so testify panicked on arity. - server: enabling advisor notifications without a recipient became invalid when the recipient list landed, but this test still did exactly that. It fails since that commit, not since the run scoping. Also switch the errors.As assertions in server_test to require.ErrorAs, which testifylint flags in the file either way.
ChangeSettingsRequest takes the recipient list as common.StringArray so an unset list can be told from one cleared to empty, while the Settings response returns a plain repeated string. toPayload round-tripped the response shape back to the API, so protojson rejected the bare array with "proto: syntax error: unexpected token [" and no advisor setting could be saved.
Both panes opened at 60vh unless a row double-click asked otherwise, so a pasted deep link (?details= or ?insight=) landed half-height and had to be maximized by hand every time. Each pane now opens maximized however it was triggered, and 60vh is reached only via the toggle; the choice is per-open and does not carry over. Drops the initialMaximized prop and the page-level state that fed it, and makes the toggle's aria-label follow its state.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Filters the list to the insights of one check, via the existing check_name support on ListInsights. No control was added to the filter bar: the table's first column is the check summary, so with the filter on every row shows the same check, and Clear filters already picks it up. MarkInsightsRead shares its filter object with the list query but takes InsightsFilters, which had no check_name — so the bulk mark-as-read would have ignored the new filter and marked every insight read, defeating the very guard that requires a filter before enabling the button. Added check_name there and wired it through to models.InsightFilters, which already applied it. InsightsFilters is unreleased (absent from main), so its fields are renumbered to put check_name first; no reserved ranges are needed.
The link competed with the row's own click targets and pushed the summary around. It becomes a non-clickable FileOpenOutlined indicator (a document with a north-east arrow) marking rows that have documentation; the working link already lives in the details pane, which the tooltip points at, and copy-as-text still carries the URL. Deliberately not OpenInNew: that glyph is the standard "opens in a new tab" affordance and would promise a click this indicator does not accept.
format-fast used $(FILES), which is empty unless passed explicitly, so a bare `make format-fast` died with "cannot use -w with standard input". $(GO_FILES) falls back to every *.go file, matching how `format` behaves; passing FILES="a.go b.go" still narrows it.
The interval spinner stepped by 0.1, so a click turned 24 into 24.1. step: 1 alone would not have fixed it: a number input anchors its steps to min, so the 0.1 floor made every stepped value fractional and would have left a plain 24 failing the browser's step check. The floor is now 1 hour, and the schema rejects typed fractions the spinner cannot produce. Intervals below an hour are no longer accepted by the form, so an install carrying one must correct that field before the tab can be saved. pmm-managed still accepts any interval down to a second; this is a UI floor only.
FileOpenOutlined's north-east arrow collapsed into something reading as a mouse cursor at the cell's ~16px font size, implying a click the indicator does not accept. DescriptionOutlined is the plain document glyph.
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
2 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.
Ticket number: PMM-14109
Feature build: SUBMODULES-4438