Skip to content

Conversation

@majagrubic
Copy link
Contributor

Summary

I was playing around with TSVB visualization to answer a Discuss question and noticed that sometimes Chrome freezes entirely when TSVB is open, which led me to believe there's a memory leak somewhere. I am not 100% sure it was caused by lodash set, but after removing it, I couldn't reproduce the freezing of Chrome and thought I'd open this for review, in case you think it's beneficial.

Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM on green CI - tested different combinations of TSVB visualizations, and I'm not noticing any issues

@majagrubic majagrubic marked this pull request as ready for review April 30, 2020 20:00
@majagrubic majagrubic requested review from a team and flash1293 April 30, 2020 20:00
@majagrubic majagrubic added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Apr 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

I tested TSVB and did a sanity check of all the functions touched in this PR. I couldn't notice any breakage.

I reviewed the code, but it's hard to tell whether there's another place like https://github.com/elastic/kibana/pull/64918/files#diff-1accfc2b04681b275b52ae76e6ddd9b1R39 due to the lack of types. Also, I had a hard time parsing the logic - so many aggs :D

From my understanding the difference in behavior between lodash and set-value is this:

let a = { deep: { nested: { path: { b: '123' } } } }
set(a, 'deep.nested.path', { c: '456' }); // { deep: { nested: { path: { b: '123', c: '456' } } } }
_.set(a, 'deep.nested.path', { c: '456' }); // { deep: { nested: { path: { c: '456' } } } }

We could do something like this:

import set from 'set-value';

export overwrite(obj, path, val) {
  set(obj, path, undefined);
  set(obj, path, val);
}

and then use overwrite instead of set directly in all the places you touched. What do you think @majagrubic ? Seems like a small adjustment and then we can be safe(r) no logic is breaking.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@legrego legrego self-requested a review May 4, 2020 10:20
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Tested updated implementation, still LGTM!

@legrego legrego merged commit ccede29 into elastic:master May 4, 2020
legrego pushed a commit to legrego/kibana that referenced this pull request May 4, 2020
legrego pushed a commit to legrego/kibana that referenced this pull request May 4, 2020
@legrego legrego added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:fix labels May 4, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 4, 2020
…ana into alerting/np-tests-migration

* 'alerting/np-tests-migration' of github.com:gmmorris/kibana:
  [APM] Agent remote config: validation for Java agent configs (elastic#63956)
  [APM] Fix duplicate index patterns (elastic#64883)
  Drilldowns (elastic#61219)
  [Alerting] fix labels and links in PagerDuty action ui and docs (elastic#64032)
  [Event Log] Ensure sorting tests are less flaky (elastic#64781)
  update endpoint to restrict removing with datasources (elastic#64978)
  [Logs UI] [Alerting] Alerts management page enhancements (elastic#64654)
  Adjust kibana app owning files (elastic#65064)
  Migrate tutorial resources (elastic#64298)
  [Logs UI] Tweak copy in log alerts dialog (elastic#64645)
  [Logs UI] [Alerting] Documentation (elastic#64886)
  [Logs UI] Add dataset filter to ML module setup screen (elastic#64470)
  [TSVB] Fixing memory leak (elastic#64918)
  Bump backport to 5.4.1 (elastic#65041)
timroes pushed a commit to timroes/kibana that referenced this pull request May 7, 2020
majagrubic pushed a commit to majagrubic/kibana that referenced this pull request May 7, 2020
# Conflicts:
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/annotations/date_histogram.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/series/date_histogram.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/series/positive_rate.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/series/split_by_filter.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/series/split_by_filters.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/table/date_histogram.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/table/split_by_everything.js
#	src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/table/split_by_terms.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v6.8.9 v7.7.0 v7.8.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants