Skip to content

Add rest_total_hits_as_int into Kibana App - #26404

Merged
timroes merged 6 commits into
elastic:masterfrom
timroes:total-hits-kibana-app
Dec 4, 2018
Merged

timroes merged 6 commits into
elastic:masterfrom
timroes:total-hits-kibana-app

Conversation

@timroes

@timroes timroes commented Nov 29, 2018

Copy link
Copy Markdown
Contributor

Summary

Implements Step 1 of #26356.

This will add rest_total_hits_as_int: true to all requests in Kibana App code, where we later potentially call hits.total on. This includes the following places:

  • Courier (Rollup and default search strategy)
  • TSVB (all requests, though I think we actually call hits.total not on all results, but better being safe than sorry)
  • Graph (in the place we read out hits.total later)
  • Reporting

Unfortunately those changes are not testable right now :-(
The PR should pass, since the parameter is accepted (but has no effect) in ES right now. As soon as elastic/elasticsearch#35849 is merged, this PR should make sure, that the Kibana App code will still work against ES master. Unfortunately we can also not properly test it against that PR version, because even if I let this PR run against that PR in it's own branch (which our CI can), the tests will fail, because all other (non Kibana App) places, haven't adjusted to that parameter yet, and thus tests from ML, APM, etc. will just fail.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@timroes timroes added v7.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v6.6.0 labels Nov 29, 2018
@elasticmachine

Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-app

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@timroes

timroes commented Nov 29, 2018

Copy link
Copy Markdown
Contributor Author

Jenkins, test again - CI seemed to never have started

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@timroes

timroes commented Nov 29, 2018

Copy link
Copy Markdown
Contributor Author

Jenkins, test this - ES master should accept this now

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@timroes

timroes commented Nov 30, 2018

Copy link
Copy Markdown
Contributor Author

Jenkins, test this - now the change should be in master :)

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@timroes

timroes commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

Jenkins, test this

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@timroes

timroes commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

Jenkins, Test this - CI failure

@elasticmachine

Copy link
Copy Markdown
Contributor

💔 Build Failed

@timroes

timroes commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

Jenkins, test this

@elasticmachine

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@timroes
timroes requested a review from ppisljar December 3, 2018 13:31
@ppisljar

ppisljar commented Dec 3, 2018

Copy link
Copy Markdown
Contributor

timelion ?

@timroes

timroes commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

Indeed. Should have the same issue, even though I couldn't see hits.total being accessed directly anywhere, but will dig a bit deeper, to see where it's accessed and fix that too.

@timroes

timroes commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

@ppisljar Timelion is using a bucket script aggregation to gather the count (see https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/timelion/server/series_functions/es/lib/create_date_agg.js#L46:L51) which is not effected by that change.

@timroes
timroes requested a review from markov00 December 4, 2018 08:01
@markov00

markov00 commented Dec 4, 2018

Copy link
Copy Markdown
Contributor

@timroes what about vega? specially in the case where some existing/saved vega visualization are using the total count.

@timroes

timroes commented Dec 4, 2018

Copy link
Copy Markdown
Contributor Author

I will create a separate issue for Vega, since that requires some more discussion. Please feel free to review this PR as is :)

@markov00 markov00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code LGTM. Tested on various dashboard/visualization and all network requests sends out the rest_total_hits_as_int flag correctly.

@timroes
timroes merged commit 1852b4b into elastic:master Dec 4, 2018
@timroes
timroes deleted the total-hits-kibana-app branch December 4, 2018 09:43
timroes added a commit to timroes/kibana that referenced this pull request Dec 4, 2018
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v6.6.0 v7.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants