Merged
Conversation
legrego
approved these changes
Apr 30, 2020
Member
legrego
left a comment
There was a problem hiding this comment.
LGTM on green CI - tested different combinations of TSVB visualizations, and I'm not noticing any issues
Contributor
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
flash1293
reviewed
May 1, 2020
Contributor
flash1293
left a comment
There was a problem hiding this comment.
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.
Contributor
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
legrego
approved these changes
May 4, 2020
Member
legrego
left a comment
There was a problem hiding this comment.
Tested updated implementation, still LGTM!
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
added a commit
that referenced
this pull request
May 4, 2020
legrego
added a commit
that referenced
this pull request
May 4, 2020
legrego
added a commit
that referenced
this pull request
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
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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- [ ] This was checked for keyboard-only and screenreader accessibility- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser- [ ] This was checked for cross-browser compatibility, including a check against IE11For maintainers