[Index Management] Fix unhandled error in ds data retention modal #196524

ElenaStoeva · 2024-10-16T10:30:16Z

Fixes #196331

Summary

This PR fixes the bug in the Edit ds data retention modal where we were comparing the max retention period with an undefined value (now the comparison happens only if value is defined). Also, the PR makes the data retention field get re-validated only when the time unit changes (otherwise, when we switch off the toggle to enable to data retention field, the field would get validated and would immediately show an error "A data retention value is required." which is not great UX).

How to test:

Start serverless ES with yarn es serverless --projectType=security -E data_streams.lifecycle.retention.max=200d and kibana with yarn serverless-security
Navigate to Kibana, create a data stream using Dev Tools:

PUT _index_template/ds
{
  "index_patterns": ["ds"],
  "data_stream": {}
}

POST ds/_doc
{
  "@timestamp": "2020-01-27"
}

Navigate to Index Management. Find the data stream and select it --> Click "Manage" --> Click "Edit data retention"
Disable the toggle "Keep data up to maximum retention period"
Verify that the field is enabled correctly, there is not endless spinner, and no console error.

Screen.Recording.2024-10-16.at.11.23.29.mov

elasticmachine · 2024-10-16T10:30:19Z

Pinging @elastic/kibana-management (Team:Kibana Management)

mattkime · 2024-10-18T13:12:53Z

/ci

mattkime

Code looks good and works well but it would be nice if there was a test to go along with it. The validation function would benefit from a unit test. Also, instead of having one big validation function, multiple validators can be passed which would further simplify the code and testing.

ElenaStoeva · 2024-10-21T22:49:35Z

Thanks for the review @mattkime! I refactored the validations and added unit tests for the comparing validation. Could you please take another look?

mattkime · 2024-10-22T02:07:07Z

...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx

+          if (!formData.infiniteRetentionPeriod) {
+            // If project level data retention is enabled, we need to enforce the global max retention
+            const { globalMaxRetention, enableProjectLevelRetentionChecks } =
+              customData.value as any;


Why is any used here? can it be avoided? It would be nice to know but if you don't, its fine. If this is kept as is then it might be good to create a follow up ticket.

I think this is added to avoid a type error because the type of customData.value is unknown (see here).

mattkime

Currently the form will accept a decimal value but an error will be shown when attempting to save

mattkime · 2024-10-22T04:57:08Z

I'm not sure if this is related but if you don't specify the retention period you get this alert which feels a bit like an error message. Might be worthy of a new issue.

ElenaStoeva · 2024-10-22T10:57:56Z

Currently the form will accept a decimal value but an error will be shown when attempting to save

Nice catch! The issue with decimal numbers seems to exist even before this PR. The validation that compares the data retention with max data retention has been implemented to support integer values only. If we want to support decimals, the fix would become much more complex and probably out of the scope of this PR, so I think it's okay for now to add field validation for integer numbers only. @jovana-andjelkovic do you know if we should support decimals in the form?

I'm not sure if this is related but if you don't specify the retention period you get this alert which feels a bit like an error message. Might be worthy of a new issue.

Yes, I don't think this is related to these changes, but it would be nice to address it in a follow-up PR.

jovana-andjelkovic · 2024-10-22T12:39:52Z

@ElenaStoeva @mattkime short answer: it would make sense to not allow decimal input, especially as we allow them to input different time periods.

Long answer:
Ideal case scenario is that we'd transform decimal input into shorter period for the user, so in case they enter 1.5 days we update the field to 36 hours but as this was not something we're supporting right now (that I know of) it sounds more like a feature enhancement than a bug to fix, so we should just remove decimal input to avoid errors.

cc @bitzandeb in case you disagree

mattkime

Now correctly prompts user to enter an integer value. It looks like minor test type fixes remain. Approving with assumption that remaining test fixes are very minor.

elasticmachine · 2024-10-22T14:43:20Z

💚 Build Succeeded

Buildkite Build
Commit: 82a6197

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`indexManagement`	693	694	+1

Async chunks

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

id	before	after	diff
`indexManagement`	688.0KB	688.5KB	+455.0B

History

💔 Build #244608 failed 790b1cac32dde47c506617b28d4c3b1c7a5f493c
💔 Build #244580 failed b0718ce
💔 Build #244477 failed 628daf4
💛 Build #243154 was flaky cae640d
💔 Build #243093 failed 7609dae
💔 Build #243026 failed a719654

cc @ElenaStoeva

kibanamachine · 2024-10-22T14:53:57Z

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11462845300

kibanamachine · 2024-10-22T14:58:43Z

💔 All backports failed

Status	Branch	Result
❌	8.16	Backport failed because of merge conflicts
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 196524

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-10-23T15:14:42Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11482918898

@timestamp

…astic#196524) Fixes elastic#196331 ## Summary This PR fixes the bug in the Edit ds data retention modal where we were comparing the max retention period with an undefined `value` (now the comparison happens only if `value` is defined). Also, the PR makes the data retention field get re-validated only when the time unit changes (otherwise, when we switch off the toggle to enable to data retention field, the field would get validated and would immediately show an error "A data retention value is required." which is not great UX). ### How to test: 1. Start serverless ES with `yarn es serverless --projectType=security -E data_streams.lifecycle.retention.max=200d` and kibana with `yarn serverless-security` 2. Navigate to Kibana, create a data stream using Dev Tools: ``` PUT _index_template/ds { "index_patterns": ["ds"], "data_stream": {} } POST ds/_doc { "@timestamp": "2020-01-27" } ``` 3. Navigate to Index Management. Find the data stream and select it --> Click "Manage" --> Click "Edit data retention" 4. Disable the toggle "Keep data up to maximum retention period" 5. Verify that the field is enabled correctly, there is not endless spinner, and no console error. https://github.com/user-attachments/assets/957e0869-ee23-46d9-8f20-134937f6f8cf --------- Co-authored-by: Matthew Kime <[email protected]> (cherry picked from commit d8fa996)

kibanamachine · 2024-10-23T15:19:44Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@timestamp

…al (#196524) (#197481) # Backport This will backport the following commits from `main` to `8.x`: - [[Index Management] Fix unhandled error in ds data retention modal (#196524)](#196524)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) \r\nClick \"Manage\" --> Click \"Edit data retention\"\r\n4. Disable the toggle \"Keep data up to maximum retention period\"\r\n5. Verify that the field is enabled correctly, there is not endless\r\nspinner, and no console error.\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/957e0869-ee23-46d9-8f20-134937f6f8cf\r\n\r\n---------\r\n\r\nCo-authored-by: Matthew Kime <[email protected]>","sha":"d8fa996c5052d10da2f438b93723437f5631bbde","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Index Management","Team:Kibana Management","release_note:skip","v9.0.0","backport:prev-minor","v8.17.0"],"title":"[Index Management] Fix unhandled error in ds data retention modal","number":196524,"url":"https://github.com/elastic/kibana/pull/196524","mergeCommit":{"message":"[Index Management] Fix unhandled error in ds data retention modal (#196524)\n\nFixes https://github.com/elastic/kibana/issues/196331\r\n\r\n## Summary\r\n\r\nThis PR fixes the bug in the Edit ds data retention modal where we were\r\ncomparing the max retention period with an undefined `value` (now the\r\ncomparison happens only if `value` is defined). Also, the PR makes the\r\ndata retention field get re-validated only when the time unit changes\r\n(otherwise, when we switch off the toggle to enable to data retention\r\nfield, the field would get validated and would immediately show an error\r\n\"A data retention value is required.\" which is not great UX).\r\n\r\n### How to test:\r\n1. Start serverless ES with `yarn es serverless --projectType=security\r\n-E data_streams.lifecycle.retention.max=200d` and kibana with `yarn\r\nserverless-security`\r\n2. Navigate to Kibana, create a data stream using Dev Tools:\r\n```\r\nPUT _index_template/ds\r\n{\r\n \"index_patterns\": [\"ds\"],\r\n \"data_stream\": {}\r\n}\r\n\r\nPOST ds/_doc\r\n{\r\n \"@timestamp\": \"2020-01-27\"\r\n}\r\n```\r\n3. Navigate to Index Management. Find the data stream and select it -->\r\nClick \"Manage\" --> Click \"Edit data retention\"\r\n4. Disable the toggle \"Keep data up to maximum retention period\"\r\n5. Verify that the field is enabled correctly, there is not endless\r\nspinner, and no console error.\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/957e0869-ee23-46d9-8f20-134937f6f8cf\r\n\r\n---------\r\n\r\nCo-authored-by: Matthew Kime <[email protected]>","sha":"d8fa996c5052d10da2f438b93723437f5631bbde"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196524","number":196524,"mergeCommit":{"message":"[Index Management] Fix unhandled error in ds data retention modal (#196524)\n\nFixes https://github.com/elastic/kibana/issues/196331\r\n\r\n## Summary\r\n\r\nThis PR fixes the bug in the Edit ds data retention modal where we were\r\ncomparing the max retention period with an undefined `value` (now the\r\ncomparison happens only if `value` is defined). Also, the PR makes the\r\ndata retention field get re-validated only when the time unit changes\r\n(otherwise, when we switch off the toggle to enable to data retention\r\nfield, the field would get validated and would immediately show an error\r\n\"A data retention value is required.\" which is not great UX).\r\n\r\n### How to test:\r\n1. Start serverless ES with `yarn es serverless --projectType=security\r\n-E data_streams.lifecycle.retention.max=200d` and kibana with `yarn\r\nserverless-security`\r\n2. Navigate to Kibana, create a data stream using Dev Tools:\r\n```\r\nPUT _index_template/ds\r\n{\r\n \"index_patterns\": [\"ds\"],\r\n \"data_stream\": {}\r\n}\r\n\r\nPOST ds/_doc\r\n{\r\n \"@timestamp\": \"2020-01-27\"\r\n}\r\n```\r\n3. Navigate to Index Management. Find the data stream and select it -->\r\nClick \"Manage\" --> Click \"Edit data retention\"\r\n4. Disable the toggle \"Keep data up to maximum retention period\"\r\n5. Verify that the field is enabled correctly, there is not endless\r\nspinner, and no console error.\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/957e0869-ee23-46d9-8f20-134937f6f8cf\r\n\r\n---------\r\n\r\nCo-authored-by: Matthew Kime <[email protected]>","sha":"d8fa996c5052d10da2f438b93723437f5631bbde"}},{"branch":"8.x","label":"v8.17.0","branchLabelMappingKey":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Elena Stoeva <[email protected]>

[Index Management] Fix unhandled error in ds data retention modal

a719654

ElenaStoeva self-assigned this Oct 16, 2024

ElenaStoeva requested a review from a team as a code owner October 16, 2024 10:30

ElenaStoeva requested a review from mattkime October 16, 2024 10:30

ElenaStoeva added 2 commits October 16, 2024 13:39

Merge branch 'main' into data-stream/retention-period-bug-fix

7609dae

Merge branch 'main' into data-stream/retention-period-bug-fix

cae640d

ElenaStoeva added v9.0.0 v8.16.0 v8.17.0 labels Oct 17, 2024

Merge branch 'main' into data-stream/retention-period-bug-fix

c70dd22

mattkime requested changes Oct 19, 2024

View reviewed changes

Refactor validators and add tests

893cca2

ElenaStoeva force-pushed the data-stream/retention-period-bug-fix branch from 5477c6a to 893cca2 Compare October 21, 2024 22:47

Merge branch 'main' into data-stream/retention-period-bug-fix

628daf4

ElenaStoeva requested a review from mattkime October 21, 2024 22:49

mattkime reviewed Oct 22, 2024

View reviewed changes

mattkime requested changes Oct 22, 2024

View reviewed changes

Add validation for integer

b0718ce

ElenaStoeva requested a review from mattkime October 22, 2024 10:58

Fix type errors

82a6197

ElenaStoeva force-pushed the data-stream/retention-period-bug-fix branch from 790b1ca to 82a6197 Compare October 22, 2024 12:59

mattkime approved these changes Oct 22, 2024

View reviewed changes

ElenaStoeva merged commit d8fa996 into elastic:main Oct 22, 2024
21 checks passed

ElenaStoeva mentioned this pull request Oct 23, 2024

Create a common Int Validator and use it in ingest_pipelines and Index_lifecycle_management #196527

Merged

ElenaStoeva added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.16.0 labels Oct 23, 2024

kibanamachine mentioned this pull request Oct 23, 2024

[8.x] [Index Management] Fix unhandled error in ds data retention modal (#196524) #197481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Index Management] Fix unhandled error in ds data retention modal #196524

[Index Management] Fix unhandled error in ds data retention modal #196524

ElenaStoeva commented Oct 16, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Oct 16, 2024

mattkime commented Oct 18, 2024

mattkime left a comment

ElenaStoeva commented Oct 21, 2024

mattkime Oct 22, 2024 •

edited

Loading

ElenaStoeva Oct 22, 2024

mattkime left a comment

mattkime commented Oct 22, 2024

ElenaStoeva commented Oct 22, 2024

jovana-andjelkovic commented Oct 22, 2024

mattkime left a comment

elasticmachine commented Oct 22, 2024

kibanamachine commented Oct 22, 2024

kibanamachine commented Oct 22, 2024

kibanamachine commented Oct 23, 2024

kibanamachine commented Oct 23, 2024

[Index Management] Fix unhandled error in ds data retention modal #196524

[Index Management] Fix unhandled error in ds data retention modal #196524

Conversation

ElenaStoeva commented Oct 16, 2024 • edited by kibanamachine Loading

Summary

How to test:

elasticmachine commented Oct 16, 2024

mattkime commented Oct 18, 2024

mattkime left a comment

Choose a reason for hiding this comment

ElenaStoeva commented Oct 21, 2024

mattkime Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

ElenaStoeva Oct 22, 2024

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

mattkime commented Oct 22, 2024

ElenaStoeva commented Oct 22, 2024

jovana-andjelkovic commented Oct 22, 2024

mattkime left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 22, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Oct 22, 2024

kibanamachine commented Oct 22, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Oct 23, 2024

kibanamachine commented Oct 23, 2024

💚 All backports created successfully

Questions ?

ElenaStoeva commented Oct 16, 2024 •

edited by kibanamachine

Loading

mattkime Oct 22, 2024 •

edited

Loading