-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve performance of tap table by throttling updates #1623
Conversation
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 great! Thanks for fixing this! Had one comment but I don't think it should block this from being merged.
web/app/js/components/Tap.jsx
Outdated
@@ -22,11 +22,14 @@ class Tap extends React.Component { | |||
constructor(props) { | |||
super(props); | |||
this.api = this.props.api; | |||
this.tapResultsById = {}; | |||
this.tapResultFilterOptions = this.getInitialTapFilterOptions(); | |||
this.debouncedWebsocketRecvHandler = _.throttle(this.updateTapResults, 500); |
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 am wondering if this should be renamed to throttleWebsocketRecvHandler
instead. I realize that this is still called debounced...
in TopModule.jsx so it's probably fine to keep it as it is.
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.
🥇 🌟 thanks for making this fix!
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
e2018fe
to
7f95c3d
Compare
* master: Update CHANGES.md for v18.9.1 release (linkerd#1631) Cleanly shutdown tap stream to data plane proxies (linkerd#1624) Change breadcrumb header to default font in styles.css (linkerd#1633) Improve top table to better cope with high RPS traffic (linkerd#1634) Add small success rate chart to table, misc web tweaks (linkerd#1628) Consolidate the source and destination columns in the Tap and Top tables (linkerd#1620) remove extraneous calc function in sidebar.css (linkerd#1632) Display more helpful websocket errors (linkerd#1626) Add breadcrumb navigation at the top of linkerd dashboard (linkerd#1613) Introduce inject check for known sidecars (linkerd#1619) Bump Prometheus to v2.4.0, Grafana to 5.2.4 (linkerd#1625) Improve performance of tap table by throttling updates (linkerd#1623) Add with-source flag to top (linkerd#1614) Conflicts: web/app/css/styles.css web/app/js/components/ResourceDetail.jsx web/app/js/index.js
This branch updates the tap table to limit re-rendering to once every 500ms, similar to the approach taken for the top table in #1616. With this change in place I'm able to tap an entire namespace without the page freezing. It's still the case that this table renders pretty slowly (~250ms), but given the 500ms throttling it's not a problem. In the long run, however, I'd like to reduce the amount of data in this table to speed up rendering.
Fixes #1597.