-
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
Replace lodash template with static react renderer for discover's row formatter #98072
Replace lodash template with static react renderer for discover's row formatter #98072
Conversation
src/plugins/discover/public/application/angular/helpers/row_formatter.tsx
Show resolved
Hide resolved
@elasticmachine merge upstream ACK will have a look today |
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.
Code LGTM, 👍 . dangerouslySetInnerHTML is safe here due to the use of field formatters. There is one regression here, performance. so the new implementation needs about double of the time to execute.
Old: 2.72 ms in this case
New: 5.15 ms in this case
This sums up when there are multiple rows rendered. However I don't think this is a blocker, since we recently switched Discover to the use of #89264 , so it's just used by the legacy table. Do you agree dear @timroes ?
@kertal I would consider this to be fine (also security outwins performance anyway). I am also not sure if in this very fast are we can really rely on the "ratio" between those numbers or if there might be too much variation in it. If we can quickly look into the react performance and figure there's an easy win (not sure how often that's rendered, maybe React.memo will help us here) we can still make it, but I am also fine merging it as it is. |
about the variation, tested in with multiple docs also, generally, performance regression is about the same |
Sounds like the performance impact is tolerable then - thanks for verifying! Would be great to get an official approval whenever y'all get the chance - this isn't a huge priority, I just don't want to forget about it |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
In this case React.memo won't help, it's rendered in every row, and usually docs are are not equal unless you've got a set of duplicate docs (but also then you would have to check it with |
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.
Code LGTM 👍 , didn't test again, but there were no changes to my first test. The performance regression is acceptable, security > performance in this case!
… formatter (elastic#98072) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
… formatter (elastic#98072) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Replaces the rendering logic for Discover's
row_formatter
. Previous implementation useslodash.template
, which requires unsafe code execution (unsafe-eval
in CSP parlance). New implementation uses ReactDOM to render static markup.This is a prerequisite for hardening our content security policy.
Related: #94626