-
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] Support Kibana deprecations #97159
[Upgrade Assistant] Support Kibana deprecations #97159
Conversation
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 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.
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?
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?
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".
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?
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]).
"type": "long" | ||
"type": "long", | ||
"_meta": { | ||
"description": "Number of times a user opened the Elasticsearch cluster deprecations page" |
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 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 = '') => { |
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.
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. :)
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 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 = '') => { |
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: 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'); |
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.
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. 😄
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.
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
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.
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 = { |
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 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));
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 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); |
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.
Similar question here.
|
||
notifications.toasts.addSuccess(i18nTexts.successMessage); | ||
// Refetch deprecations | ||
await getAllDeprecations(); |
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 this await
necessary?
<EuiFlexItem> | ||
<EuiFlexGroup> | ||
<EuiFlexItem grow={false}> | ||
<EuiFieldSearch |
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.
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.
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 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', |
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.
Does the API no longer support this level?
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.
warning is still relevant, but not applicable for the filter
* 2.0. | ||
*/ | ||
|
||
import type { DomainDeprecationDetails } from 'kibana/public'; |
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 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.
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 decision is somewhat related to #87599 (review). I don't think it's suggested to have a root-level __jest__
directory anymore.
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 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.
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.
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.
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.
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.
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 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:
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.
I like this idea! I had not considered that. Agreed it's not ideal for the user to have to click the
👍 Deb will be reviewing the copy.
👍 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
@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 originally had a flyout, but Michael suggested using a modal instead. I can't recall now what the reasoning was 😅. @mdefazio thoughts here?
👍 Similar feedback was recently brought up in demo. I opened #98482 on the core side to add support for this. |
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:
|
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.
Minimal scss changes look good.
@dborodyansky will follow up with a separate issue for UX tweaks.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [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: |
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.
Migrating UX design thread to planning issue.
I'm going to go ahead and merge this. Will address design and copy changes separately. |
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.
@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", |
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.
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.
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 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', |
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 feels like any links to more information should be Learn more links within the description of the 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.
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', |
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.
"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".
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.
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.', |
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.
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}'?", |
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.
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}'", |
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.
Can this be more specific? Again, you're not fixing the plugin, you're fixing a specific instance of a deprecated feature being used.
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 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.', |
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'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.
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:
"<domainId> is using a deprecated feature"
(needs copy review) and the message field is used as the description.config
vs. other Kibana deprecations in the deprecation list ([Core deprecations service] Add deprecationType field #96060)domainId
, so it renders as an empty string in the UI (core team looking into)How to review
Local setup
UA_READONLY_MODE
tofalse
to enable UA.timelion
plugin. (This commit has been reverted so you will need to pull it into your branch to test.)timelion
deprecation, go toapp/timelion
, clickSave
at the top of the page, thenSave entire Timelion sheet
. When you go back to UA, the deprecation should appear in the list.kibana.yml
:xpack.spaces.enabled: false
Summary of changes
critical
level. Depending on the deprecation, a user can also view steps to fix the deprecation or click a button to automatically resolve it.Screenshots
Screenshots