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] Add deprecation logging as step on overview page #126277

Merged
merged 12 commits into from
Mar 2, 2022

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Feb 23, 2022

Fixes #125842

This PR makes the deprecation logging feature more prominent in Upgrade Assistant by adding it as its own separate step on the Overview page.

Screenshots

// Deprecation logging enabled
Screen Shot 2022-02-23 at 9 22 54 AM

// Deprecation logging disabled
Screen Shot 2022-02-23 at 9 23 07 AM

// Error state
Screen Shot 2022-02-23 at 9 22 30 AM

// Missing privileges
Screen Shot 2022-02-23 at 9 42 56 AM

// New test coverage
Screen Shot 2022-02-23 at 1 12 54 PM

@alisonelizabeth alisonelizabeth added release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Upgrade Assistant v7.17.1 labels Feb 23, 2022
@alisonelizabeth alisonelizabeth marked this pull request as ready for review February 24, 2022 15:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-deployment-management (Team:Deployment Management)

setIsComplete(false);
}

setIsComplete(logsCount?.count === 0);
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 that we're bubbling this state up to the steps UI. If this step is marked as incomplete, does it block the user from upgrading? If so, then I'm concerned about users being blocked from upgrading in the event an Elastic-triggered log shows up and they have no way to fix it. Maybe I'm just being paranoid, but given the trouble we had with these logs originally I think we might want to err on the side of caution.

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 didn't realize we were blocking the upgrade on cloud, but now I see that we are. I will remove this.

Copy link
Contributor Author

@alisonelizabeth alisonelizabeth Feb 24, 2022

Choose a reason for hiding this comment

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

Actually, on further review, I see that we are calling the status route to determine whether or not the upgrade button is disabled. This takes into account any critical deprecations from ES or Kibana, and whether the system indices migration is needed. It does not check deprecation logs.

With that said, I think the current approach is OK to leave as-is.

Copy link
Contributor

@cjcenizal cjcenizal Feb 25, 2022

Choose a reason for hiding this comment

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

👍 Thanks for looking into it. Just wanted to make sure the actual button in UA that takes you to Cloud isn't disabled by the incomplete state of the step. But I believe that button is enabled/disabled based on the Status API, right? That's literally what you wrote. 🤦 Thanks!

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@alisonelizabeth alisonelizabeth requested review from sabarasaba and removed request for sebelga February 28, 2022 14:12
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes @alisonelizabeth! code lgtm, tested locally. Left one small question 🚀


<EuiSpacer />

<EuiButton onClick={navigateToEsDeprecationLogs} data-test-subj="viewLogsLink">
Copy link
Member

Choose a reason for hiding this comment

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

if a user doesn't have index privileges, the only thing they can do in the es deprecation logs page is enable/disable log collection right? Wondering if it's worthy to still show this CTA in that case 🤔

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 correct. I was on the fence for this as well. I'm going to go ahead and remove it.

I looked back at the previous code when it was part of the deprecations step, and I see we weren't showing the sentence/link to the deprecation logs when a user did not have privileges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-03-01 at 9 36 26 AM

@sebelga
Copy link
Contributor

sebelga commented Mar 1, 2022

I am wondering if the title of the step shouldn't be simply "Address Elasticsearch deprecations" or "Address API deprecations"
The facts that the deprecation are surfaced in the logs is an implementation information. They could be shown in a table 😊 WDYT?

@alisonelizabeth
Copy link
Contributor Author

I am wondering if the title of the step shouldn't be simply "Address Elasticsearch deprecations" or "Address API deprecations"
The facts that the deprecation are surfaced in the logs is an implementation information. They could be shown in a table 😊 WDYT?

Thanks for the feedback @sebelga! That's a good point.

@jrodewig would you mind weighing in here on the copy?

@jrodewig jrodewig added the ui-copy Review of UI copy with docs team is recommended label Mar 1, 2022
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Thanks for the ping. This looks good overall, but I think the missing privileges error needs some work.

I also left some other non-blocking suggestions.


const i18nTexts = {
logsStepTitle: i18n.translate('xpack.upgradeAssistant.overview.logsStep.title', {
defaultMessage: 'Address deprecation logs',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly about it, but I like @sabarasaba's suggestion here.

Suggested change
defaultMessage: 'Address deprecation logs',
defaultMessage: 'Address API deprecations',

}),
logsStepDescription: i18n.translate('xpack.upgradeAssistant.overview.logsStep.description', {
defaultMessage:
'Review the Elasticsearch deprecation logs to make sure you are not using deprecated APIs.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor wording nit.

Suggested change
'Review the Elasticsearch deprecation logs to make sure you are not using deprecated APIs.',
'Review the Elasticsearch deprecation logs to ensure you're not using deprecated APIs.',

enableLogsButtonLabel: i18n.translate(
'xpack.upgradeAssistant.overview.logsStep.enableLogsButtonLabel',
{
defaultMessage: 'Enable logs',
Copy link
Contributor

Choose a reason for hiding this comment

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

"Enabled logging" sounds a bit more natural to me.

Suggested change
defaultMessage: 'Enable logs',
defaultMessage: 'Enable logging',

}
),
loadingError: i18n.translate('xpack.upgradeAssistant.overview.logsStep.loadingError', {
defaultMessage: 'An error occurred while retrieving the count of deprecation logs',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defaultMessage: 'An error occurred while retrieving the count of deprecation logs',
defaultMessage: 'An error occurred while retrieving the deprecation log count',

missingPrivilegesTitle: i18n.translate(
'xpack.upgradeAssistant.overview.logsStep.missingPrivilegesTitle',
{
defaultMessage: 'You require index privileges to analyze the deprecation logs',
Copy link
Contributor

@jrodewig jrodewig Mar 1, 2022

Choose a reason for hiding this comment

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

This wording is a little awkward to me, and I don't think it stands on its own.

Can we add a description that includes the required index privileges and the indices you need them on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-03-02 at 9 13 33 AM

@alisonelizabeth
Copy link
Contributor Author

alisonelizabeth commented Mar 2, 2022

Thanks for the review @jrodewig! I addressed your feedback in bf592ed.

Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

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

Copy changes LGTM. Thanks @alisonelizabeth.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
upgradeAssistant 209 211 +2

Async chunks

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

id before after diff
upgradeAssistant 182.1KB 185.5KB +3.4KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Upgrade Assistant release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more ui-copy Review of UI copy with docs team is recommended v7.17.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants