Implement logic for storing fields that are neither dimensions nor metrics (aka tags) by salvatore-campagna · Pull Request #87929 · elastic/elasticsearch

salvatore-campagna · 2022-06-22T17:37:05Z

We want labels to be collected storing just the latest value
when aggregating documents in a rollup index. This means
labels will behave similarly to counter metrics.

salvatore-campagna · 2022-06-23T08:18:11Z

elasticsearch-ci/part-2 is failing at the moment for a mapping issue involving IPv4 addresses which are handled as keywords. The following line return format.format(values.nextValue()); is failing because the byte array includes non-UTF8 characters like, for instance, '0xFF' which are used to encode an IPv4 and IPv6 address. To be more precise the RawDocValueFormat format is used when formatting the value instead of using the appropriate IpDocValueFormat. As a result this is a mapping mistake. NOTE: if the type of the field is set to keyword instead of ip the test is green.

salvatore-campagna · 2022-07-20T06:59:05Z

@elasticmachine run elasticsearch-ci/part-2

csoulios

Thanks for iterating on this.
I left some more comments.

csoulios · 2022-07-22T12:38:43Z

x-pack/plugin/rollup/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/rollup/10_basic.yml

  - skip:
-      version: " - 8.2.99"
+      version: " - 8.3.99"
      reason: tsdb indexing changed in 8.3.0


reason should be something like "rollup: labels support added in 8.4.0"

csoulios · 2022-07-22T12:47:15Z

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

+            if (validTypes.contains(value.getClass()) == false) {
                throw new IllegalArgumentException(
-                    "Expected [" + VALID_TYPES + "] for field [" + field + "], " + "got [" + value.getClass() + "]"
+                    "Expected [" + VALID_METRIC_TYPES + "] for field [" + field + "], " + "got [" + value.getClass() + "]"


Should VALID_METRIC_TYPES be validTypes?

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

csoulios · 2022-07-22T13:43:47Z

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

+            final Object[] value = fieldValues.apply(docValueCount);
+            if (metricFieldProducers.containsKey(field)) {
+                // TODO: missing support for array metrics
+                collectMetric(field, value[0]);


About the TODO: missing support for array metrics comment here. Is this something you plan to add in this PR or in a following PR?

Not really, because I think that needs to decide first how do we handle that which I don't have any idea at the moment.

I think it should work in the same way as aggregations work on multi-value fields.

csoulios · 2022-07-22T13:58:37Z

...plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/TimeseriesFieldTypeHelper.java

+        final MappedFieldType fieldType = mapperService.mappingLookup().getFieldType(field);
+        return fieldType != null
+            && (timestampField.equals(field) == false)
+            && (fieldType.isAggregatable())


Why must a label field be aggregatable?

This is basically to avoid fields of type text which is not aggregatable...while it would work with fields of type keyword which are aggregatable.

If I remove that condition the tests result in an errors saying something like "you need to use aggregatable fields or use <some API I don't remember> to support non-aggregatable fields

cool thanks for the explanation. I had missed that

This actually means that we cannot have a text field as label?

salvatore-campagna · 2022-07-22T15:22:26Z

@elasticmachine update branch

salvatore-campagna · 2022-07-25T10:35:26Z

@elasticmachine update branch

elasticmachine · 2022-07-25T10:35:28Z

merge conflict between base and head

…labels

csoulios

LGTM!

Thank you for iterating on this

salvatore-campagna · 2022-07-25T14:28:14Z

LGTM!

Thank you for iterating on this

Are we still waiting for something before being able to merge or can I proceed?

csoulios · 2022-07-25T14:53:50Z

Are we still waiting for something before being able to merge or can I proceed?

Please go ahead and merge it.

We have identified 2 TODO tasks that can be implemented in follow-up PRs:

text fields as labels
Computing metric aggs on array fields

salvatore-campagna · 2022-07-25T15:10:07Z

Are we still waiting for something before being able to merge or can I proceed?

Please go ahead and merge it.

We have identified 2 TODO tasks that can be implemented in follow-up PRs:

text fields as labels

Computing metric aggs on array fields

I will create two tasks in #74660 to track them.

salvatore-campagna added 16 commits June 22, 2022 19:02

feature: include support for labels in downsampling

49a093e

fix: add missing parent constructor call

d65790c

fix: add getter for label fields

4e81297

fix: collect last value for labels

6240a36

fix: extract collection logic in Collectable interface

d08c72e

fix: extract common metric and label logic in AbstractFieldProducer

eab6819

comment: clarify comment about metrics vs labels usage

c253102

fix: extract common collect method for labels and metrics

0084baf

fix: introduce a FieldProducer interface

2115d3a

fix: use more descriptive exception message

eb87a60

fix: restore metric names

cbd1d8b

fix: use Sets.union instead of streams and collect

3f586ba

fix: double instead of numeric

6c02521

todo: boolean and other types like integer still do not work

0413373

fix: unit test Label collection

698868b

fix: code format violations

8458d5a

salvatore-campagna requested a review from csoulios June 22, 2022 17:37

salvatore-campagna self-assigned this Jun 22, 2022

elasticsearchmachine added the v8.4.0 label Jun 22, 2022

fix: remove unused interface

f6df2ca

salvatore-campagna marked this pull request as draft June 22, 2022 17:45

salvatore-campagna added :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jun 22, 2022

salvatore-campagna added 2 commits June 23, 2022 08:56

fix: import code format violation

d099317

fix: make 'label' field final

b76b5c7

salvatore-campagna added 4 commits June 23, 2022 12:12

test: add more tests for different types

d9ec390

fix: now lables are rolledup too

6b4b8d8

fix: use the correct DocValueFormat for each field

ee46911

fix: cannot compare a docuble counter to an integer

cab6597

sethmlarson added the Team:Clients Meta label for clients team label Jul 19, 2022

elasticsearchmachine removed the Team:Clients Meta label for clients team label Jul 19, 2022

sethmlarson added the Team:Clients Meta label for clients team label Jul 19, 2022

elasticsearchmachine removed the Team:Clients Meta label for clients team label Jul 19, 2022

sethmlarson added the Team:Clients Meta label for clients team label Jul 20, 2022

elasticsearchmachine removed the Team:Clients Meta label for clients team label Jul 20, 2022

sethmlarson added the Team:Clients Meta label for clients team label Jul 20, 2022

elasticsearchmachine removed the Team:Clients Meta label for clients team label Jul 20, 2022

salvatore-campagna added 2 commits July 20, 2022 09:06

fix: rename field to name

594003f

fix: rename field to name

808758c

salvatore-campagna requested a review from csoulios July 20, 2022 10:15

csoulios reviewed Jul 22, 2022

View reviewed changes

salvatore-campagna added 3 commits July 22, 2022 17:20

fix: clarify skip reason

a673369

fix: use the right variable ion the error message

3972f79

fix: use the log guard to avoid traces

00dae30

Merge branch 'master' into feature/downsampling-labels

aa8d3f3

elasticsearchmachine changed the base branch from master to main July 22, 2022 23:06

salvatore-campagna added 3 commits July 25, 2022 14:37

Merge remote-tracking branch 'origin/main' into feature/downsampling-…

6b1440a

…labels

fix: code format violations

e254486

fix: datetime format

2eec140

csoulios approved these changes Jul 25, 2022

View reviewed changes

salvatore-campagna merged commit 504b30c into elastic:main Jul 25, 2022

abdonpijpelink mentioned this pull request Apr 5, 2023

[DOCS] Add documentation for TSDS label fields #95041

Closed

Conversation

salvatore-campagna commented Jun 22, 2022

Uh oh!

salvatore-campagna commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

salvatore-campagna commented Jul 20, 2022

Uh oh!

csoulios 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!

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!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Jul 22, 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!

salvatore-campagna commented Jul 22, 2022

Uh oh!

salvatore-campagna commented Jul 25, 2022

Uh oh!

elasticmachine commented Jul 25, 2022

Uh oh!

csoulios left a comment

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna commented Jul 25, 2022

Uh oh!

csoulios commented Jul 25, 2022

Uh oh!

salvatore-campagna commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

salvatore-campagna commented Jun 23, 2022 •

edited

Loading

salvatore-campagna Jul 22, 2022 •

edited

Loading