TSDB: Add time series information to field caps by imotov · Pull Request #78790 · elastic/elasticsearch

imotov · 2021-10-06T20:43:03Z

Exposes information about dimensions and metrics via field caps. This
information will be needed for different query languages support.

Relates to #74660

Exposes information about dimensions and metrics via field caps. This information will be needed for PromQL support. Relates to elastic#74660

elasticmachine · 2021-10-06T23:23:56Z

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

elasticmachine · 2021-10-06T23:24:20Z

Pinging @elastic/es-search (Team:Search)

nik9000 · 2021-10-07T15:13:41Z

docs/reference/search/field-caps.asciidoc

        "searchable": false,
-        "aggregatable": false
+        "aggregatable": false,
+        "time_series_dimension": false


Do we want these to appear if the field isn't a dimension? I know lots of folks have thousands of fields and these responses can get quick large. Maybe we should leave it out if it is false.

That's a good idea.

nik9000 · 2021-10-07T15:17:50Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), INDICES_FIELD); // 6
+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_SEARCHABLE_INDICES_FIELD); // 7
+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_AGGREGATABLE_INDICES_FIELD); // 8
+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_DIMENSION_INDICES_FIELD); // 9


Maybe move this to your reflection builder. This is a bit scary.

If you mean InstantiatingObjectParser we cannot use it here since the contractor needs context as the first argument. I can take a look at adding this functionality to the InstantiatingObjectParser in a separate PR.

ah. Makes sense.

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

nik9000 · 2021-10-07T15:28:06Z

...re/src/test/java/org/elasticsearch/xpack/core/ml/dataframe/analyses/ClassificationTests.java


    private static FieldCapabilities createFieldCapabilities(String field, String type) {
-        return new FieldCapabilities(field, type, false, true, true, null, null, null, Collections.emptyMap());
+        return new FieldCapabilities(field, type, false, true, true, false, null, null, null, null, null, Collections.emptyMap());


Maybe it'd be helpful to make a

public static final FieldCapabilities simple(String name, String type, boolean searchable, boolean aggregable) { return new FieldCapabilities(field, type, false, searchable, aggregable, false, null, null, null, null, null, Collections.emptyMap()); }

on FieldCapabilities or some test helper. It looks like a super common pattern.

Or a second ctor. I'm not usually a big fan of extra ctors for tests, but there are so many args here and most code seems to pass exactly the same arguments.

…s-support

…sts for it

nik9000 · 2021-10-13T15:56:52Z

docs/reference/search/field-caps.asciidoc

+  indices have the same definition for the field.
+
+`metric_conflicts_indices`::
+  The list of indices where this field is present but don't have the same metrics type.


Maybe s/metrics type/time_series_metric/

nik9000 · 2021-10-13T15:57:05Z

docs/reference/search/field-caps.asciidoc

+`time_series_dimension`::
+  Whether this field is used as a time series dimension.
+
+`time_series_metrics`::


Is it time_series_metric - no trailing s?

nik9000 · 2021-10-13T15:58:59Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), INDICES_FIELD); // 6
+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_SEARCHABLE_INDICES_FIELD); // 7
+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_AGGREGATABLE_INDICES_FIELD); // 8
+        PARSER.declareStringArray(ConstructingObjectParser.optionalConstructorArg(), NON_DIMENSION_INDICES_FIELD); // 9


ah. Makes sense.

nik9000 · 2021-10-13T16:01:15Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

            this.isAggregatable &= agg;
            this.isMetadataField |= isMetadataField;
+            this.isDimension &= isDimension;
+            // If we have discrepancy in metric types - we ignore it


s/ignore it/treat the fields as non-metrics and return metricConflictIndices/`?

nik9000 · 2021-10-13T16:03:13Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

+            if (isDimension == false && indiceList.stream().anyMatch((caps) -> caps.isDimension)) {
+                // Collect all indices that disagree on the dimension flag
+                nonDimensionIndices = indiceList.stream()
+                    .filter((caps) -> caps.isDimension == false)


It looks like it collects all indices that agree.

If at least one dimension was "false" the end result will be false. However, if at least one dimension was true, we need to report all indices where it was marked as false (the trouble makers) as a non-dimensional index. I think this is consistent with non aggregatable and non searchable indices? Or did I miss your point?

I guess maybe just change the comment to // report the trouble makers or something.

How about now? 155e64f#diff-e86a5a0ec29b846d9e460bcd19923f3ddfc06ec911fbba471fdda9c616d9dca9R528

👍 Thanks. I'm being picky about language. It makes me feel better.

nik9000 · 2021-10-13T16:03:52Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

+            if (indiceList.stream().anyMatch((caps) -> caps.metricType != metricType)) {
+                // Collect all indices that disagree on the dimension flag
+                metricConflictsIndices = indiceList.stream()
+                    .map(caps -> caps.name)


This is just all indices, right?

I suppose if one is in conflict then they all are, but we do set the metric type to null so I think maybe this should filter out the indices that don't consider it a field? I dunno.

Either way the comment above it inaccurate

Copy/paste error

nik9000 · 2021-10-13T16:09:51Z

server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilities.java

            this.isSearchable &= search;
            this.isAggregatable &= agg;
            this.isMetadataField |= isMetadataField;
+            this.isDimension &= isDimension;


Having a non_dimension_indices field makes me think maybe this should be |=.

Or maybe we should flip the logic in non_dimension_indices. I guess we want to degrade to non-tsdb mode when we get a combination.

I think if a field was introduced as non dimension in early indices and then got converted to a dimension in a new index we are in trouble here. To me, this is a fatal error and the main use for this index list will be to generate an appropriate error message to the user.

👍. It feels weird to have non_dimension_indices when the top level is false. Maybe I just need to get used to it.

nik9000

LGTM. I left a some comments about the docs - I think they still mention null but now the fields are absent instead. Also, I wanted to rewrite the description of non_dimension_indices because I still haven't grown comfortable describing it. I understand why its the right way to return the data. I just can't describe why properly yet.

nik9000 · 2021-10-13T17:58:00Z

docs/reference/search/field-caps.asciidoc

+  Whether this field is used as a time series dimension.
+
+`time_series_metric`::
+  Contains metric type if this fields is used as a time series metrics, null if the field is not used as metric.


It is absent if the field is not a metric.

nik9000 · 2021-10-13T17:59:56Z

docs/reference/search/field-caps.asciidoc


+`non_dimension_indices`::
+  The list of indices where this field is not marked as a dimension field, or null if all
+  indices have the same definition for the field.


Maybe a more obvious way of saying this could be "If this is present in response then some indices have the field marked as a dimension and other indices, the ones in this list, do not."

…s-support

This is a followup for elastic#78790, which allows us to replace ConstructingObjectParser with InstantiatingObjectParser which makes keeping track of the positional arguments somewhat easier.

…79206) This is a followup for #78790, which allows us to replace ConstructingObjectParser with InstantiatingObjectParser which makes keeping track of the positional arguments somewhat easier.

To assist the user in configuring the visualizations correctly while leveraging TSDB functionality, information about TSDB configuration should be exposed via the field caps API per field. Especially for metrics fields, it must be clear which fields are metrics and if they belong to only time-series indexes or mixed time-series and non-time-series indexes. To further distinguish metric fields when they belong to any of the following indices: - Standard (non-time-series) indexes - Time series indexes - Downsampled time series indexes This PR modifies the field caps API so that the mapping parameters time_series_dimension and time_series_dimension are presented only when they are set on fields of time-series indexes. Those parameters are completely ignored when they are set on standard (non-time-series) indexes. This PR revisits some of the conventions adopted by elastic#78790

Add time series information to field caps

58bc365

Exposes information about dimensions and metrics via field caps. This information will be needed for PromQL support. Relates to elastic#74660

imotov added >enhancement v8.0.0 :StorageEngine/TSDB You know, for Metrics labels Oct 6, 2021

imotov mentioned this pull request Oct 6, 2021

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

Closed

Fix tests

e0da632

imotov requested a review from csoulios October 6, 2021 23:23

imotov marked this pull request as ready for review October 6, 2021 23:23

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 6, 2021

imotov added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Oct 6, 2021

elasticmachine added the Team:Search Meta label for search team label Oct 6, 2021

nik9000 reviewed Oct 7, 2021

View reviewed changes

imotov added 4 commits October 11, 2021 09:42

Address review comments

3a3b535

Merge remote-tracking branch 'elastic/master' into tsdb-add-field-cap…

66ed887

…s-support

Fix javadoc

9ef464a

Fix serialization

ffdb382

imotov requested a review from nik9000 October 11, 2021 23:12

nik9000 approved these changes Oct 12, 2021

View reviewed changes

Fix mertric_conflicts_indices generation after refactoring and add te…

91afda5

…sts for it

nik9000 reviewed Oct 13, 2021

View reviewed changes

Address the second round of review comments

155e64f

nik9000 approved these changes Oct 13, 2021

View reviewed changes

imotov added 2 commits October 13, 2021 08:08

Improve docs

22abdeb

Merge remote-tracking branch 'elastic/master' into tsdb-add-field-cap…

e788a01

…s-support

imotov merged commit f6034e6 into elastic:master Oct 13, 2021

imotov mentioned this pull request Oct 14, 2021

Enable InstantiatingObjectParser to pass context as a first argument #79206

Merged

imotov deleted the tsdb-add-field-caps-support branch October 15, 2021 20:56

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

wchaparro assigned imotov and unassigned imotov Dec 16, 2021

csoulios mentioned this pull request Jul 21, 2022

[TSDB] Metric fields in the field caps API #88695

Merged

Conversation

imotov commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 6, 2021

Uh oh!

elasticmachine commented Oct 6, 2021

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!

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!

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!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imotov commented Oct 6, 2021 •

edited

Loading