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

[Upgrade Assistant] Support Kibana deprecations #97159

Merged
merged 25 commits into from
Apr 30, 2021

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Apr 14, 2021

This PR adds support for displaying Kibana deprecations in Upgrade Assistant.

UA is consuming the core deprecations service in order to surface these deprecations. Each deprecation should either have manual steps that we display in the UI to fix the deprecation or an API to automatically resolve it. It is possible to have both, although I'm not sure how common this will be.

There are a few limitations of the deprecations service that we are currently looking into:

  • The service only requires a deprecation message and no details field like the ES deprecation info API. UA derives the deprecation title as: "<domainId> is using a deprecated feature" (needs copy review) and the message field is used as the description.
  • We are not yet enforcing config deprecations to provide corrective steps ([Core deprecations service] Require manual corrective steps for config deprecations #96059)
  • Ability to sort by config vs. other Kibana deprecations in the deprecation list ([Core deprecations service] Add deprecationType field #96060)
  • Some deprecations don't have an associated domainId, so it renders as an empty string in the UI (core team looking into)
  • For corrective steps, the service only requires a plugin owner to register an array of strings. It would be ideal to support both a title/description, as well as allow markdown for the description.
  • We may consider bulk actions support, but this work is not yet being tracked.

How to review

Local setup

  1. Edit constants.ts and change UA_READONLY_MODE to false to enable UA.
  2. There are no Kibana plugins that are registering an API action to resolve a deprecation yet. I have mocked this behavior for testing purposes in 480b6cd. This is specific to the timelion plugin. (This commit has been reverted so you will need to pull it into your branch to test.)
  3. To trigger the timelion deprecation, go to app/timelion, click Save at the top of the page, then Save entire Timelion sheet. When you go back to UA, the deprecation should appear in the list.
  4. To trigger a config deprecation, you can disable the spaces plugin in your kibana.yml: xpack.spaces.enabled: false

Summary of changes

  • The overview page now shows the Kibana deprecation stats in addition to Elasticsearch
  • A new Kibana deprecations page, which lists all Kibana deprecations. Users can search, or filter by critical level. Depending on the deprecation, a user can also view steps to fix the deprecation or click a button to automatically resolve it.
  • I extracted some common components for the ES and Kibana deprecation list view - search bar, filter actions, empty prompt, pagination.
  • Updated a11y tests and CITs
  • Added telemetry support for navigating to Kibana deprecations page

Screenshots

Screenshots Screen Shot 2021-04-19 at 10 20 33 AM Screen Shot 2021-04-22 at 2 27 53 PM Screen Shot 2021-04-22 at 2 17 48 PM Screen Shot 2021-04-22 at 2 17 11 PM Screen Shot 2021-04-22 at 2 17 19 PM Screen Shot 2021-04-22 at 4 09 05 PM

@alisonelizabeth alisonelizabeth added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Upgrade Assistant v7.14.0 labels Apr 14, 2021
@cjcenizal cjcenizal requested review from cjcenizal and removed request for jloleysens April 27, 2021 21:02
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Awesome work Alison! I had a lot of very superficial comments and nit-picks. Nothing blocking. Tested locally and everything looks good. Here are a few UX comments.

Deprecations summary

When the counts are 0, can we style them in light gray, or maybe just replace it with a green check icon and the word "None" in gray? Dmitry will probably have a much better idea. The goal is to improve scannability and also to remove the cognitive dissonance of a red 0 which looks bad, but is actually good.

Is there any way to make the entire panel clickable, like EuiCard? Making people acquire this little link introduces a tiny bit of friction, and I think we really want to grease the rails so people can get into these deprecations and start fixing them as smoothly as possible.

image

Empty prompt copy

I think the language here is potentially misleading. "Ready to upgrade!" makes it sound like I'm ready to upgrade, but I might still need to address issues in the other tab. Can we do a copy pass to guide users to the appropriate next step?

image

Markdown support

I might have already mentioned it, so at the risk of sounding like a 🦜 I'll say markdown would let us format code blocks and links really nicely. Maybe we can make an issue to track this with Core?

image

Deprecation toggle feedback

Can we get a writer to take a pass at this? I'd expect to see these imperative statements as button labels, not as state change confirmations. I'm more used to seeing something like "Elasticsearch deprecations will be logged" and "Elasticsearch deprecations won't be logged".

image

Steps modal vs flyout

I think a flyout will give us more real estate to show the user a long list of steps. We can also provide more identifiable information in the header, like the "Critical" badge and whatever else is being surfaced in the table row. We can also surface the "Quick resolve" button in the footer. WDYT?

image

Resolution UX

Before I click "Resolve" I'd like to know what the outcome will be. To be honest I don't really know what happened even after clicking the button. A better UX would be modal content that sets my expectations appropriately (e.g. "Your 5 timelion sheets will be converted into dashboards. You will be able to find them in the Dashboards app, with the prefix 'Converted-timelion' in their name.") and success toast content that confirms those expectations have been met (e.g. "5 timelion sheets converted to dashboards. Find them in the Dashboards app. [Link]).

image

image

"type": "long"
"type": "long",
"_meta": {
"description": "Number of times a user opened the Elasticsearch cluster deprecations page"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: this is technically a tab, not a page. Might be more future-proof to generically say "a user viewed the list of Elasticsearch cluster deprecations." Same idea with the index deprecations, below.

@@ -28,6 +34,61 @@ export interface CheckupTabProps extends UpgradeAssistantTabProps {
checkupLabel: string;
}

export const filterDeps = (level: LevelFilterOption, search: string = '') => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit, I've seen a shift from enums to unions, usually in src/core. Applied here, LevelFilterOption would become:

type LevelFilterOption = 'all' | 'critical';

Then conditions like line 40 becomes a bit terser:

if (level !== 'all') {

I used to be a big fan of enums but have drifted toward unions over time. I don't feel strongly either way though so happy to leave this as-is. Just wanted to share an alternative. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 This was existing behavior. I've changed it to a union. There are several other places in the code where enums are still being used, but I think this can be addressed on a case-by-case basis.

@@ -28,6 +34,61 @@ export interface CheckupTabProps extends UpgradeAssistantTabProps {
checkupLabel: string;
}

export const filterDeps = (level: LevelFilterOption, search: string = '') => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This name sounds like function is responsible for filtering the dependencies. But it's really for creating a filtering function. How about naming it createDependenciesFilter?

if (search.length > 0) {
conditions.push((dep) => {
try {
const searchReg = new RegExp(search, 'i');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we only searching if a string includes another string, or is there more to this search? If the former, then dep.message.toLowerCase().includes(search.toLowerCase()) seems easier to read and understand to me. Not sure if everyone knows what 'i' means in this context. But this is like.... the nittiest nitpick ever so I'm also fine leaving this as-is. 😄

Copy link
Member

@Bamieh Bamieh Apr 28, 2021

Choose a reason for hiding this comment

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

That is true includes might be slightly easier to read. Converting both strings toLowerCase makes me feel uneasy thats all :d

I'd be surprised if we have devs not familiar with basic regular expressions writing code in our code base!

I'm impartial on what you decide on. Just sharing my thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both for your feedback! I've decided to leave as-is, but left a brief code comment for anyone who might be unsure what i is used for.

/**
* Collection of calculated fields based on props
*/
const calcFields = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this design adds some unnecessary complexity in a few ways:

  • Attaching the helper functions to the calcFields namespace object introduces more information but I'm not seeing any benefits -- maybe I'm overlooking them?
  • The helper functions are named like nouns, not verbs, which seems unusual. Was this deliberate?
  • The functions expect a props config object. Yet at their call-sites we're destructuring the component's props and then re-assembling them into objects to provide as arguments, to satisfy this expectation. Seems like unnecessary work.

I'd suggest simplifying these by removing the namespacing object, naming the functions with verbs, and refactoring the parameters to accept individual arguments, for example:

const filterDeprecations = (currentFilter: LevelFilterOption, search: string, deprecations?: EnrichedDeprecationInfo[] = []) =>
  deprecations.filter(filterDeps(currentFilter, search));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 This was pre-existing code. Agreed this can be simplified and made more clear. I've updated the implementation.


const toggleResolveModal = (newResolveModalContent?: DomainDeprecationDetails) => {
if (typeof newResolveModalContent === 'undefined') {
setResolveModalContent(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here.


notifications.toasts.addSuccess(i18nTexts.successMessage);
// Refetch deprecations
await getAllDeprecations();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this await necessary?

<EuiFlexItem>
<EuiFlexGroup>
<EuiFlexItem grow={false}>
<EuiFieldSearch
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider using EuiSearchBar? I haven't looked into whether it will work in this use case, so I'm just mentioning it for awareness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! I had not. This implementation was mostly carried over from the previous UI. I will make a note to look into this and consider updating in a follow-up PR.

@@ -35,7 +35,6 @@ export enum LoadingState {
export enum LevelFilterOption {
all = 'all',
critical = 'critical',
warning = 'warning',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the API no longer support this level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

warning is still relevant, but not applicable for the filter

* 2.0.
*/

import type { DomainDeprecationDetails } from 'kibana/public';
Copy link
Contributor

Choose a reason for hiding this comment

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

I love seeing all these tests! I didn't notice them at first because they're in tests_client_integration and I'm used to seeing them inside a root-level __jest__ directory. What was the intention behind using a different directory name? It might be easier for people to discover them if we follow the __jest__ convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this decision is somewhat related to #87599 (review). I don't think it's suggested to have a root-level __jest__ directory anymore.

Copy link
Contributor

@cjcenizal cjcenizal Apr 28, 2021

Choose a reason for hiding this comment

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

Thanks Alison! I traced that issue you linked to back to #85513, where it looks like the intent was to move tests out of __tests__ directories. Have you heard something regarding the use of __jest__ directories? Just trying to understand the decision and background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch! I might have confused the two. Let me follow up with the operations team to confirm. The directory name wasn't made as part of this PR so I think it's OK to address separately if needed.

Copy link
Contributor Author

@alisonelizabeth alisonelizabeth Apr 29, 2021

Choose a reason for hiding this comment

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

Perhaps this is worth discussing at our next team sync (I'll add it to the agenda). Core's recommendation is to put jest tests alongside source files, with the exception of jest "integration tests", which should live in a folder called integration_tests and run via node scripts/jest_integration. However, this is intended for any jest tests that use an external service (ES/Kibana) as part of the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

++

@alisonelizabeth
Copy link
Contributor Author

Thanks for the review and great feedback, @cjcenizal! I'm still working through some of your code comments and will respond to each one when I've addressed it.

In regards to your UX comments:

When the counts are 0, can we style them in light gray, or maybe just replace it with a green check icon and the word "None" in gray? Dmitry will probably have a much better idea. The goal is to improve scannability and also to remove the cognitive dissonance of a red 0 which looks bad, but is actually good.

This is a great point. I'll work with @dborodyansky on improving this. I will likely address this in a separate PR since this one did not specifically introduce the concept of stats panels.

Is there any way to make the entire panel clickable, like EuiCard? Making people acquire this little link introduces a tiny bit of friction, and I think we really want to grease the rails so people can get into these deprecations and start fixing them as smoothly as possible.

I like this idea! I had not considered that. Agreed it's not ideal for the user to have to click the View deprecations link as-is. Similar to my comment above, I'd like to address this separately.

I think the language here is potentially misleading. "Ready to upgrade!" makes it sound like I'm ready to upgrade, but I might still need to address issues in the other tab. Can we do a copy pass to guide users to the appropriate next step?

👍 Deb will be reviewing the copy.

I might have already mentioned it, so at the risk of sounding like a 🦜 I'll say markdown would let us format code blocks and links really nicely. Maybe we can make an issue to track this with Core?

👍 This is on core's radar. A similar request has come up for another new service they're working on and are exploring ideas (notifications I believe?), so I didn't open up an issue yet. /cc @Bamieh

Can we get a writer to take a pass at this? I'd expect to see these imperative statements as button labels, not as state change confirmations. I'm more used to seeing something like "Elasticsearch deprecations will be logged" and "Elasticsearch deprecations won't be logged".

@debadair would you mind taking another pass at this? CJ included screenshots in his original comment, but feel free to reach out if you need any additional context.

I think a flyout will give us more real estate to show the user a long list of steps. We can also provide more identifiable information in the header, like the "Critical" badge and whatever else is being surfaced in the table row. We can also surface the "Quick resolve" button in the footer. WDYT?

I originally had a flyout, but Michael suggested using a modal instead. I can't recall now what the reasoning was 😅. @mdefazio thoughts here?

Before I click "Resolve" I'd like to know what the outcome will be. To be honest I don't really know what happened even after clicking the button. A better UX would be modal content that sets my expectations appropriately (e.g. "Your 5 timelion sheets will be converted into dashboards. You will be able to find them in the Dashboards app, with the prefix 'Converted-timelion' in their name.") and success toast content that confirms those expectations have been met (e.g. "5 timelion sheets converted to dashboards. Find them in the Dashboards app. [Link]).

👍 Similar feedback was recently brought up in demo. I opened #98482 on the core side to add support for this.

@alisonelizabeth
Copy link
Contributor Author

Pending copy and design review, I believe I've addressed all feedback up to this point. A few suggestions related to the UX I will explore outside of this PR and address separately:

  • Consider using EuiTable component instead of a custom list component with EuiAccordion for listing deprecations
  • Support ability for a user to choose page count (currently hard-coded to 25)
  • Improvements to the stats panels
    • Consider using EuiCard to make entire panel clickable
    • Better UX for when there are 0 deprecations
  • Reconsider use of flyout vs. modal for showing Kibana manual steps
  • Once [Core deprecations service] Add description field to API action #98482 is implemented, add details to Kibana API action

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Minimal scss changes look good.

@dborodyansky will follow up with a separate issue for UX tweaks.

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
upgradeAssistant 72 87 +15

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
upgradeAssistant 119.5KB 146.4KB +26.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
upgradeAssistant 30.2KB 29.3KB -892.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
upgrade-assistant-telemetry 13 14 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@dborodyansky dborodyansky left a comment

Choose a reason for hiding this comment

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

Migrating UX design thread to planning issue.

@alisonelizabeth
Copy link
Contributor Author

I'm going to go ahead and merge this. Will address design and copy changes separately.

Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

@alisonelizabeth I left some general comments, but have a few more specific ones to add. (In particular, I'd init cap Critical and Warning."

const i18nTexts = {
getDeprecationTitle: (domainId: string) => {
return i18n.translate('xpack.upgradeAssistant.deprecationGroupItemTitle', {
defaultMessage: "'{domainId}' is using a deprecated feature",
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a collapsible list of non-specific titles isn't very helpful--especially since one plugin might have multiple deprecation entries. This & the "Fix xyz" terminology also makes it sound like you need to fix the plugin itself.

Is there any way to surface what the actual problem is at this point? Something like:

Reporting: The xpack.reporting.roles setting is deprecated.

Also, showing the raw domainId strings isn't very user-friendly--especially for the "xpack" plugins, because xpack doesn't really mean much anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 This feedback has come up a few times. The deprecations service currently only supports providing a description, which is quite long in some cases. I've opened #99625 so that a title can also be provided and it won't be hard-coded in UA.

});
},
docLinkText: i18n.translate('xpack.upgradeAssistant.deprecationGroupItem.docLinkText', {
defaultMessage: 'View documentation',
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like any links to more information should be Learn more links within the description of the deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this may have been carried over from the ES deprecations list, where there may be one deprecation type that affects several indices, and therefore we only link to the docs once. @dborodyansky is doing an audit of the ES deprecations page vs. Kibana and making sure both UIs are aligned. I'm going to hold off on making any changes here until then.

manualFixButtonLabel: i18n.translate(
'xpack.upgradeAssistant.deprecationGroupItem.fixButtonLabel',
{
defaultMessage: 'Show steps to fix',
Copy link
Contributor

Choose a reason for hiding this comment

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

"Show steps to fix" and "Quick resolve" are an awkward pairing.

Instead of a generic "Quick resolve" label, is it possible to provide an indication of what's going to happen/what you need to do? For example, "Migrate worksheets". If there's a way to do it automatically, the next prompt could list what's going to happen and include the Quick resolve button as CJ suggested.

Unless it's really clear what's going to happen, I think people will be reluctant to choose "Quick resolve".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point.

I opened #98482, which will allow the consumer to define an explanation of what the "automatic fix" will do. However, I don't think this would be applicable to the button label, and I'm also not sure it makes sense to add another field for the user to define something like this.

Do you have any other suggestions? Once #96060 is implemented, we will know whether the deprecation is config- or feature-related. I'm not sure if that would help here or not.

Also, in #97159 (review), CJ suggested co-locating the manual steps and "quick resolve" button in the same flyout. In we went in this direction, I think there would only be a single initial entry point. /cc @dborodyansky

defaultMessage: 'Kibana',
}),
pageDescription: i18n.translate('xpack.upgradeAssistant.kibanaDeprecations.pageDescription', {
defaultMessage: 'Some Kibana issues may require your attention. Resolve them before upgrading.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Review the issues listed here and make the necessary changes before upgrading.
Critical issues must be resolved before you upgrade.

i18n.translate(
'xpack.upgradeAssistant.kibanaDeprecations.resolveConfirmationModal.modalTitle',
{
defaultMessage: "Resolve '{domainId}'?",
Copy link
Contributor

Choose a reason for hiding this comment

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

Resolving seems really weird. Maybe Resolve deprecation in ? (I don't really like "resolve deprecation" either, but that's how we talk about instances of using deprecated features.

const i18nTexts = {
getModalTitle: (domainId: string) =>
i18n.translate('xpack.upgradeAssistant.kibanaDeprecations.stepsModal.modalTitle', {
defaultMessage: "Fix '{domainId}'",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be more specific? Again, you're not fixing the plugin, you're fixing a specific instance of a deprecated feature being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently. For now, I could follow the same suggestion in #97159 (comment).

modalDescription: i18n.translate(
'xpack.upgradeAssistant.kibanaDeprecations.stepsModal.modalDescription',
{
defaultMessage: 'Follow the steps below to address this deprecation.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use resolve instead of introducing another word for fix. Again, it would be better if this could be specific, rather than a generic intro. If it has to be generic, "To resolve this deprecation:" or, maybe just omit the intro entirely? It doesn't really add anything.

@alisonelizabeth
Copy link
Contributor Author

@debadair thanks for the copy review! I've addressed some of your feedback in #99632. I few of your comments will require changes to the Kibana deprecations service and I will need to address separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants