[ML] Single metric viewer: Fix race condition when changing jobs. #45420
Conversation
walterra
added
bug
walterra
requested review from
darnautov,
alvarezmelissa87 and
peteharverson
|
Pinging @elastic/ml-ui |
💔 Build Failed |
| } | ||
|
|
||
| function showFlyout() { | ||
| function showFlyout(setSkipRefresh = true) { |
There was a problem hiding this comment.
Wouldn't it be more intuitive to have setSkipRefresh = false and then send in true when we want to set it? 🤔
There was a problem hiding this comment.
My intention was to have it default to true because it is the default behaviour and therefore in the usual cases where it's used and e.g. passed on as a prop like onClose={closeFlyout} doesn't need wrapping in another callback and no code change to set it to true. The only time we need it to be false is inside applySelection where we call closeFlyout directly and can easily pass on the flag.
x-pack/legacy/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.js
Show resolved
Hide resolved
💔 Build Failed |
|
With these first couple of commits I can no longer produce the original issues (focus chart blank, or wrong entity dropdowns, when switching jobs). But I now see a new issue where the focus chart is blank after changing the end time of a job. Here an example using gallery data. This happens even with refresh turned off:
I also saw the context chart blank on one occasion: |
…bana into ml-fix-job-selection-refresh
|
@peteharverson I addressed the issue you're seeing in ffcccbc, please have another look. |
💔 Build Failed |
|
@peteharverson b0eddb5 has a fix for the issue where applying the same job with job selector wouldn't update the time range. |
|
Gave this a local test since the latest commit. Unfortunately, I'm seeing an issue now where when I select a farequote job from the jobs list and then use the action icon to navigate to the single metric viewer I end up with a blank screen and these errors in the console:
|
💔 Build Failed |
|
@alvarezmelissa87 @peteharverson 0dbd337 fixes the rendering loop with blank page you found. |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
Tested the latest changes and wasn't able to reproduce any of the previous issues. Thank you @walterra 🙌 |
…bana into ml-fix-job-selection-refresh
💚 Build Succeeded |
…astic#45420) Fixes a race condition where changing jobs in Single Metric Viewer could result in the focus chart being empty. Applying the job change would cause two events to be triggered which would cause the Single Metric Viewer to refresh twice: The updated time range would be triggered via timefilter, the updated jobs via the globalState listener. This PR solves the problem by setting a global flag skipRefresh which allows us to skip updating single metric viewer and only update again once both the new time range and jobs are set. The PR also adds some checks to avoid unnecessary updates in d3 code triggered by React render calls.
…astic#45420) Fixes a race condition where changing jobs in Single Metric Viewer could result in the focus chart being empty. Applying the job change would cause two events to be triggered which would cause the Single Metric Viewer to refresh twice: The updated time range would be triggered via timefilter, the updated jobs via the globalState listener. This PR solves the problem by setting a global flag skipRefresh which allows us to skip updating single metric viewer and only update again once both the new time range and jobs are set. The PR also adds some checks to avoid unnecessary updates in d3 code triggered by React render calls.
…5420) (#45643) Fixes a race condition where changing jobs in Single Metric Viewer could result in the focus chart being empty. Applying the job change would cause two events to be triggered which would cause the Single Metric Viewer to refresh twice: The updated time range would be triggered via timefilter, the updated jobs via the globalState listener. This PR solves the problem by setting a global flag skipRefresh which allows us to skip updating single metric viewer and only update again once both the new time range and jobs are set. The PR also adds some checks to avoid unnecessary updates in d3 code triggered by React render calls.
…5420) (#45644) Fixes a race condition where changing jobs in Single Metric Viewer could result in the focus chart being empty. Applying the job change would cause two events to be triggered which would cause the Single Metric Viewer to refresh twice: The updated time range would be triggered via timefilter, the updated jobs via the globalState listener. This PR solves the problem by setting a global flag skipRefresh which allows us to skip updating single metric viewer and only update again once both the new time range and jobs are set. The PR also adds some checks to avoid unnecessary updates in d3 code triggered by React render calls.
Summary
Part of #42909.
Fixes a race condition where changing jobs in Single Metric Viewer could result in the focus chart being empty.
Applying the job change would cause two events to be triggered which would cause the Single Metric Viewer to refresh twice: The updated time range would be triggered via
timefilter, the updated jobs via theglobalStatelistener.This PR solves the problem by setting a global flag
skipRefreshwhich allows us to skip updating single metric viewer and only update again once both the new time range and jobs are set. The PR also adds some checks to avoid unnecessary updates ind3code triggered by React render calls.I'm not sure polluting the
globalStatewith this flag is the best approach but it allowed me to implement the fix for7.4with the least code changes. For7.5maybe we can find a better solution, for example using an observable that keeps track of it.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenarioslegacy code covered by manual testsThis was checked for keyboard-only and screenreader accessibilityFor maintainers