-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Upgrade Assistant] New ES deprecations page #107053
[Upgrade Assistant] New ES deprecations page #107053
Conversation
3b3e9be
to
32ac42e
Compare
…_deprecation_design_updates
@@ -0,0 +1,142 @@ | |||
/* |
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.
This file was largely unchanged from es_deprecations/deprecations/reindex/flyout/container.tsx
@@ -0,0 +1,187 @@ | |||
/* |
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.
Refactoring of polling_service.ts
. Work initially started as part of #99245.
@dborodyansky - I have a few outstanding design questions 😄.
|
…_deprecation_design_updates
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.
Telemetry and Core changes LGTM!
@elastic/infra-telemetry Heads up with the removal of some fields and addition of another.
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.
UX review
Here's a UX review based on testing the PR locally. I'll provide a code review next.
Lost overview information
As I mentioned to @dborodyansky it feels strange to lose pertinent information as we navigate from the Overview page to this page. I'd expect to see a top-level count of critical and non-critical issues. Once we add support for reindexing progress to the Overview page, we should surface it on this page too if it's useful.
Cancelling a reindex looks like an error
If you cancel a reindex, there's a red "x" rendered for the step. I'd expect to see a "Reindex canceled" toast and everything revert to the original state, instead.
Icon positioning
Small nitpick, but I'd expect the "automatable" and "check" icons to both be on the left side in the table rows.
Slight disconnect between reindex callout and acceptable changes
The callout refers to "accepting each change", but the user is left to infer that means checking the boxes of the items below. I think this could be solved as simply as adding a header that says "Changes" or "Accept changes" above the callout.
resolutionTooltipLabel: i18n.translate( | ||
'xpack.upgradeAssistant.esDeprecations.indexSettings.resolutionTooltipLabel', | ||
{ | ||
defaultMessage: 'This deprecation requires removing index settings from an index to resolve.', |
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'll defer to @debadair if she has a different opinion, but I think this will flow better if we phrase it as guidance: "Resolve this deprecation by...". I think we should also work in the word "automatic" to explain the meaning of the icon. In this case I'd suggest: "Resolve this deprecation by removing settings from this index. This is an automated resolution."
Same suggestion for the other resolution tooltips.
Thanks @cjcenizal
++ This is Dmitry's original design: As mentioned, this doesn't appear to be supported yet in EUI within the Search Bar component. If we feel strongly this is the best approach, I can reach out and see what our options are.
I did not focus on the reindex UX as part of this PR. Will make a note of this to follow up on /cc @yuliacech
👍 |
@alisonelizabeth I also noticed that if you upgrade an ML snapshot, the flyout doesn't surface anything to indicate an upgrade is in progress, and you're still able to click the "Delete" and "Upgrade" buttons. It's not a big deal, it's just a bit of a rough edge in the UX if this upgrade ends up taking loner than expected. |
Is the checkup_api_response.json fixture still being used? I can't see it being referenced anywhere and I wonder if we can remove it. |
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.
🥂 Great work on this @alisonelizabeth! I had some questions, comments, and suggestions, but no blockers. Personally, I would like to see the file names mirror the names of the code being exported, e.g. table_row.tsx
-> reindex_table_row.tsx
, but it's NBD.
|
||
import { ElasticsearchTestBed, setupElasticsearchPage, setupEnvironment } from './helpers'; | ||
|
||
describe('Elasticsearch deprecations', () => { |
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.
This file is large enough I'm finding it difficult to navigate. What do you think of breaking it up into separate files, similar to what we did with ILM?
In this case I'd suggest breaking it up into 6 files with names that will let me skim the files and get a sense of what's covered and where:
deprecations_table.tsx
to test table states and interaction, including search bar, pagination, and empty stateml_snapshots_deprecation_flyout.tsx
to test states of and interaction with the ML Snapshots deprecationindex_settings_deprecation_flyout.tsx
to test states of and interaction with the index settings deprecationreindex_deprecation_flyout.tsx
to test states of and interaction with the reindex deprecation -- this is currently light on tests but I expect it will growdefault_deprecation_flyout.tsx
to test the content of the default deprecation -- this is currently light on tests but maybe it could grow?error_handling.tsx
As part of a subsequent scope we could also explore breaking up the helpers
files and co-locating them with the 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.
Tests refactored in 037ebb7
test('renders deprecations', () => { | ||
const { exists, find } = testBed; | ||
// Verify container exists | ||
expect(exists('esDeprecationsContent')).toBe(true); |
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.
Minor suggestion: abstracting these behind helpers like hasPageContainer
and getTableRows
can make the tests easier to read and reduce the need for explanatory comments.
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.
👍 going to leave as-is for now, will consider refactoring in subsequent PR
// Reopen the flyout | ||
await actions.clickMlDeprecationAt(0); | ||
|
||
// Flyout actions should not be visible if deprecation was 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.
Thanks for adding all of these great comments! Super-helpful.
}); | ||
}); | ||
|
||
describe('reindex deprecation', () => { |
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.
Should we add a comment here that the reindexing UX is largely tested by unit tests colocated with the reindex flyout? If we have plans to migrate those tests to CITs it might be mentioning that here as TODO
, 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.
Thanks for calling this out! I meant to do that actually. I only added one test case for the reindexing flyout, although I think more CITs are warranted. I'd like to treat reindexing as a separate piece of work and add more tests at that time, as I expect the UX to change and some of the existing code to be refactored.
'.euiFilterSelect__items .euiFilterSelectItem' | ||
); | ||
|
||
expect(clusterTypeFilterButton).not.toBe(null); |
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.
FWIW Jest publishes a toBeNull
assertion which will surface "nicer error messages" according to the docs: https://jestjs.io/docs/expect#tobenull
}, [api, indexName, reindexState, updateStatus]); | ||
|
||
const cancelReindex = useCallback(async () => { | ||
await api.sendReindexTelemetryData({ stop: true }); |
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.
Same here.
@@ -125,6 +125,37 @@ export class ApiService { | |||
method: 'get', | |||
}); | |||
} | |||
|
|||
public async sendReindexTelemetryData(telemetryData: { [key: string]: boolean }) { | |||
const result = await this.sendRequest({ |
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.
Is it worth wrapping the telemetry requests in a try/catch
so we can ignore any errors? Seems like we wouldn't want to disrupt the UX in any way if these fail.
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 don't think it's necessary since the send_request
util is already wrapping the request in try/catch
.
|
||
return Object.keys(deprecations).reduce((combinedDeprecations, deprecationType) => { | ||
if (deprecationType === 'index_settings') { | ||
combinedDeprecations = [...combinedDeprecations, ...indices]; |
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.
The intentions here might be clearer if we use concat
:
combinedDeprecations = combinedDeprecations.concat(indices);
} | ||
); | ||
|
||
combinedDeprecations = [...combinedDeprecations, ...enrichedDeprecationInfo]; |
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.
Same here.
cluster, | ||
indices, | ||
totalCriticalDeprecations: criticalWarnings.length, | ||
deprecations: combinedDeprecations.sort(sortByCritical), |
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.
Just curious, why are we sorting on the server if we also sort on the client?
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.
Awesome work @alisonelizabeth! Tested locally and seems to work well. I left a few nits and suggestions but no blockers. I do would like to see in a subsequent PR the elasticsearch.test.ts
being split into smaller files, was a bit hard to follow everything though the comments you left were super helpful!
@@ -200,6 +200,7 @@ export class DocLinksService { | |||
deprecationLogging: `${ELASTICSEARCH_DOCS}logging.html#deprecation-logging`, | |||
setupUpgrade: `${ELASTICSEARCH_DOCS}setup-upgrade.html`, | |||
releaseHighlights: `${ELASTICSEARCH_DOCS}release-highlights.html`, | |||
deprecationInfo: `${ELASTICSEARCH_DOCS}migration-api-deprecation.html`, |
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 think this link isn't being used anymore
} | ||
), | ||
closeButtonLabel: i18n.translate( | ||
'xpack.upgradeAssistant.esDeprecations.deprecationDetailsFlyout.cancelButtonLabel', |
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.
nit: should this be closeButtonLabel
instead?
removeButtonLabel: i18n.translate( | ||
'xpack.upgradeAssistant.esDeprecations.removeSettingsFlyout.removeButtonLabel', | ||
{ | ||
defaultMessage: 'Remove deprecated settings', |
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.
nit: maybe we should support singular/plural form in these strings
import { DEPRECATION_TYPE_MAP } from '../constants'; | ||
|
||
const i18nTexts = { | ||
criticalBadgeLabel: i18n.translate( |
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.
doesn't seem this one is being used anymore
'This cluster is using {clusterCount} deprecated cluster settings and {indexCount} deprecated index settings', | ||
getWarningDeprecationMessage: (warningDeprecations: number) => | ||
i18n.translate('xpack.upgradeAssistant.esDeprecationStats.warningDeprecationsTooltip', { | ||
defaultMessage: 'This cluster has {warningDeprecations} non-critical deprecations', |
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.
nit: maybe we should support singular/plural form
await actions.clickMlDeprecationAt(0); | ||
|
||
// Flyout actions should not be visible if deprecation was resolved | ||
expect(find('mlSnapshotDetails.upgradeSnapshotButton').length).toBe(0); |
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.
nit: maybe would be more semantically correct if we wrote this like:
expect(exists('mlSnapshotDetails.upgradeSnapshotButton')).toBe(false);
expect(exists('mlSnapshotDetails.deleteSnapshotButton')).toBe(false);
const { find, actions } = testBed; | ||
|
||
expect(find('esDeprecationsPagination').find('.euiPagination__item').length).toEqual( | ||
Math.round(deprecations.length / 50) // Default rows per page is 50 |
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.
nit: seems kibana deprecations table defaults to 25 (defined in the constants file) but es deprecations table to 50. maybe we should make them default to the same number? and import this DEPRECATIONS_PER_PAGE
value in here 🤔
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.
Good catch! After an initial design review with CJ and Dmitry, we decided to change the default number of pages to 50
. I'll update the Kibana deprecations to align as part of #109150.
Thanks for the review @cjcenizal and @sabarasaba! I believe I addressed all of your comments via d253ca0 and 037ebb7. There are a few items I’d like to follow up on in a separate PR:
|
@cjcenizal I fixed the issues with the buttons. You bring up a good point though about surfacing progress somewhere. I think this should be addressed separately. @dborodyansky do you have any thoughts here? Here's a screenshot of the flyout we're discussing - the "upgrade" option could potentially be a long-running task, so if the user reopened the flyout while the upgrade was still in progress it'd be nice to indicate that somewhere. |
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.
Should each "resolution" type have a different icon? I'm currently using the indexSetting icon for all, but I'm not sure that makes sense.
Small nitpick, but I'd expect the "automatable" and "check" icons to both be on the left side in the table rows.
Per follow up discussions, converging on left aligned uniform icons and subdued Manual
type, as in the following design:
Should the "resolution" type display along with the status (when applicable)? I currently only display the status in this scenario, but can make changes.
Status replacing resolution type works well in my opinion.
The "upgrade" option could potentially be a long-running task, so if the user reopened the flyout while the upgrade was still in progress it'd be nice to indicate that somewhere.
Same as with reindexing, setting isLoading
on the button should give appropriate feedback while process is running.
I'd expect to see a top-level count of critical and non-critical issues. Once we add support for reindexing progress to the Overview page, we should surface it on this page too if it's useful.
Following pattern elsewhere in Kibana, proposing EuiHealth
for tally display, in lieu of ability to display counts in filters:
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
Thanks @dborodyansky for the design feedback! I'm going to go ahead and merge this PR as-is (as it's quite large 😅 ). I've added your comments to the UA planning issue and plan to address early next week. |
# Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/upgrade_assistant/public/application/components/es_deprecations/deprecations/reindex/button.tsx # x-pack/plugins/upgrade_assistant/public/application/components/es_deprecations/deprecations/reindex/flyout/container.tsx
* [Upgrade Assistant] New ES deprecations page (#107053) # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/upgrade_assistant/public/application/components/es_deprecations/deprecations/reindex/button.tsx # x-pack/plugins/upgrade_assistant/public/application/components/es_deprecations/deprecations/reindex/flyout/container.tsx * fix linting
This PR updates the ES deprecations page in Upgrade Assistant to align with the new design prototype.
It supports 4 different deprecation "types":
This is representative in the code under
es_deprecations/deprecation_types
.Technical implementation details:
With the new design, we need to be able to track the status of ML and reindex actions at the row level. This logic was previously encapsulated within a button component (example). This is no longer possible since we need to open the flyout via the "Issue" column and also provide the status of the resolution via the "Resolution" column. In order to achieve this, I built a custom table using EUI components rather than
EuiMemoryTable
that wraps each applicable row with a context provider. Long term, I think it's worth evaluating if we can keep track of all ML and reindex statuses in a single context, rather than per index name and per ML job ID. However, this requires a fairly significant refactoring that I think is out of scope for this PR.How to test
Unfortunately, this can only be tested on
master
with mocked data. That said, I have created a parallel branch based on7.x
that can be used to test the changes against real deprecations.yarn es snapshot -E path.data=/path/to/6.8.16-data
. This contains indices and ML job model snapshots created on 6.8. This will trigger the reindex and ML deprecations.yarn start
.Deferred work
The following work will be completed in separate PRs:
resolve_during_rolling_upgrade
field - waiting on more information from the ES team about thisScreenshots
Overview of changes
// Reindex flyout
// Deprecated index settings flyout
// Default flyout (deprecation does not have automatic resolution)
// ML flyout
// Some resolution states
// Tooltip on Resolution type
// No deprecations
// Invalid search
// Initial loading state
// No search results