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

[Reporting] New UI for migrating reporting indices ILM policy #103853

Merged
merged 18 commits into from
Jul 5, 2021

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Jun 30, 2021

Summary

This contribution adds new UI to the reporting plugin for guiding users to migrate their reporting indices to the reporting-provided ILM policy. Introduced in #103850.

How to test

  1. Start Kibana + ES on basic and activate your trial to access all features
  2. In Kibana's start up logs you should see Creating ILM policy for managing reporting indices... if this is the first time starting with the new changes. Subsequent starts should contain a debug callout that this step is being skipped.
  3. Navigate to the "Reporting" plugin, on a fresh start this page should be empty, no callout present.
  4. Create a report by going to the "dashboard" app and using the "Share" menu to generate a PDF report
  5. Navigate to the "Reporting" plugin, there should still be no callout present since all of your indices are up-to-date
  6. Run the following request, unsetting the ILM policy managing reporting indices:
curl --request PUT \
  --url 'http://localhost:9200/.reporting-*/_settings' \
  --header 'Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==' \
  --header 'Content-Type: application/json' \
  --data '{
	"settings": {
		"index.lifecycle.name": null
	}
}'
  1. Go to the reporting plugin, the callout to migrate indices should be present (as well as the link to the kibana-reporting ILM policy in the bottom right), clicking it should migrate all reporting indices to use the reporting ILM policy.
  2. Re-run the request to unset the ILM policies and delete the kibana-reporting ILM policy, can be done through the ILM policy UI
  3. Navigate to the Reporting UI, this time the banner should be present and the link to the ILM policy should be removed
  4. Press the primary button in the call-to-action
  5. The callout should be gone and the link to the ILM policy should be back (clicking this link takes you to the ILM UI)

Notes

  • There was jest snapshot that was captured against UI that was in "loading" state. The changes introduced here await the end of the loading state and so the snapshot is populated with a lot more UI than before. This is the primary source of new lines.

Screenshots

Call-to-action to migrate reporting indices

Screenshot 2021-06-29 at 15 55 30

Successful migration of reporting indices

  • Note the success callout
  • Note the link to "Edit ILM policy" in the bottom right

Screenshot 2021-06-29 at 15 55 56

ILM policy exists, but migration is still needed

Screenshot 2021-06-29 at 15 56 27

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
We could overwrite an existing ILM policy Low Med This requires that a user has a policy called kibana-reporting and that there is a bug in our code that will re-create the policy. Further mitigations will be achieved by adding a functional API test

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens jloleysens marked this pull request as ready for review June 30, 2021 13:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

…-policy-2

* 'master' of github.com:elastic/kibana:
  [Reporting] Reintroduce "ILM policy for managing reporting indices" (elastic#103850)
  [Security Solution][Endpoint] Allow activity log scrolling on small screens (elastic#103852)
  Allow zero (0) to unset unenroll_timeout field (elastic#103790)
  [TSVB] Metric count is depicted as `-` instead of 0 (elastic#103717)
  [Query] Es query/field base (elastic#103177)
  Remove add data button from nav (elastic#103810)
  Fix telemetry advanced setting style (elastic#103838)
  [Transform] Fix default naming and sorting fields suggestion for `top_metrics` agg (elastic#103690)
  [APM] use conventional error rate color for correlations (elastic#103500)

# Conflicts:
#	x-pack/plugins/reporting/server/lib/store/store.ts
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

onMigrationDone();
toasts.addSuccess({ title: i18nTexts.migrateSuccessTitle });
} catch (e) {
toasts.addError(e, { title: i18nTexts.migrateErrorTitle });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
toasts.addError(e, { title: i18nTexts.migrateErrorTitle });
toasts.addError(e, {
title: i18nTexts.migrateErrorTitle,
toastMessage: e.body?.message,
});

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Just a small suggestion on showing more info about the error in the toast message

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

This is great - I'm really happy with how this turned out.

I tested the default behavior, and it works well.

I also tested what happens when there is a custom ILM policy for reporting in place before Kibana runs with this change.

(I had to do some fiddling with the code to prevent kibana-reporting policy from being set, which is already happening in master but not 7.x)

  1. Create a custom ILM policy, link it to an index template, create a report. The list of policies should show 1 linked index.
  2. Run Kibana with this change. The new kibana-reporting policy is created:
    image
  3. Go to Stack Management > Reporting. You see the "Reporting indices are not managed by the same ILM policy" notice.
  4. Click the "Migrate Indices" button. A success toast message pops up.
  5. Back in the ILM listing, kibana-reporting is linked to 1 index and the custom policy is linked to 0
    image

This is what I expected should happen - there should only be 1 policy for the indices.

Then I tried:

  1. Delete the current reporting index
  2. Generate another report
  3. In the ILM policy listing, the kibana-reporting policy is the only one that shows its linked to an index

Big 👍 !!

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

Any follow up feedback will be handled in follow up PRs!

@jloleysens jloleysens merged commit 4c448c9 into elastic:master Jul 5, 2021
@jloleysens jloleysens deleted the reporting/ilm-policy-2 branch July 5, 2021 12:21
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 103853

jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 5, 2021
…c#103853)

* revert revert

* wip; first iteration of server-side functionality

* wip; public side code started. using the new ilm policy status endpoint

* * refactored ReportingIlmPolicyManager -> IlmReportingManager
* restructured files for the policy manager code
* re-added the IlmPolicyStatusResponse interface

* added the ability to use useRequest

* * removed extra server-side endpoint, we do both migration steps
  in one
* added ilm policy context and context for api client
* added es ui shared so we can use userequest

* added working link to ILM policy

* refactor of the migration route response (again), added logic for determining when to show ilm-policy link

* fix type issues and refactor to use testbed pattern for report listing component

* added tests for base cases of ilm policy banner

* remove non-existent prop

* added minor fixes (handling 404 from ES) and added API integration tests

* resolve merge conflict and remove unused file

* update toast error message

Co-authored-by: Kibana Machine <[email protected]>
jloleysens added a commit that referenced this pull request Jul 5, 2021
… (#104312)

* revert revert

* wip; first iteration of server-side functionality

* wip; public side code started. using the new ilm policy status endpoint

* * refactored ReportingIlmPolicyManager -> IlmReportingManager
* restructured files for the policy manager code
* re-added the IlmPolicyStatusResponse interface

* added the ability to use useRequest

* * removed extra server-side endpoint, we do both migration steps
  in one
* added ilm policy context and context for api client
* added es ui shared so we can use userequest

* added working link to ILM policy

* refactor of the migration route response (again), added logic for determining when to show ilm-policy link

* fix type issues and refactor to use testbed pattern for report listing component

* added tests for base cases of ilm policy banner

* remove non-existent prop

* added minor fixes (handling 404 from ES) and added API integration tests

* resolve merge conflict and remove unused file

* update toast error message

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
reporting 51 59 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
reporting 131 132 +1

Async chunks

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

id before after diff
reporting 64.9KB 70.4KB +5.5KB

Page load bundle

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

id before after diff
reporting 54.4KB 56.6KB +2.2KB
Unknown metric groups

API count

id before after diff
reporting 132 133 +1

History

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

@debadair
Copy link
Contributor

debadair commented Jul 9, 2021

I think we want to avoid overloading the term migrate, since that action has a specific meaning within the context of ILM. Instead, I'd focus on applying the new policy:

Apply new lifecycle policy for reports

Not all reporting indices are using the recommended ILM policy.
To ensure your reports are managed consistently,
all reporting indices should use the kibana-reporting policy.

Apply kibana-reporting policy

For the success toast, I'd focus on the resolution of the problem:

Recommended policy active for all reporting indices

I'd specify "reporting" in the edit link--without the context of the migration prompt, it seems too generic:

Edit reporting ILM policy

@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants