TSDB: Add `_tsid` field to time_series indices by csoulios · Pull Request #80276 · elastic/elasticsearch

csoulios · 2021-11-03T13:19:16Z

This PR adds support for a field named _tsid that uniquely identifies the time series a document belongs to.

When a document is indexed in a time series index (IndexMode.TIME_SERIES), _tsid field is generated from the values of all dimension fields.

elasticmachine · 2021-11-03T13:19:20Z

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

nik9000

We talked on video a bit but I had already started typing stuff. This makes public what we talked about.

server/src/main/java/org/elasticsearch/index/IndexMode.java

server/src/main/java/org/elasticsearch/index/IndexSettings.java

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

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

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

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

weizijun · 2021-11-08T09:29:43Z

Will index sorting settings about dealing _tsid added in this PR? Or create another PR do the feature?

csoulios · 2021-11-08T14:11:28Z

@weizijun this PR is work in progress. I need to add more tests and complete the index sorting part before it is ready to be merged.

so that they are added explicitly in the field mapper

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

to IndexMode so that it is only created in time_series mode

nik9000

I realized a thing about encoding these even if we're not using them. I think it's rare that a field will be a dimension outside of time series mode but it's possible. We should probably come up with a scheme to skip those fields....

qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java

nik9000 · 2021-11-23T20:44:51Z

server/src/main/java/org/elasticsearch/index/IndexSortConfig.java

+                new FieldSortSpec(TimeSeriesIdFieldMapper.NAME),
+                new FieldSortSpec(DataStreamTimestampFieldMapper.DEFAULT_PATH) };
+            return;
+        }


This could probably use the same "move it into a method on IndexMode" treatment that the builder used. But that can wait for a follow up I think.

nik9000 · 2021-11-23T20:46:57Z

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

+        if (dimension) {
+            // Extract the tsid part of the dimension field
+            BytesReference bytes = TimeSeriesIdFieldMapper.extractTsidValue(NetworkAddress.format(address));
+            context.doc().addDimensionBytes(fieldType().name(), bytes);


Now that I think about.... Can't you declare something a dimension without enabling the tsid field? I wonder if we should avoid encoding it if the field isn't present.

For ip and numeric field types, we can skip the encodeTsidValue part if _tsid field is not going to be generated.

However, for keyword fields we still must do the encoding, because must validate the string length.

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

nik9000 · 2021-11-25T15:55:32Z

Good call!

…

On Thu, Nov 25, 2021, 10:21 AM Christos Soulios ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java <#80276 (comment)> : > @@ -497,18 +499,14 @@ private static InetAddress value(XContentParser parser, InetAddress nullValue) t } private void indexValue(DocumentParserContext context, InetAddress address) { + if (dimension) { + // Extract the tsid part of the dimension field + BytesReference bytes = TimeSeriesIdFieldMapper.extractTsidValue(NetworkAddress.format(address)); + context.doc().addDimensionBytes(fieldType().name(), bytes); For ip and numeric field types, we can skip the encodeTsidValue part if _tsid field is not going to be generated. However, for keyword fields we still must do the encoding, because must validate the string length. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#80276 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUXIUOG6MA3K3XFT6H7P3UNZIANANCNFSM5HI45LVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

csoulios · 2021-11-25T18:52:21Z

@elasticmachine run elasticsearch-ci/eql-correctness

csoulios · 2021-11-29T09:50:51Z

@elasticmachine run elasticsearch-ci/part-2

This is a follow up to #80276 Relates to #74660 Co-authored-by: Nik Everett <nik9000@gmail.com>

This PR builds on the work added in #80276 that generates the _tsid field for keyword, ip and number dimension fields. It adds support for unsigned_long dimension fields.

Since _tsid cannot be a multi-value field, this PR modifies the TimeSeriesIdFieldMapper so that _tsid is added as a SortedDocValuesField (instead of a SortedSetDocValuesField) Relates to #80276

csoulios added 2 commits November 3, 2021 02:48

Initial commit

2119b6d

Checkstyle

9461331

csoulios added :StorageEngine/TSDB You know, for Metrics v8.1.0 labels Nov 3, 2021

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

csoulios marked this pull request as draft November 3, 2021 13:19

Merge branch 'master' into tsid-gen

2ab1ff5

nik9000 mentioned this pull request Nov 3, 2021

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

Closed

nik9000 reviewed Nov 3, 2021

View reviewed changes

csoulios added 8 commits November 3, 2021 19:39

Removed TimeSeriesIdGeneratorService class

fff4d7e

Cleanup

ca6c86e

Merge branch 'master' into tsid-gen

12d89cf

Revert

681831f

Remove check for dynamic mapping update

8267f74

Return a shared instance of the tsid field mapper

7fbedc8

Remove tsid generator from Mapping class

1b5ef49

Remove TimeSeriesIdGenerator class

95278e4

Merge branch 'master' into tsid-gen

505d42b

csoulios added 9 commits November 9, 2021 22:55

Merge branch 'master' into tsid-gen

3ecaa8e

Delegate tsid generation to TsidFieldMapper

e62254d

Simplified dimension fields

2975b48

so that they are added explicitly in the field mapper

More cleanup

7587518

More cleanup

b6f3f94

Cleanup exception handling when parsing _tsid

68e1526

Fix broken tests

a125aab

Merge branch 'master' into tsid-gen

07ed667

Added more tests

bc43ba0

Merge branch 'master' into tsid-gen

c1cf213

romseygeek reviewed Nov 23, 2021

View reviewed changes

server/src/main/java/org/elasticsearch/index/mapper/TimeSeriesIdFieldMapper.java Outdated Show resolved Hide resolved

csoulios added 2 commits November 23, 2021 20:48

Fix conflicts

6a8dca6

Delegated building of tsid field mapper creation

5106a6e

to IndexMode so that it is only created in time_series mode

csoulios requested review from nik9000 and romseygeek November 23, 2021 19:06

csoulios added 2 commits November 23, 2021 21:08

Fix brojen test

0e10639

Merge branch 'master' into tsid-gen

6d7d4ae

nik9000 approved these changes Nov 23, 2021

View reviewed changes

csoulios added 2 commits November 23, 2021 22:55

Fix broken tests

91c91db

Addressed reviewer comments

709700b

Merge branch 'master' into tsid-gen

1453e63

Merge branch 'master' into tsid-gen

12e5105

csoulios merged commit c0b4b60 into elastic:master Nov 29, 2021

csoulios deleted the tsid-gen branch November 29, 2021 10:44

This was referenced Dec 2, 2021

TSDB: Add unsigned_long dimension fields to _tsid #81284

Merged

TSDB: Add security tests for _tsid #81382

Merged

csoulios added a commit that referenced this pull request Dec 9, 2021

TSDB: Add security tests for _tsid (#81382)

8fc9ba7

This is a follow up to #80276 Relates to #74660 Co-authored-by: Nik Everett <nik9000@gmail.com>

wchaparro assigned csoulios and unassigned csoulios Dec 16, 2021

csoulios mentioned this pull request Jan 25, 2022

TSDB: Change _tsid field to SortedDocValuesField #83045

Merged

pugnascotia added the >enhancement label Jan 31, 2022

Conversation

csoulios commented Nov 3, 2021

Uh oh!

elasticmachine commented Nov 3, 2021

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weizijun commented Nov 8, 2021

Uh oh!

csoulios commented Nov 8, 2021

Uh oh!

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nik9000 Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

csoulios Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nik9000 commented Nov 25, 2021 via email

Uh oh!

csoulios commented Nov 25, 2021

Uh oh!

csoulios commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants