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.

Sign up for GitHub

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

Jump to bottom

[Synthetics] Fix filters persistance #152543

Merged
merged 5 commits into from
Mar 6, 2023
Merged

[Synthetics] Fix filters persistance #152543

merged 5 commits into from
Mar 6, 2023

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Mar 2, 2023 •
edited by kibanamachine
Loading

Summary

Fixes #152291

Keep filters persistance after monitor has been edited.

@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v8.7.0 v8.8.0 labels Mar 2, 2023
@shahzad31 shahzad31 marked this pull request as ready for review March 2, 2023 08:51
@shahzad31 shahzad31 requested review from a team as code owners March 2, 2023 08:51
awahab07
awahab07 reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

The reported problem is resolved. ✔️

Would be nice to handle the following edge case as well where an orphan option value makes the UI in an inconsistent state.

152543-Should-remove-orphan-filter-options.mov

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Mar 2, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

awahab07
awahab07 approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@awahab07 awahab07 left a comment

Choose a reason for hiding this comment

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

LGTM.

Works well.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

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

id before after diff
observability 1.1MB 1.1MB +96.0B
synthetics 1.4MB 1.4MB +290.0B
total +386.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
observability 53 54 +1
securitySolution 428 430 +2
total +3

Total ESLint disabled count

id before after diff
observability 59 60 +1
securitySolution 506 508 +2
total +3

History

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

maryam-saeidi
maryam-saeidi requested changes Mar 6, 2023
View reviewed changes
...ins/observability/public/components/shared/field_value_suggestions/field_value_selection.tsx
@@ -95,7 +95,8 @@ export function FieldValueSelection({

useEffect(() => {
setOptions(formatOptions(values, selectedValue, excludedValue, showCount));
}, [values, selectedValue, showCount, excludedValue]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(values), JSON.stringify(selectedValue), showCount, excludedValue]);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this impact performance since it uses JSON.stringify on every render?
Can't we use useMemo in the parent component instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think in this specific case. it won't have any impact. Since it's only meant to be triggered on new fetch of the data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the new fetch of the data remains same, we don't want existing component to re-render since data selection should remain same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using useMemo won't help in parent component.

Copy link
Member

@maryam-saeidi maryam-saeidi Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

i think in this specific case. it won't have any impact. Since it's only meant to be triggered on new fetch of the data.

You mean FieldValueSelection only re-renders when there is new data? Where is this logic?

Using useMemo won't help in parent component.

Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is being used on auto-refresh in synthetics app. so when that happens. the new data remains same , but it will trigger useMemo re-render. this is why we are doing this deep comparison. Json.stringify impact is minimal here. since data isn't big.

@shahzad31 shahzad31 merged commit 60a2c1a into elastic:main Mar 6, 2023
@shahzad31 shahzad31 deleted the fix-persis-filters branch March 6, 2023 09:21
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.7 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.7:
- [Security Solution]Fix incorrect number of invalid connectors is shown on the toast message (#152313)

Manual backport

To create the backport manually run:

node scripts/backport --pr 152543

Questions ?

Please refer to the Backport tool documentation

bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@shahzad31 @bmorelli25
[Synthetics] Fix filters persistance (elastic#152543)
cd8a626
@kibanamachine kibanamachine mentioned this pull request Apr 21, 2023
Merged
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 31, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

3 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@justinkambic justinkambic added Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team and removed Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Apr 15, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@justinkambic
Copy link
Contributor

I've removed the 8.7 label from this PR. It seems the automated backport failed back when the PR was merged and we likely never followed up to get this backported. At this point it's unlikely we'll release another 8.7 patch, so we'll remove the label to make the bot happy.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

5 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 152543 locally

@maryam-saeidi maryam-saeidi added the backport:skip This commit does not require backporting label Aug 5, 2024
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awahab07 awahab07 awahab07 approved these changes

@maryam-saeidi maryam-saeidi maryam-saeidi approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Synthetics] Overview - filters persist after editing monitor, but do not appear to be selected
7 participants
@shahzad31 @elasticmachine @kibana-ci @kibanamachine @justinkambic @awahab07 @maryam-saeidi