[Data Quality] Integrate failure store modal#231846
[Data Quality] Integrate failure store modal#231846SoniaSanzV merged 13 commits intoelastic:mainfrom
Conversation
|
Pinging @elastic/kibana-management (Team:Kibana Management) |
| const content = ( | ||
| <> | ||
| <EuiText textAlign="left">{title}</EuiText> | ||
| <EuiSpacer size="xs" /> | ||
| <EuiText textAlign="left" data-test-subj={`datasetQualityDetailsSummaryKpiValue-${title}`}> | ||
| <h2>{kpiValue}</h2> | ||
| </EuiText> | ||
| <EuiSpacer size="xs" /> | ||
| <EuiText textAlign="left">{footer}</EuiText> | ||
| </> | ||
| ); | ||
|
|
||
| return onClick ? ( | ||
| <EuiButtonEmpty | ||
| isDisabled={isDisabled} | ||
| onClick={onClick} | ||
| css={css` | ||
| height: 100%; | ||
| min-width: 300px; | ||
| border: ${isSelected | ||
| ? `${euiTheme.border.width.thick} solid ${euiTheme.colors.borderStrongPrimary}` | ||
| : 'none'}; | ||
| background-color: ${isSelected ? euiTheme.colors.backgroundLightPrimary : 'inherit'}; | ||
| `} | ||
| css={style} | ||
| contentProps={{ | ||
| css: css` | ||
| justify-content: flex-start; | ||
| `, | ||
| }} | ||
| data-test-subj={`datasetQualityDetailsSummaryKpiCard-${title}`} | ||
| data-test-subj={dataTestSubject} | ||
| > | ||
| <EuiText textAlign="left">{title}</EuiText> | ||
| <EuiSpacer size="xs" /> | ||
| <EuiText textAlign="left" data-test-subj={`datasetQualityDetailsSummaryKpiValue-${title}`}> | ||
| <h2>{kpiValue}</h2> | ||
| </EuiText> | ||
| <EuiSpacer size="xs" /> | ||
| <EuiText textAlign="left">{footer}</EuiText> | ||
| {content} | ||
| </EuiButtonEmpty> | ||
| ) : ( | ||
| <div css={style} data-test-subj={dataTestSubject}> | ||
| {content} | ||
| </div> |
There was a problem hiding this comment.
I needed to change this to be able to include the failure store button. The component was a button itself, so I couldn't add a button inside of a button.
ElenaStoeva
left a comment
There was a problem hiding this comment.
Great job @SoniaSanzV! Tested locally and it looks good to me. Code changes also lgtm but will leave it up to @elastic/obs-ux-logs-team to review in more detail.
|
Can we extend this work to also show the modal from the main page? Or maybe create an issue to handle it in a follow up PR? Screen.Recording.2025-08-25.at.08.39.51.mov |
...ck/platform/plugins/shared/dataset_quality/public/hooks/use_dataset_quality_details_state.ts
Outdated
Show resolved
Hide resolved
...ed/dataset_quality/public/state_machines/dataset_quality_details_controller/state_machine.ts
Outdated
Show resolved
Hide resolved
...ed/dataset_quality/public/state_machines/dataset_quality_details_controller/state_machine.ts
Outdated
Show resolved
Hide resolved
...ed/dataset_quality/public/state_machines/dataset_quality_details_controller/state_machine.ts
Outdated
Show resolved
Hide resolved
...form/plugins/shared/dataset_quality/server/routes/data_streams/update_failure_store/index.ts
Outdated
Show resolved
Hide resolved
...ins/shared/dataset_quality/public/state_machines/dataset_quality_details_controller/types.ts
Outdated
Show resolved
Hide resolved
...m/plugins/shared/dataset_quality/server/routes/data_streams/get_data_stream_details/index.ts
Outdated
Show resolved
Hide resolved
...m/plugins/shared/dataset_quality/server/routes/data_streams/get_data_stream_details/index.ts
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| const updateFailureStoreRoute = createDatasetQualityServerRoute({ | ||
| endpoint: 'PUT /internal/dataset_quality/data_streams/{dataStream}/update_failure_store', |
There was a problem hiding this comment.
Not a blocker and not asking for this change in the current PR but I was expecting somehow the modal component handling the update by itself, in the current setup every consumer would need to create a route to actually enable the failure store and I'm guessing all the routes will be handled in the same way, all of them will call the same elasticsearch endpoint and configure the same properties there.
There was a problem hiding this comment.
@ElenaStoeva and I discussed that option, but in the end we decided would be better to let the modal stateful.
...ataset_quality/server/routes/data_streams/get_data_streams_default_retention_period/index.ts
Show resolved
Hide resolved
| failure_store: { | ||
| enabled: failureStoreEnabled, | ||
| lifecycle: { | ||
| data_retention: customRetentionPeriod, |
There was a problem hiding this comment.
What happens if customRetentionPeriod is undefined?
this would end up being:
# stateful
{
...
lifecycle: {
enabled: false
}
}
#serverless
{
...
lifecycle: {}
}
What does that mean in practice?
There was a problem hiding this comment.
If we set lifecycle enabled=false in serverless it would fail because lifecycle cannot be disabled in serverless. In the moment we add a data retention period, it would be set to enabled: true by ES.
{
"error": {
"root_cause": [
{
"type": "illegal_argument_exception",
"reason": "failures lifecycle cannot be disabled in serverless, please remove 'enabled=false'"
}
],
"type": "illegal_argument_exception",
"reason": "failures lifecycle cannot be disabled in serverless, please remove 'enabled=false'"
},
"status": 400
}
But this is something that it's allowed in stateful, users can set a lifecycle data retention period but set it to enabled=false.
In practice, here, if failure store is disabled, the custom data retention would be undefined, so lifecycle would always be enabled=true when the user sets a custom retention period
There was a problem hiding this comment.
but not having a customRetentionPeriod, if failure store is enabled, means that the defaultRetention will be used?
There was a problem hiding this comment.
If the failure store is enabled, yes
There was a problem hiding this comment.
Thanks for the details, now I have a better understanding
There was a problem hiding this comment.
I misunderstood this, I've created a PR for fixing it #233292
...ck/platform/plugins/shared/dataset_quality/public/hooks/use_dataset_quality_details_state.ts
Outdated
Show resolved
Hide resolved
|
This works beautifully (I tested locally for stateful and serverless) :chefs-kiss: Screen.Recording.2025-08-26.at.10.21.11.movThen when I checked the settings of the dataStream Good job @SoniaSanzV 🚀 |
yngrdyn
left a comment
There was a problem hiding this comment.
instead of changing the test I'd prefer we change the route code
If dataStreamDetails is empty then return empty, otherwise append the defaultRetentionPeriod
Maybe we would have other implications in the UI if dataStreamDetails only contains the defaultRetentionPeriod
Makes total sense, changed in da179b3 |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
History
cc @SoniaSanzV |
) ## Summary I did an error when implementing #231846. In Stateful, when a data stream has the failure retention period set to default, I set the lifecycle to enabled: false, which is wrong. This is the config for a data stream with failure store enabled by default (before interacting with the modal). <img width="706" height="326" alt="Screenshot 2025-08-28 at 12 03 44" src="https://github.com/user-attachments/assets/4a0f78d9-f593-4b7c-bfe1-ee7c7eb90aeb" /> Before this PR: The lifecycle is set to false when no custom retention period, so the retention is not determined by the default value, which is incorrect: <img width="636" height="287" alt="Screenshot 2025-08-28 at 12 04 37" src="https://github.com/user-attachments/assets/387f423f-e3cf-4864-9508-124a60fcc38e" /> With this change: The enabled lifecycle is determined by the enabled fault storage and not by a customRetentionPeriod being assigned. <img width="688" height="321" alt="Screenshot 2025-08-28 at 12 05 45" src="https://github.com/user-attachments/assets/7b05b988-96ac-4fd2-a0d3-bbe1663ba926" /> With customPeriod: <img width="619" height="346" alt="Screenshot 2025-08-28 at 12 27 33" src="https://github.com/user-attachments/assets/1d2d2846-7399-42cd-9f5d-fe96f0b09adc" />
…tic#233292) ## Summary I did an error when implementing elastic#231846. In Stateful, when a data stream has the failure retention period set to default, I set the lifecycle to enabled: false, which is wrong. This is the config for a data stream with failure store enabled by default (before interacting with the modal). <img width="706" height="326" alt="Screenshot 2025-08-28 at 12 03 44" src="https://github.com/user-attachments/assets/4a0f78d9-f593-4b7c-bfe1-ee7c7eb90aeb" /> Before this PR: The lifecycle is set to false when no custom retention period, so the retention is not determined by the default value, which is incorrect: <img width="636" height="287" alt="Screenshot 2025-08-28 at 12 04 37" src="https://github.com/user-attachments/assets/387f423f-e3cf-4864-9508-124a60fcc38e" /> With this change: The enabled lifecycle is determined by the enabled fault storage and not by a customRetentionPeriod being assigned. <img width="688" height="321" alt="Screenshot 2025-08-28 at 12 05 45" src="https://github.com/user-attachments/assets/7b05b988-96ac-4fd2-a0d3-bbe1663ba926" /> With customPeriod: <img width="619" height="346" alt="Screenshot 2025-08-28 at 12 27 33" src="https://github.com/user-attachments/assets/1d2d2846-7399-42cd-9f5d-fe96f0b09adc" />
Closes #231848 ## Summary With #231846 we introduced the possibility for the users to enabling failure store from the data quality plugin, but they couldn't edit or disable it from there. With this PR, we introduce the option of editing failure store from data set quality. ### How to test * Go to data quality plugin * Select a data set * Enable the failure store if disabled * Click the failure store card * Click the new pen icon * Verify the failure store modal opens * Verify that you can edit data and disable the failure store Note: Despite that in the mocks the icon is on the left, I add it to the right so the discover button is consisten with the one that the other card enables. ### Mocks <img width="1846" height="690" alt="image" src="https://github.com/user-attachments/assets/3069ae8c-b276-487a-9104-106f4adfa948" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes elastic#231848 ## Summary With elastic#231846 we introduced the possibility for the users to enabling failure store from the data quality plugin, but they couldn't edit or disable it from there. With this PR, we introduce the option of editing failure store from data set quality. ### How to test * Go to data quality plugin * Select a data set * Enable the failure store if disabled * Click the failure store card * Click the new pen icon * Verify the failure store modal opens * Verify that you can edit data and disable the failure store Note: Despite that in the mocks the icon is on the left, I add it to the right so the discover button is consisten with the one that the other card enables. ### Mocks <img width="1846" height="690" alt="image" src="https://github.com/user-attachments/assets/3069ae8c-b276-487a-9104-106f4adfa948" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes elastic#231848 ## Summary With elastic#231846 we introduced the possibility for the users to enabling failure store from the data quality plugin, but they couldn't edit or disable it from there. With this PR, we introduce the option of editing failure store from data set quality. ### How to test * Go to data quality plugin * Select a data set * Enable the failure store if disabled * Click the failure store card * Click the new pen icon * Verify the failure store modal opens * Verify that you can edit data and disable the failure store Note: Despite that in the mocks the icon is on the left, I add it to the right so the discover button is consisten with the one that the other card enables. ### Mocks <img width="1846" height="690" alt="image" src="https://github.com/user-attachments/assets/3069ae8c-b276-487a-9104-106f4adfa948" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes elastic#231848 ## Summary With elastic#231846 we introduced the possibility for the users to enabling failure store from the data quality plugin, but they couldn't edit or disable it from there. With this PR, we introduce the option of editing failure store from data set quality. ### How to test * Go to data quality plugin * Select a data set * Enable the failure store if disabled * Click the failure store card * Click the new pen icon * Verify the failure store modal opens * Verify that you can edit data and disable the failure store Note: Despite that in the mocks the icon is on the left, I add it to the right so the discover button is consisten with the one that the other card enables. ### Mocks <img width="1846" height="690" alt="image" src="https://github.com/user-attachments/assets/3069ae8c-b276-487a-9104-106f4adfa948" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes #231848 ## Summary With #231846 we introduced the possibility for the users to enabling failure store from the data quality plugin, but they couldn't edit or disable it from there. With this PR, we introduce the option of editing failure store from data set quality. ### How to test * Go to data quality plugin * Select a data set * Enable the failure store if disabled * Click the failure store card * Click the new pen icon * Verify the failure store modal opens * Verify that you can edit data and disable the failure store Note: Despite that in the mocks the icon is on the left, I add it to the right so the discover button is consisten with the one that the other card enables. ### Mocks <img width="1846" height="690" alt="image" src="https://github.com/user-attachments/assets/3069ae8c-b276-487a-9104-106f4adfa948" /> --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes #232736 ## Summary In #231846 we implemented the failure store modal in the Data set detail page but the main page still redirects to Index Management.This PR implements new modal also in the Data quality main page. https://github.com/user-attachments/assets/93d5ae87-ad5b-4f18-b271-185c05691ddb --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…239916) Closes elastic#232736 ## Summary In elastic#231846 we implemented the failure store modal in the Data set detail page but the main page still redirects to Index Management.This PR implements new modal also in the Data quality main page. https://github.com/user-attachments/assets/93d5ae87-ad5b-4f18-b271-185c05691ddb --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit b625245)
…239916) Closes elastic#232736 ## Summary In elastic#231846 we implemented the failure store modal in the Data set detail page but the main page still redirects to Index Management.This PR implements new modal also in the Data quality main page. https://github.com/user-attachments/assets/93d5ae87-ad5b-4f18-b271-185c05691ddb --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

Closes #231429
Summary
This PR integrates the Failure Store Modal in the Dataset Quality page.
failure_store.mov
Note: I'm not sure if there is a better way of refreshing the card after saving the failure store configs using the State Machine