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

Improve state sync error handling #74264

Merged
merged 8 commits into from
Aug 6, 2020
Merged

Improve state sync error handling #74264

merged 8 commits into from
Aug 6, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 4, 2020

Summary

Fixes #71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see #71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in #71461:

  1. Kibana users state:storeInSessionStorage
  2. Users often intuitively share hashed dashboard urls directly
  3. When someone opens those urls - there is a blank screen with warning

In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.

How to test

Simplest way to test is to repeat real user flow:

  1. Enabled state:storeInSessionStorage
  2. Go to dashboard
  3. Copy url and open it in new incognito tab or different browser
  4. Dashboard should load. Warning(s) should be displayed. State (like time range or filters) should reset.

Please note

When testing you may notice multiple warnings,
This is a separate common issue which needs attention:
tracked here: #67270

Implementation details

After reviewing all state syncing usages, I decided to add error handling level not on a state_sync util level itself, but on storage level instead. You could view documentation diffs for insights.

The reason is that kbnUrlStateStorage is used both by applications code directly and indirectly by state_sync utils. If I'd add error handling apis on state_sync level, then error handling in application code would be more tedious: both had to make sure state_sync is setup to handle errors, but also every usage of kbnUrlStateStorage api is wrapped in try / catch.

Release notes

Fix apps break on unrestorable session state in URL

Checklist

@Dosant Dosant added the bug Fixes for quality problems that affect the customer experience label Aug 5, 2020
@Dosant Dosant marked this pull request as ready for review August 5, 2020 08:50
@Dosant Dosant requested a review from a team August 5, 2020 08:50
@Dosant Dosant requested review from a team as code owners August 5, 2020 08:50
@Dosant Dosant requested a review from a team August 5, 2020 08:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@Dosant Dosant requested a review from flash1293 August 5, 2020 08:50
streamich
streamich approved these changes Aug 5, 2020
View reviewed changes
Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

flash1293
flash1293 approved these changes Aug 5, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

I tested this on dashboard and it seems like I get a toast even if I open the tab in the same window. Shouldn't it use the localstorage and retain the hash when doing this?

It's also broken on 7.8 so not blocking for this PR but if we can't fix it here I will open a new issue for this.

nreese
nreese approved these changes Aug 5, 2020
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

maps changes lgtm
code review

igoristic
igoristic approved these changes Aug 5, 2020
View reviewed changes
Copy link
Contributor

@igoristic igoristic left a comment

Choose a reason for hiding this comment

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

Monitoring changes look good 👍

@Dosant
Copy link
Contributor Author

Dosant commented Aug 6, 2020

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Aug 6, 2020

@flash1293, it uses session storage.
Just checked 7.5, to make sure this how it was the same before state syncing utils: https://github.com/elastic/kibana/blob/7.5/src/legacy/ui/public/state_management/state_storage/hashed_item_store_singleton.js#L22

@flash1293
Copy link
Contributor

Thanks for confirming @Dosant . In this case everything works as expected

@Dosant
Copy link
Contributor Author

Dosant commented Aug 6, 2020

@elasticmachine merge upstream

@ppisljar
Copy link
Member

ppisljar commented Aug 6, 2020

we should backport this to 7.9

@Dosant
Merge branch 'dev/improve-state-sync-error-handling' of github.com:Do…
1822c0b 
…sant/kibana into dev/improve-state-sync-error-handling
@Dosant Dosant added the v7.9.1 label Aug 6, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
kibanaUtils 191 +1 190

async chunks size

id value diff baseline
discover 430.3KB +286.0B 430.0KB
lens 25.1KB +106.0B 25.0KB
maps 3.3MB +1.1KB 3.3MB
monitoring 1.1MB +129.0B 1.1MB
timelion 591.5KB +100.0B 591.4KB
total +1.7KB

page load bundle size

id value diff baseline
dashboard 679.4KB +107.0B 679.3KB
kibanaUtils 469.0KB +1.1KB 467.9KB
visualize 30.9KB +85.0B 30.8KB
total +1.3KB

History

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

@Dosant Dosant merged commit 3064c6e into elastic:master Aug 6, 2020
@Dosant Dosant mentioned this pull request Aug 6, 2020
Merged
Dosant added a commit to Dosant/kibana that referenced this pull request Aug 6, 2020
@Dosant @elasticmachine
Improve state sync error handling (elastic#74264)
384b2e9 
Fixes elastic#71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see elastic#71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in elastic#71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.


Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit to Dosant/kibana that referenced this pull request Aug 6, 2020
@Dosant
Improve state sync error handling (elastic#74264)
c0f22bc 
Fixes elastic#71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see elastic#71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in elastic#71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
# Conflicts:
#	src/plugins/timelion/public/app.js
#	x-pack/plugins/lens/public/app_plugin/app.tsx
@Dosant Dosant mentioned this pull request Aug 6, 2020
Closed
Dosant added a commit that referenced this pull request Aug 10, 2020
@Dosant @elasticmachine
Improve state sync error handling (#74264) (#74524)
e0efd61 
Fixes #71461 regression since 7.7

New state syncing utils didn't properly handle errors,
Errors happening during URL parsing or writing wasn't handled, so state syncing could stop or in worth case blow out. (see #71461)

There are not much scenarios where missing proper error handling could really impact users, except the one described in #71461:

Kibana users state:storeInSessionStorage
Users often intuitively share hashed dashboard urls directly
When someone opens those urls - there is a blank screen with warning
In 7.6 - dashboard would still load with default state.
Since 7.7 these still could be achieved by removing query params for URL, but it is not obvious for regular users.

This PR makes sure that behaviour is similar to one we had before 7.7.


Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Aug 10, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

4 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

ppisljar
ppisljar reviewed Aug 18, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

5 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@Dosant Dosant mentioned this pull request Aug 26, 2020
Closed
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@Dosant Dosant removed backport missing Added to PRs automatically when the are determined to be missing a backport. v7.9.1 labels Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppisljar ppisljar ppisljar left review comments

@igoristic igoristic igoristic approved these changes

@streamich streamich streamich approved these changes

@flash1293 flash1293 flash1293 approved these changes

@nreese nreese nreese approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:StateManagement release_note:fix v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dashboard breaks on unrestorable session state in URL
8 participants
@Dosant @elasticmachine @flash1293 @ppisljar @kibanamachine @nreese @streamich @igoristic