[TSDB] Add support for downsampling `aggregate_metric_double` fields by csoulios · Pull Request #90029 · elastic/elasticsearch

csoulios · 2022-09-13T12:37:43Z

This PR adds support for downsampling metric fields of type aggregate_metric_double, enabling the rollup-of-rollups functionality.

to the test index mapping

Change how we serialize metrics to doc

for computing metrics

elasticsearchmachine · 2022-09-13T17:09:16Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

...gin/rollup/src/main/java/org/elasticsearch/xpack/downsample/AbstractRollupFieldProducer.java

nik9000 · 2022-09-13T17:14:10Z

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

+        assert fieldType != null : "Unknown field type for field: [" + field + "]";
+        Map<String, FieldValueFetcher> fetchers = new HashMap<>();
+
+        if (fieldType instanceof AggregateDoubleMetricFieldMapper.AggregateDoubleMetricFieldType aggMetricFieldType) {


Is this one of the threads you'd pull on to fix the other instanceof checks? If the field type built the fetcher - or if we had a Map<Class<FieldType>, FetcherFactory> or something?

...rollup/src/test/java/org/elasticsearch/xpack/downsample/DownsampleActionSingleNodeTests.java

exists in the index.

csoulios · 2022-09-15T12:34:47Z

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

+                }
+            }
+        } else {
+            if (context.fieldExistsInIndex(field)) {


Include only the fields that exist in the index, instead of all mapped field.

This should improve downsampling performance for cases such as ECS schema.

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

...rollup/src/test/java/org/elasticsearch/xpack/downsample/DownsampleActionSingleNodeTests.java

nik9000

java code looks fine to me. I'd love to plumb the instanceof checks through another way, but that's fine for a follow up change. I wish you luck fighting your gradle demons.

nik9000 · 2022-09-19T20:56:52Z

...lugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TimeseriesFieldTypeHelper.java

        public TimeseriesFieldTypeHelper build(final String timestampField) throws IOException {
-            final MapperService mapperService = indicesService.createIndexMapperServiceForValidation(indexMetadata);
-            final CompressedXContent sourceIndexCompressedXContent = new CompressedXContent(indexMapping);
-            mapperService.merge(MapperService.SINGLE_MAPPING_NAME, sourceIndexCompressedXContent, MapperService.MergeReason.INDEX_TEMPLATE);


Was this important?

Ah. It moved.

Yes, I moved it one level up so that I can reuse the MapperService instance

rollup module extends mapper-aggregate-metric instead of importing it via `api`. This solves some Classloading issues

from rollup build.gradle file

csoulios · 2022-09-20T11:12:48Z

@elasticmachine run elasticsearch-ci/docs

Moved FieldValueFetcher in the mappers package so that it can be retrieved through the MappedFieldType and remove the instanceof clause

csoulios · 2022-09-20T12:10:22Z

@nik9000 I have sorted out the classloading issue. I had to modify the rollup x-pack plugin so that it extends the x-pack-aggregate-metric plugin. This solved the problem. For more details see b39ee34

Finally, I made an effort to move the FieldValueFetchers inside the server module so that it is returned by MappedFieldType. As discussed, this removes the instanceof condtional, but spills the downsmpling logic in the mappers code. To be honest I am not sure what's best (or what's worse). Also, it does not remove the instanceof conditionals when creating MetricFieldProducer and LabelFieldProducer instances.
The change can be seen in 69664b0. If you don't like it, I am happy to revert it.

With all those changes, I think the PR is ready for one more review.
Thanks for all the iterations

csoulios · 2022-09-20T12:12:08Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

+    /**
+     * Create a helper class to fetch value field data.
+     */
+    public FieldValueFetcher fieldValueFetcher(SearchExecutionContext context) {


I know that naming this fieldValueFetcher looks confusing, especially when the method is right below a method named valueFetcher, but I did not have any better idea (maybe fieldDataFetcher, but again there's fieldDataBuilder). I am open to suggestions

I would prefer to keep this in the rollup module, but this works for now. I can put together a followup PR with some ideas once this is merged. Or I can try. And if I fail then, well, I'll know.

csoulios · 2022-09-20T12:15:00Z

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

-/**
- * Utility class used for fetching field values by reading field data
- */
-class FieldValueFetcher {


This class became an interface and moved into server module in the org.elasticsearch.index.mapper package.

The static methods were moved in the RollupShardIndexer class

csoulios · 2022-09-20T12:16:09Z

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

+        final Map<String, FormattedDocValues> docValuesFetchers = new LinkedHashMap<>(fieldValueFetchers.size());
+        for (FieldValueFetcher fetcher : fieldValueFetchers) {
+            for (Map.Entry<String, FormattedDocValues> e : fetcher.getLeaves(ctx).entrySet()) {
+                if (searchContext.fieldExistsInIndex(e.getKey())) {


If field does not exist in the index, it will be ignored even if it exists in the mapping. This should improve performance for use cases such as ECS

nik9000 · 2022-09-20T14:24:26Z

@nik9000 I have sorted out the classloading issue. I had to modify the rollup x-pack plugin so that it extends the x-pack-aggregate-metric plugin. This solved the problem. For more details see b39ee34

got it. I don't really know if there is a "right" way to do this, but I'm just happy this works.

nik9000

LGTM.

I do think it's be nice to keep the fetchers in the rollup module and I think I know how. But let's get this in and I can make a PR and we can see if it's a reasonable way.

nik9000 · 2022-09-20T14:25:29Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

+    /**
+     * Create a helper class to fetch value field data.
+     */
+    public FieldValueFetcher fieldValueFetcher(SearchExecutionContext context) {


I would prefer to keep this in the rollup module, but this works for now. I can put together a followup PR with some ideas once this is merged. Or I can try. And if I fail then, well, I'll know.

nik9000 · 2022-09-20T14:26:54Z

server/src/main/java/org/elasticsearch/index/mapper/FieldValueFetcher.java

+/**
+ * Interface for retrieving field values by reading field data
+ */
+public interface FieldValueFetcher {


If it's in server it's nice to have a very good name. Its a FormattedDocValuesFetcher or something. I dunno. Probably not worth changing it now. If I can't figure out a good way to move it back to the module then we should rename it.

This reverts commit 69664b0.

csoulios · 2022-09-20T15:44:53Z

I reverted the last change. I moved the FieldValueFetcher class back to the rollup module.I am going to merge this PR as is. Let's work on a following PR on how to remove the instanceof conditionals

csoulios · 2022-09-20T16:12:38Z

@elasticmachine run elasticsearch-ci/bwc

csoulios · 2022-09-20T17:04:37Z

@elasticmachine run elasticsearch-ci/part-1

csoulios added 10 commits September 5, 2022 15:07

Use XContentBuilder for index mapping

3af2f77

Merge branch 'main' into tsdb-rollup-of-rollups

88ce0d2

Merge branch 'main' into tsdb-rollup-of-rollups

5bde2a6

Added aggregate_metric_double field

695da28

to the test index mapping

initial support for rollup of rollups

977eeb2

Merge branch 'main' into tsdb-rollup-of-rollups

d582542

Merge branch 'main' into tsdb-rollup-of-rollups

d2befc7

Add fetcher support for aggregate_double_metric

146c924

Change how we serialize metrics to doc

Collect aggregate_metric_double fields

c10fd78

for computing metrics

Cleanup

cebdc91

elasticsearchmachine added the v8.5.0 label Sep 13, 2022

csoulios added :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data >non-issue :StorageEngine/TSDB You know, for Metrics labels Sep 13, 2022

Fix gradle dependencies

6c4e297

csoulios marked this pull request as ready for review September 13, 2022 17:08

csoulios requested a review from nik9000 September 13, 2022 17:08

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 13, 2022

nik9000 reviewed Sep 13, 2022

View reviewed changes

csoulios added 6 commits September 13, 2022 22:04

Added testRollupOfRollups test

07d02d2

Format test

fde224f

javadoc

32927a3

Merge branch 'main' into tsdb-rollup-of-rollups

548c96c

Merge branch 'main' into tsdb-rollup-of-rollups

fc27e34

Create fetcher only if mapped field

bf4ded9

exists in the index.

csoulios commented Sep 15, 2022

View reviewed changes

csoulios added 2 commits September 15, 2022 19:31

Removed fuction that was not used

fe63511

Support aggregate_metric_double label fields

ee82647

csoulios commented Sep 15, 2022

View reviewed changes

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/FieldValueFetcher.java Show resolved Hide resolved

csoulios commented Sep 15, 2022

View reviewed changes

...rollup/src/test/java/org/elasticsearch/xpack/downsample/DownsampleActionSingleNodeTests.java Show resolved Hide resolved

csoulios added 3 commits September 15, 2022 22:48

cleanup

dd390af

Merge branch 'main' into tsdb-rollup-of-rollups

253c88a

Validate downsampling interval

5bc0cb4

csoulios mentioned this pull request Sep 19, 2022

Add better support for metric data types (TSDB) #74660

Closed

nik9000 reviewed Sep 19, 2022

View reviewed changes

csoulios added 4 commits September 20, 2022 01:15

Add tests for rollups of rollups

08c8fe5

Use aggregate_metric field mapper as plugin

b39ee34

rollup module extends mapper-aggregate-metric instead of importing it via `api`. This solves some Classloading issues

Merge branch 'main' into tsdb-rollup-of-rollups

113bf91

Removed unused dependency

6c1597e

from rollup build.gradle file

Move FieldValueFetcher into server module

69664b0

Moved FieldValueFetcher in the mappers package so that it can be retrieved through the MappedFieldType and remove the instanceof clause

csoulios requested a review from nik9000 September 20, 2022 12:10

csoulios commented Sep 20, 2022

View reviewed changes

nik9000 approved these changes Sep 20, 2022

View reviewed changes

Revert "Move FieldValueFetcher into server module"

f52f2d0

This reverts commit 69664b0.

Merge branch 'main' into tsdb-rollup-of-rollups

48e7bf9

csoulios merged commit b9f20e9 into elastic:main Sep 20, 2022

csoulios deleted the tsdb-rollup-of-rollups branch September 20, 2022 17:44

Conversation

csoulios commented Sep 13, 2022

Uh oh!

elasticsearchmachine commented Sep 13, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csoulios commented Sep 20, 2022

Uh oh!

csoulios commented Sep 20, 2022

Uh oh!

csoulios Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Sep 20, 2022

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csoulios commented Sep 20, 2022

Uh oh!

csoulios commented Sep 20, 2022

Uh oh!

csoulios commented Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

csoulios Sep 20, 2022 •

edited

Loading