Rollup adding support for date field metrics (#34185) #34200

benwtrent · 2018-10-01T19:39:35Z

This allows date data field types to be rolled up with our supported metric aggregations.

elasticmachine · 2018-10-01T19:39:36Z

Pinging @elastic/es-search-aggs

colings86

I think we are going to want to restrict the supported metrics to min, max, and value_counthere. The reason is that apart fromsumandavg` not making much sense, it would take less than 10,000,000 rolled up documents to overflow a long with dates which would probably lead to some very strange behaviours. I can see people reaching 10,000,000 original documents in a single rolled up document but more than that I can definitely see this overflow happening on the query side.

benwtrent · 2018-10-11T11:13:32Z

@colings86 Think I addressed your issue, just waiting on @polyfractal for the final stamp of approval :)

colings86

I left a nit comment but otherwise LGTM. I would like @polyfractal to give it the green light before this is merged though

colings86 · 2018-10-15T12:59:23Z

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

@@ -36,6 +37,9 @@
    public static final String ROLLUP_MISSING = "ROLLUP_MISSING_40710B25931745D4B0B8B310F6912A69";
    public static final List<String> SUPPORTED_METRICS = Arrays.asList(MaxAggregationBuilder.NAME, MinAggregationBuilder.NAME,


nit: Can we rename this SUPPORTED_NUMERIC_METRICS now?

I wonder if we should have SUPPORTED_NUMERIC_METRICS, SUPPORTED_DATE_METRICS, and then a union of those in a third list SUPPORTED_METRICS?

E.g. in the MetricsConfig constructor (and a few other places), we validate that an agg is one of the supported metrics. We don't yet have the field mapper type, so we can't validate numeric vs date field and their associated metrics, but it's a quick check to make sure the config is at least sane.

The current SUPPORTED_METRICS is technically a superset of the date metrics too so everything is fine. But I'm worried in the future the two lists may diverge for whatever reason, and we miss the validation because dates support something numerics don't, or vice versa.

So, I am making SUPPORTED_NUMERIC_METRICS and SUPPORTED_DATE_METRICS two separate lists with overlapping metrics.

Then there is a Set<String> that will be SUPPORTED_METRICS that is a union of the two (need to verify that the type change will not break other logic). This should lay the ground work/pattern for future non-overlapping supported metrics.

polyfractal

Left a few questions, minor tweaks.

~~Also, I think we need to add a test to RollupRequestTranslationTests too. I believe the translation will "work" (not throw an error), but won't translate correctly due to field naming conventions.~~

~~Timestamps are stored as foo.date_histogram.timestamp + foo.date_histogram.interval + foo.date_histogram.time_zone. Metric values, however, are stored as foo.max.value.~~

Which means we treat all metrics as defaulting to .value when rewriting, and that won't match the .timestamp that date_histos use. I think that will break the rewriting, and we need to figure out a way to change from .value to .timestamp. I haven't looked to closely yet, but we could probably do something like detect if the agg is targeting the string "date_histogram" (since the convention is defined) and switch on that.

~~Probably good to verify on the response translation side too... but I think that side should "just work" once the request is rewritten correctly.~~

~~And yes, it would have been better if date_histos just used .value for their timestamp. alas :(~~

polyfractal · 2018-10-15T14:08:52Z

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

@@ -36,6 +37,9 @@
    public static final String ROLLUP_MISSING = "ROLLUP_MISSING_40710B25931745D4B0B8B310F6912A69";
    public static final List<String> SUPPORTED_METRICS = Arrays.asList(MaxAggregationBuilder.NAME, MinAggregationBuilder.NAME,


I wonder if we should have SUPPORTED_NUMERIC_METRICS, SUPPORTED_DATE_METRICS, and then a union of those in a third list SUPPORTED_METRICS?

E.g. in the MetricsConfig constructor (and a few other places), we validate that an agg is one of the supported metrics. We don't yet have the field mapper type, so we can't validate numeric vs date field and their associated metrics, but it's a quick check to make sure the config is at least sane.

The current SUPPORTED_METRICS is technically a superset of the date metrics too so everything is fine. But I'm worried in the future the two lists may diverge for whatever reason, and we miss the validation because dates support something numerics don't, or vice versa.

polyfractal · 2018-10-15T14:09:19Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java

@@ -108,18 +109,26 @@ public void validateMappings(Map<String, Map<String, FieldCapabilities>> fieldCa
        Map<String, FieldCapabilities> fieldCaps = fieldCapsResponse.get(field);
        if (fieldCaps != null && fieldCaps.isEmpty() == false) {
            fieldCaps.forEach((key, value) -> {
-                if (RollupField.NUMERIC_FIELD_MAPPER_TYPES.contains(key)) {
+                if (RollupField.NUMERIC_FIELD_MAPPER_TYPES.contains(key) || RollupField.DATE_FIELD_MAPPER_TYPE.equals(key)) {


Now that there are two cases, I wonder if we should push the isAggregatable() check out of the type conditionals (e.g. apply to everything), then have an if...if else...else for the types? That way we won't need another conditional inside the first that specializes for the date type.

Not sure how that would look, so the current way is fine if that ends up being messier. :)

Has a nice side effect that the validation error will include both a message about non-aggregatable field as well as the metric missing at the same time.

polyfractal · 2018-10-15T14:09:30Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/MetricConfig.java

+                        RollupField.SUPPORTED_DATE_METRICS.containsAll(metrics) == false) {
+                        List<String> unsupportedMetrics = new ArrayList<>(metrics);
+                        unsupportedMetrics.removeAll(RollupField.SUPPORTED_DATE_METRICS);
+                        validationException.addValidationError("Only the metrics " + RollupField.SUPPORTED_DATE_METRICS.toString() +


Super tiny nit: could we wrap the SUPPORTED_DATE_METRICS with brackets ([...])? Makes it easier for the user to see the difference in message and dynamic values.

Ditto to below with unsupportedMetrics

@polyfractal A Collection<String> object when transformed to a String transforms to a [Metric1, Metric2]. To clarify, you want me to add another bracket so that the printout will be [[Metric1, Metric2]] ?

Reference Repl: https://repl.it/@ben_w_trent/QuerulousSubtleApplicationframework

Ah, nope, I'm just bad at Java. [Metric1, Metric2] is exactly what I was wanting. :)

polyfractal · 2018-10-15T16:04:33Z

Chatted on Slack, the rewriting .timestamp business doesn't apply (I confused myself). We will be adding distinct min/max metrics on the timestamp field, rather than reusing the .timestamp field that the date_histo generates.

Introduces a bit of duplication in the document but keeps everything simple.

polyfractal · 2018-10-16T13:46:05Z

LGTM thanks! <3

* Rollup adding support for date field metrics (#34185) * Restricting supported metrics for `date` field rollup * fixing expected error message for yaml test * Addressing PR comments

Rollup adding support for date field metrics (elastic#34185)

81add30

benwtrent added >enhancement review :Analytics/Aggregations Aggregations v7.0.0 v6.5.0 labels Oct 1, 2018

colings86 requested changes Oct 2, 2018

View reviewed changes

benwtrent added 3 commits October 2, 2018 07:12

Merge branch 'master' into feature/rollup-add-date-metric-support

0edd4ad

Restricting supported metrics for date field rollup

f0e0624

fixing expected error message for yaml test

e58405d

benwtrent requested a review from polyfractal October 3, 2018 20:58

rjernst removed the review label Oct 10, 2018

colings86 approved these changes Oct 15, 2018

View reviewed changes

$polyfractal$

polyfractal suggested changes Oct 15, 2018

View reviewed changes

Merge branch 'master' into feature/rollup-add-date-metric-support

c75f4c0

Addressing PR comments

42c1638

$polyfractal$

polyfractal approved these changes Oct 16, 2018

View reviewed changes

benwtrent merged commit af2877c into elastic:master Oct 16, 2018

benwtrent deleted the feature/rollup-add-date-metric-support branch October 16, 2018 13:55

Mpdreamz mentioned this pull request Dec 13, 2018

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

Closed

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 adding support for date field metrics (#34185) #34200

Rollup adding support for date field metrics (#34185) #34200

benwtrent commented Oct 1, 2018

elasticmachine commented Oct 1, 2018

colings86 left a comment

benwtrent commented Oct 11, 2018

colings86 left a comment

colings86 Oct 15, 2018

$@polyfractal$ polyfractal Oct 15, 2018

benwtrent Oct 15, 2018

$@polyfractal$ polyfractal left a comment •

edited

$@polyfractal$ polyfractal Oct 15, 2018

$@polyfractal$ polyfractal Oct 15, 2018

$@polyfractal$ polyfractal Oct 15, 2018

benwtrent Oct 15, 2018

$@polyfractal$ polyfractal Oct 15, 2018

polyfractal commented Oct 15, 2018

polyfractal commented Oct 16, 2018

		@@ -36,6 +37,9 @@
		public static final String ROLLUP_MISSING = "ROLLUP_MISSING_40710B25931745D4B0B8B310F6912A69";
		public static final List<String> SUPPORTED_METRICS = Arrays.asList(MaxAggregationBuilder.NAME, MinAggregationBuilder.NAME,

Rollup adding support for date field metrics (#34185) #34200

Rollup adding support for date field metrics (#34185) #34200

Conversation

benwtrent commented Oct 1, 2018

elasticmachine commented Oct 1, 2018

colings86 left a comment

Choose a reason for hiding this comment

benwtrent commented Oct 11, 2018

colings86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal commented Oct 15, 2018

polyfractal commented Oct 16, 2018

$@polyfractal$ polyfractal left a comment •

edited