Skip to content
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

cleaning up esaggs #77646

Merged
merged 4 commits into from
Sep 19, 2020
Merged

cleaning up esaggs #77646

merged 4 commits into from
Sep 19, 2020

Conversation

ppisljar
Copy link
Member

Summary

esaggs was using calculateObjectHash method to

  1. calculate hash of the es request object, to compare it later to figure out if new request is needed
  2. calculate hash of the tabify input, to figure out if we need to recalculate tabified data

since introducing expressions, this was no longer functioning as expected as searchsource on which the hashes were stored is recreated on every execution.

in the expression world this should be solved by expression caching mechanism and should not require doing things like this.

this PR removes this functionality, which was not working and was causing significant performance hit.

@ppisljar ppisljar requested a review from a team as a code owner September 16, 2020 17:23
@ppisljar ppisljar added chore Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.10.0 v8.0.0 labels Sep 16, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code LGTM.

Comment on lines -143 to -145
// We only need to reexecute the query, if forceFetch was true or the hash of the request body has changed
// since the last request
const shouldQuery = forceFetch || (searchSource as any).lastQuery !== queryHash;
Copy link
Member

Choose a reason for hiding this comment

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

Should we be removing the forceFetch request handler param as well, since it doesn't do anything at this point?

@ppisljar ppisljar requested a review from a team September 17, 2020 04:14
@ppisljar
Copy link
Member Author

@elasticmachine merge upstream

@ppisljar
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 18, 2020

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
data 1.5MB -1.7KB 1.5MB
visTypeTimelion 715.9KB -16.0B 716.0KB
visualizations 412.5KB -16.0B 412.5KB
total -1.7KB

History

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

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tested locally in chrome. Ran through some sanity checks in visualize and lens to make sure everything still works as expected.

Everything LGTM

@ppisljar ppisljar merged commit 8408e26 into elastic:master Sep 19, 2020
ppisljar added a commit to ppisljar/kibana that referenced this pull request Sep 19, 2020
ppisljar added a commit that referenced this pull request Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants