[Rollup] Only allow aggregating on multiples of configured interval #32052

polyfractal · 2018-07-13T20:09:54Z

We need to limit request aggregations to whole multiples of the configured interval for both histogram and date_histogram. Otherwise, agg buckets won't overlap with the rolled buckets and the results will be incorrect.

For histogram, the validation is very simple: request must be >= the config, and modulo evenly.

Dates are tricksier.

If both request and config are fixed dates, we can convert to millis and treat them just like a regular histo (request must be >= the config, and modulo evenly)
If both are calendar, we make sure the request is >= the config with a static lookup map that ranks the calendar values relatively. All calendar units are "singles", so they are evenly divisible already
We disallow any other combination (one fixed and one calendar, etc)

As an aside, the validation of dates was apparently broken before... it converted both request and config intervals to millis without regard to fixed vs calendar. So this is both an enhancement and a bugfix :)

/cc @colings86 this is what we were chatting about the other day

/cc @jen-huang @timroes

We need to limit the search request aggregations to whole multiples of the configured interval for both histogram and date_histogram. Otherwise, agg buckets won't overlap with the rolled up buckets and the results will be incorrect. For histogram, the validation is very simple: request must be >= the config, and modulo evenly. Dates are more tricky. - If both request and config are fixed dates, we can convert to millis and treat them just like the histo - If both are calendar, we make sure the request is >= the config with a static lookup map that ranks the calendar values relatively. All calendar units are "singles", so they are evenly divisible already - We disallow any other combination (one fixed, one calendar, etc)

elasticmachine · 2018-07-13T20:09:56Z

Pinging @elastic/es-search-aggs

polyfractal · 2018-07-13T20:18:10Z

Oh, note, I want to describe the fixed/calendar semantics just a bit in the rollup docs... but I want to link to the date_histo docs once they are merged. So I'll revisit the docs some more after that PR goes in.

polyfractal · 2018-07-18T16:51:09Z

Jenkins, run sample packaging tests

jimczi

Looks good @polyfractal , I left a comment regarding one unit large interval that could be considered as fixed time interval (s, m, h and d).

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java

jimczi · 2018-07-19T11:48:44Z

x-pack/docs/en/rollup/rollup-search-limitations.asciidoc

+can only search/aggregate the rollup data with an interval that is greater-than or equal to the configured rollup interval.
+
+For example, if data is rolled up at hourlyintervals, the <<rollup-search>> API can aggregate on any time interval
+hourly or greater.  Intervals that are less than an hour will throwan exception, since the data simply doesn't


nit: throwan => "throw an"

nit: hourlyintervals => hourly intervals

x-pack/docs/en/rollup/rollup-search-limitations.asciidoc

polyfractal · 2018-08-23T20:14:39Z

@jimczi updated the docs to include a better description of the time units. Once the time docs PR merges I'll include a link to that as well.

jimczi

LGTM, I left one question. Thanks for adding the docs.

jimczi · 2018-08-23T20:24:13Z

x-pack/docs/en/rollup/rollup-getting-started.asciidoc

@@ -190,7 +190,7 @@ GET /sensor_rollup/_rollup_search
        "timeline": {
            "date_histogram": {
                "field": "timestamp",
-                "interval": "7d"
+                "interval": "1w"


Why changing to a calendar interval here ?

Oops, I was changing units to fixed and just got carried away without thinking what I was doing. Thanks for spotting

…32052) We need to limit the search request aggregations to whole multiples of the configured interval for both histogram and date_histogram. Otherwise, agg buckets won't overlap with the rolled up buckets and the results will be incorrect. For histogram, the validation is very simple: request must be >= the config, and modulo evenly. Dates are more tricky. - If both request and config are fixed dates, we can convert to millis and treat them just like the histo - If both are calendar, we make sure the request is >= the config with a static lookup map that ranks the calendar values relatively. All calendar units are "singles", so they are evenly divisible already - We disallow any other combination (one fixed, one calendar, etc)

Backport of elastic#32052, with some changes. In 6.5, we forbid mixed intervals (one fixed, one calendar). But for a 6.4.x bugfix release this would be a pretty extreme break. So we continue to allow mixed intervals in 6.4, but log a deprecation warning. 6.4 also doesn't enforce that the request is a multiple of the config. We also log a deprecation if that is not the case, but don't prevent the query. Finally, mixed intervals are only used if a better job cap can't be found. To support the case where we have to compare fixed and calendar, there is a table of rough conversions (e.g. 1 day == 86400000ms). This lets us crudely ensure the user doesn't try to query `2d` on an index that is set for `1w`.

* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)

…33284) Backport of #32052, with some changes. In 6.5, we forbid mixed intervals (one fixed, one calendar). But for a 6.4.x bugfix release this would be a pretty extreme break. So we continue to allow mixed intervals in 6.4, but log a deprecation warning. 6.4 also doesn't enforce that the request is a multiple of the config. We also log a deprecation if that is not the case, but don't prevent the query. Finally, mixed intervals are only used if a better job cap can't be found. To support the case where we have to compare fixed and calendar, there is a table of rough conversions (e.g. 1 day == 86400000ms). This lets us crudely ensure the user doesn't try to query `2d` on an index that is set for `1w`.

$polyfractal$

$@polyfractal$ polyfractal added >enhancement review v7.0.0 :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v6.4.0 labels Jul 13, 2018

polyfractal added 3 commits July 16, 2018 13:29

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_histo_multiples

5ee0e33

$@polyfractal$

Docs test fix: docs were using an incompatible time interval

4dda0ce

$@polyfractal$

Fix interval in rollup IT

fc4d403

jimczi requested changes Jul 19, 2018

View reviewed changes

$@polyfractal$ polyfractal mentioned this pull request Aug 22, 2018

[Rollup] Support range aggregations #33041

Closed

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_histo_multiples

563e276

lcawl added v6.4.1 and removed v6.4.0 labels Aug 23, 2018

$@polyfractal$

Documentation tweaks

163b422

jimczi approved these changes Aug 23, 2018

View reviewed changes

polyfractal added 4 commits August 23, 2018 16:37

$@polyfractal$

More doc tweaks

8794ffb

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_histo_multiples

010f325

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_histo_multiples

d939c4d

$@polyfractal$

Fix intervals in docs

579f436

$@polyfractal$ polyfractal mentioned this pull request Aug 28, 2018

Basic Rollup implementation for Metricbeat elastic/beats#7220

Closed

polyfractal added 3 commits August 29, 2018 12:16

$@polyfractal$

MOre doc tweaks

adf4e74

$@polyfractal$

Merge remote-tracking branch 'origin/master' into rollup_histo_multiples

205574d

$@polyfractal$

checkstyle

0ba466a

$@polyfractal$ polyfractal added v6.5.0 and removed review labels Aug 29, 2018

$@polyfractal$ polyfractal removed the v6.4.1 label Aug 29, 2018

$@polyfractal$ polyfractal merged commit d93b2a2 into elastic:master Aug 29, 2018

$@polyfractal$ polyfractal mentioned this pull request Aug 30, 2018

[Rollup] Log deprecation if config and request intervals are mixed #33284

Merged

$@polyfractal$ polyfractal mentioned this pull request Sep 2, 2018

[Rollup] Fix Caps Comparator to handle calendar/fixed time #33336

Merged

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

codebrain mentioned this pull request Jan 28, 2019

[meta] 6.6.0 Release elastic/elasticsearch-net#3552

Closed

48 tasks

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rollup] Only allow aggregating on multiples of configured interval #32052

[Rollup] Only allow aggregating on multiples of configured interval #32052

$@polyfractal$ polyfractal commented Jul 13, 2018

elasticmachine commented Jul 13, 2018

polyfractal commented Jul 13, 2018

polyfractal commented Jul 18, 2018

jimczi left a comment

jimczi Jul 19, 2018

jimczi Jul 19, 2018

polyfractal commented Aug 23, 2018

jimczi left a comment

jimczi Aug 23, 2018

$@polyfractal$ polyfractal Aug 23, 2018

[Rollup] Only allow aggregating on multiples of configured interval #32052

[Rollup] Only allow aggregating on multiples of configured interval #32052

Conversation

polyfractal commented Jul 13, 2018

elasticmachine commented Jul 13, 2018

polyfractal commented Jul 13, 2018

polyfractal commented Jul 18, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi Jul 19, 2018

Choose a reason for hiding this comment

jimczi Jul 19, 2018

Choose a reason for hiding this comment

polyfractal commented Aug 23, 2018

jimczi left a comment

Choose a reason for hiding this comment

jimczi Aug 23, 2018

Choose a reason for hiding this comment

polyfractal Aug 23, 2018

Choose a reason for hiding this comment

$@polyfractal$ polyfractal commented Jul 13, 2018

$@polyfractal$ polyfractal Aug 23, 2018