Skip to content

[ML] Anomaly detection explorer: ensure user can change number of rows in the anomalies table #98232

Closed
alvarezmelissa87 wants to merge 2 commits intoelastic:masterfrom
alvarezmelissa87:ml-dfa-table-page-fix
Closed

[ML] Anomaly detection explorer: ensure user can change number of rows in the anomalies table #98232
alvarezmelissa87 wants to merge 2 commits intoelastic:masterfrom
alvarezmelissa87:ml-dfa-table-page-fix

Conversation

@alvarezmelissa87
Copy link
Copy Markdown
Contributor

Summary

Fixes #98117

Looks like the problem is in the call to decode https://github.com/elastic/kibana/blob/master/x-pack/plugins/ml/public/application/util/url_state.tsx#L51 - looks like the mlAnomaliesTable bit of the URL state (in _a) causes the rison encoding to go into an endless loop of calls.

I think it's due to the page info that goes into the state directly. Looks like it must be a reference thing 🤔
This PR creates a copy of the page object and then sends that along in the state to be encoded. This fixes the issue - I spent some time in the rison code base trying to find out why that endless loop was happening resulting in the stack size exceeded error. I'm still not 100 percent on exactly which bit is breaking it.

@darnautov - might be worth chatting over the issue.

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 changed the title ensure page update works [ML] Anomaly detection explorer: ensure user can change number of rows in the anomalies table Apr 23, 2021
@peteharverson peteharverson added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels Apr 26, 2021
Copy link
Copy Markdown
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@darnautov
Copy link
Copy Markdown
Contributor

@elasticmachine merge upstream

sortField: sort && sort.field !== undefined ? sort.field : tableState.sortField,
pageIndex: page && page.index !== undefined ? pageCopy.index : tableState.pageIndex,
pageSize: page && page.size !== undefined ? pageCopy.size : tableState.pageSize,
sortField: sort && sort.field !== undefined ? pageCopy.field : tableState.sortField,
Copy link
Copy Markdown
Contributor

@darnautov darnautov Apr 26, 2021

Choose a reason for hiding this comment

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

The actual problem is with sortField and changes in your PR replace it with pageCopy.field which is always undefined because the updates come from the sort property. Check #98270 for more details.

Also important to note, in JavaScript, numbers and strings are not a reference type, hence creatingpageCopy doesn't affect anything, because we access index and size by value at all times.

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

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
ml 5.9MB 5.9MB +32.0B

History

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

cc @alvarezmelissa87

@alvarezmelissa87
Copy link
Copy Markdown
Contributor Author

Closing in lieu of #98270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes v7.13.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ML] Cannot change number of rows in the anomalies table

5 participants