-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
[ML] Fix Limit Dropdown, simplify state management of Anomaly Explorer. #23388
Conversation
Pinging @elastic/ml-ui |
💔 Build Failed |
…tener in explorer_controller.
12efbb7
to
b7342f0
Compare
💔 Build Failed |
retest |
} else { | ||
// Ensure the search bounds align to the bucketing interval used in the swimlane so | ||
// that the first and last buckets are complete. | ||
const bounds = timefilter.getActiveBounds(); | ||
const searchBounds = getBoundsRoundedToInterval(bounds, $scope.swimlaneBucketInterval, false); | ||
const selectedJobIds = $scope.getSelectedJobIds(); | ||
const limit = mlSelectLimitService.state.get('limit'); | ||
const swimlaneLimit = (limit === undefined) ? 10 : limit.val; | ||
const swimlaneLimit = (limit === undefined) ? 10 : limit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the default limit of 10 be moved to a constant at the top of the file?
} | ||
} | ||
} | ||
|
||
function loadViewBySwimlaneForSelectedTime(earliestMs, latestMs) { | ||
const selectedJobIds = $scope.getSelectedJobIds(); | ||
const limit = mlSelectLimitService.state.get('limit'); | ||
const swimlaneLimit = (limit === undefined) ? 10 : limit.val; | ||
const swimlaneLimit = (limit === undefined) ? 10 : limit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the default limit of 10 be moved to a constant at the top of the file?
@@ -65,6 +65,11 @@ function getDefaultViewBySwimlaneData() { | |||
}; | |||
} | |||
|
|||
const SWIMLANE_TYPE = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would explorer_constants.js
be a better place for these?
return limit; | ||
} | ||
|
||
class SelectLimit extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a corresponding Jest test. Can the current Angular based tests inside __tests__/select_limit.js
be removed if a Jest test is added?
💔 Build Failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few small comments but otherwise LGTM
) { | ||
let selectionExists = false; | ||
$scope.cellData.lanes.forEach((lane) => { | ||
if ($scope.viewBySwimlaneData.laneLabels.indexOf(lane) > -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this could be done with some
to save those few extra loops :)
// which is where d3 supplies a reference to the corresponding DOM element. | ||
return function (lane) { | ||
const bucketScore = getBucketScore(lane, time); | ||
if (bucketScore === 0) { return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, rather than breaking out of this function early, you could invert the check:
if (bucketScore !== 0) {
cellMouseover(this, lane, bucketScore, i, time);
}
/>); | ||
|
||
expect(wrapper.html()).toBe( | ||
`<div class="ml-swimlanes"><div class="time-tick-labels"><svg height="25">` + | ||
`<div class="ml-swimlanes"><div class="time-tick-labels"><svg width="800" height="25">` + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, chartWidth
should be used for the width
appState={mocks.appState} | ||
lanes={[]} | ||
mlExplorerDashboardService={mocks.mlExplorerDashboardService} | ||
chartWidth={800} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, 800
should come from a constant variable
💚 Build Succeeded |
latest changes LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest edits LGTM
💚 Build Succeeded |
…r. (elastic#23388) - This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync. - Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.
…r. (#23388) (#23440) - This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync. - Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.
…r. (elastic#23388) - This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync. - Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I'm a bit late on this one but LGTM! 😸
Thanks for the great context in the description, as well!
…r. (elastic#23388) - This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync. - Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.
💔 Build Failed |
Fixes #18996 and #18538.
This fixes the limit dropdown behavior. The fix for that is actually just the
$scope.appState.fetch();
statements inexplorer_controller.js
, they avoid to run the information stored inappState
across modules out of sync.Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.
Previously, esp. restoring a selection from URL/AppState involved a lot of complexity, something like:
Initial Load -> Load Overall Swimlane -> Load View By Swimlane -> Inside the swimlane code, check for a selection in AppState, if yes, use that data to trigger a swimlane selection click programmatically -> trigger the click listener again in Anomaly Explorer controller -> update $scope.cellData and trigger updating swimlanes/influencers/charts/table again
This has now been reduced to:
Check URL/AppState on initial load and restore $scope.cellData -> use updateExplorer() to update swimlanes/charts/influencers/table considering the selection
Here's some more detail about what's done in this PR:
renderSwimlane()
does that now. That means: click events only trigger listeners and send data (data out, no DOM manipulation);props
include all the information necessary to render the swimlanes including selections (data in, update the DOM)swimlaneDataChange
listener.isChartsContainerInitialized
to make sure the Charts Container directive is ready before passing on data to the charts. Previously child directives could miss updates if theirlink
wasn't called yet which included setting up event listeners.updateExplorer()
was introduced to be able to trigger the same kind of update when we restore or do a selection on load or via click to avoid the event circling described in the beginning. That way the complexity/redundancy ofswimlaneCellClickListener()
could be reduced, it now just saves a new selection inappState
and$scope.cellData
, then triggersupdateExporer()
.The limit dropdown has been migrated to use React, this is more or less a copy of the interval dropdown.Reverted that conversion - it was originally done while trying to fix the bug related to the dropdown but it turned out to be not related. With that conversion the dropdowns for "view by" and "limit" would end up having different appearance so that should be done in a separate PR.Everything of the above is a preparation to move
explorer_controller
at one point out of the anguarljs universe and make it an independent service that just manages state, similar to how it was done forexplorerChartsContainerService
.Part of #22642 and #18374.