Fix the timestamp field of a data stream to @timestamp #59076

martijnvg · 2020-07-06T14:15:04Z

The commit makes the following changes:

The timestamp field of a data stream definition in a composable
index template can only be set to @timestamp.
Removed custom data stream timestamp field validation and reuse the validation from TimestampFieldMapper and
instead only check that the _timestamp field mapping has been defined on a backing index of a data stream.
Moved code that injects _timestamp meta field mapping from MetadataCreateIndexService#applyCreateIndexRequestWithV2Template(...) method
to MetadataIndexTemplateService#collectMappings(...) method.
Fixed a bug (Component templates are considered invalid without timestamp_field #58956) that cases timestamp field validation to be performed
for each template and instead of the final mappings that is created.

Relates to #58642
Relates to #53100
Closes #58956
Closes #58583

@timestamp

The commit makes the following changes: * The timestamp field of a data stream definition in a composable index template can only be set to '@timestamp'. * Removed custom data stream timestamp field validation and reuse the validation from `TimestampFieldMapper` and instead only check that the _timestamp field mapping has been defined on a backing index of a data stream. * Moved code that injects _timestamp meta field mapping from `MetadataCreateIndexService#applyCreateIndexRequestWithV2Template58956(...)` method to `MetadataIndexTemplateService#collectMappings(...)` method. * Fixed a bug (elastic#58956) that cases timestamp field validation to be performed for each template and instead of the final mappings that is created. Relates to elastic#58642 Relates to elastic#53100 Closes elastic#58956 Closes elastic#58583

martijnvg · 2020-07-06T15:26:38Z

@elasticmachine run elasticsearch-ci/2

elasticmachine · 2020-07-06T15:27:17Z

Pinging @elastic/es-core-features (:Core/Features/Data streams)

dakrone

LGTM, thanks Martijn! I left some really minor comments

dakrone · 2020-07-06T16:12:54Z

server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java

@@ -256,6 +257,10 @@ public String toString() {
        private final String timestampField;

        public DataStreamTemplate(String timestampField) {
+            if ("@timestamp".equals(timestampField) == false) {


Can we make this a public static string so it can be referenced elsewhere?

dakrone · 2020-07-06T16:15:21Z

server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java

+         * @return a mapping snippet for a backing index with `_timestamp` meta field mapper properly configured.
+         */
+        public Map<String, Object> getDataSteamMappingSnippet() {
+            return Map.of("_doc", Map.of(TimestampFieldMapper.NAME, Map.of("path", timestampField)));


Super minor:

Suggested change

return Map.of("_doc", Map.of(TimestampFieldMapper.NAME, Map.of("path", timestampField)));

return Map.of(MapperService.SINGLE_MAPPING_NAME, Map.of(TimestampFieldMapper.NAME, Map.of("path", timestampField)));

dakrone · 2020-07-06T16:19:00Z

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java


+        public TimestampField(String name) {
+            assert "@timestamp".equals(name) : "unexpected timestamp field [" + name + "]";


I think we should transform this into an actual error, that way if we end up using it for the HLRC it doesn't fail to throw because assertions are disabled

I think HLRC uses its own DataStream pojo. I will change this from an assertion to an actual error.

dakrone · 2020-07-06T16:20:04Z

server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java


        @SuppressWarnings("unchecked")
        private static final ConstructingObjectParser<TimestampField, Void> PARSER = new ConstructingObjectParser<>(
            "timestamp_field",
-            args -> new TimestampField((String) args[0], (Map<String, Object>) args[1])
+            args -> new TimestampField((String) args[0])


This makes me wonder, this is fine as-is, but should we even bother having this class or specifying the name of the field if it's going to be hardcoded for the foreseeable future?

I prefer to keep this class. I think that that the timestamp field will not stay hard coded for a long time.

…a stream or data stream rollover, this fixes a docs test, where a regular index creation matches (logs-*) with a template with a data stream definition.

…@timestamp

…king index

martijnvg · 2020-07-07T12:32:49Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java

@@ -902,6 +909,23 @@ public static String findV2Template(Metadata metadata, String indexName, boolean
        Optional.ofNullable(template.template())
            .map(Template::mappings)
            .ifPresent(mappings::add);
+
+        // Only include _timestamp mapping snippet if creating backing index.
+        if (indexName.startsWith(DataStream.BACKING_INDEX_PREFIX)) {


@dakrone I had to add this if statement otherwise the meta field mapper was going to be applied on each create index api call. In a docs test, the new logs-- composable index template was triggered by a regular create index api call and then the test failed, because the document being indexed had no timestamp field.

I think only applying the meta field automatically makes sense for backing indices only and not when a user creates a new index via create index api and the index composable template matches?

See commits: a23d2e4, which then was superseded by: c787332

Note currently in master, the _timestamp field gets only applied if the create index request originates from create data stream api and data stream rollover (signalled via CreateIndexClusterStateUpdateRequest#dataStreamName field), but now this logic moved MetadataIndexTemplateService.collectMappings(...) method, which didn't have that context.

Would it be cleaner to pull this check out of collectMappings and into the caller? That way we could avoid the workaround here: https://github.com/elastic/elasticsearch/pull/59076/files#diff-115cd2f0fff9dd97acc95e1d29012a15R1075.

…and keep using current timestamp field

…@timestamp

(a change got in after I merged in master into this pr, but didn't notice it until I merged this pr)

@timestamp

Backport elastic#59076 of to 7.x branch. The commit makes the following changes: * The timestamp field of a data stream definition in a composable index template can only be set to '@timestamp'. * Removed custom data stream timestamp field validation and reuse the validation from `TimestampFieldMapper` and instead only check that the _timestamp field mapping has been defined on a backing index of a data stream. * Moved code that injects _timestamp meta field mapping from `MetadataCreateIndexService#applyCreateIndexRequestWithV2Template58956(...)` method to `MetadataIndexTemplateService#collectMappings(...)` method. * Fixed a bug (elastic#58956) that cases timestamp field validation to be performed for each template and instead of the final mappings that is created. * only apply _timestamp meta field if index is created as part of a data stream or data stream rollover, this fixes a docs test, where a regular index creation matches (logs-*) with a template with a data stream definition. Relates to elastic#58642 Relates to elastic#53100 Closes elastic#58956 Closes elastic#58583

(a change got in after I merged in master into this pr, but didn't notice it until I merged this pr)

@timestamp

Backport of #59076 to 7.x branch. The commit makes the following changes: * The timestamp field of a data stream definition in a composable index template can only be set to '@timestamp'. * Removed custom data stream timestamp field validation and reuse the validation from `TimestampFieldMapper` and instead only check that the _timestamp field mapping has been defined on a backing index of a data stream. * Moved code that injects _timestamp meta field mapping from `MetadataCreateIndexService#applyCreateIndexRequestWithV2Template58956(...)` method to `MetadataIndexTemplateService#collectMappings(...)` method. * Fixed a bug (#58956) that cases timestamp field validation to be performed for each template and instead of the final mappings that is created. * only apply _timestamp meta field if index is created as part of a data stream or data stream rollover, this fixes a docs test, where a regular index creation matches (logs-*) with a template with a data stream definition. Relates to #58642 Relates to #53100 Closes #58956 Closes #58583

jtibshirani

I left a few late comments!

jtibshirani · 2020-07-09T23:00:12Z

server/src/main/java/org/elasticsearch/cluster/metadata/ComposableIndexTemplate.java

+        /**
+         * @return a mapping snippet for a backing index with `_timestamp` meta field mapper properly configured.
+         */
+        public Map<String, Object> getDataSteamMappingSnippet() {


Noticed this typo as I was trying to pull up the method: DataSteam -> DataStream.

jtibshirani · 2020-07-09T23:05:34Z

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

@@ -196,6 +196,10 @@ public void validate(DocumentFieldMappers lookup) {
        }
    }

+    public String getPath() {


Does TimestampFieldMapper need a path parameter now? We're not sure how we will eventually implement custom timestamp names, I can see an implementation where we don't use path here at all. So maybe we could simplify and just have an enabled boolean flag instead.

Depending on the implementation, we can always add path later and just default to @timestamp if it's not specified.

jtibshirani · 2020-07-09T23:12:56Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamService.java

-        } else {
-            mappingPath = "properties." + fieldPath.replace(".", ".properties.");
+        if (timestampFieldName.equals(fieldMapper.getPath()) == false) {
+            throw new IllegalArgumentException("[_timestamp] meta field doesn't point to data stream timestamp field [" +


I think this error will be hard for end users to understand, since they shouldn't be aware of the _timestamp field. Maybe we won't even need this check, once we remove the ability to set timestamp_field, as discussed in #59317 (comment)?

This is more of a sanity check. Kind of ensuring that we did apply the _data_stream_timestamp meta field mapper. If a user would run into this error then this would be a bug. Can tweak this check.

…led' option and adjusted exception messages. Relates to elastic#59076

…led' option (#59503) and adjusted exception messages. Relates to #59076

…led' option Backport elastic#59503 to 7.x and adjusted exception messages. Relates to elastic#59076

…led' option Backport elastic#59727 to 7.x and adjusted exception messages. Relates to elastic#59076

…led' option (#59727) Backport #59503 to 7.x and adjusted exception messages. Relates to #59076

…led' option (#59728) Backport #59727 to 7.x and adjusted exception messages. Relates to #59076

martijnvg added v8.0.0 :Data Management/Data streams Data streams and their lifecycles v7.9.0 >non-issue labels Jul 6, 2020

fixed tests

257d33a

martijnvg requested review from dakrone, jtibshirani and henningandersen July 6, 2020 15:26

martijnvg marked this pull request as ready for review July 6, 2020 15:27

elasticmachine added the Team:Data Management Meta label for data/management team label Jul 6, 2020

fixed more tests

95e5b2c

dakrone approved these changes Jul 6, 2020

View reviewed changes

martijnvg added 6 commits July 6, 2020 22:56

only apply _timestamp meta field if index is created as part of a dat…

a23d2e4

…a stream or data stream rollover, this fixes a docs test, where a regular index creation matches (logs-*) with a template with a data stream definition.

Merge remote-tracking branch 'es/master' into fix_timestamp_field_to_…

1e6b322

…@timestamp

checkstyle

fa6f0ab

fix ml integ tests

289aa13

Merge remote-tracking branch 'es/master' into fix_timestamp_field_to_…

3bd6764

…@timestamp

only include template with _timestamp field mapping if creation a bac…

c787332

…king index

martijnvg commented Jul 7, 2020

View reviewed changes

martijnvg added 7 commits July 7, 2020 15:10

one search and replace too many

14e8da3

instead of changeing the current timestamp field, just add @timestamp …

1c9eb00

…and keep using current timestamp field

Merge remote-tracking branch 'es/master' into fix_timestamp_field_to_…

c92c3a8

…@timestamp

disable bwc tests

d3160b0

iter

163ae67

iter2

29ce2f4

Merge remote-tracking branch 'es/master' into fix_timestamp_field_to_…

5052d87

…@timestamp

martijnvg merged commit cb6b05d into elastic:master Jul 8, 2020

martijnvg added the backport pending label Jul 8, 2020

martijnvg added a commit that referenced this pull request Jul 8, 2020

fixed compile error after merging in #59076

3515909

(a change got in after I merged in master into this pr, but didn't notice it until I merged this pr)

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Jul 8, 2020

fixed compile error after merging in elastic#59076

a4eca73

(a change got in after I merged in master into this pr, but didn't notice it until I merged this pr)

martijnvg mentioned this pull request Jul 8, 2020

Fix the timestamp field of a data stream to @timestamp #59210

Merged

This was referenced Jul 8, 2020

[DOCS] Document @timestamp as only valid DS timestamp field #59225

Merged

[7.x] [DOCS] Update get data stream API response (#59197) #59221

Merged

jloleysens mentioned this pull request Jul 8, 2020

[Index Management] Fix get data streams api integration elastic/kibana#71067

Merged

martijnvg removed the backport pending label Jul 8, 2020

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Jul 8, 2020

Renable bwc tests after elastic#59076 has been backported

cbbcf36

martijnvg added a commit that referenced this pull request Jul 8, 2020

Renable bwc tests after #59076 has been backported (#59234)

51438c3

martijnvg mentioned this pull request Jul 9, 2020

Make timestamp field mapping part of data stream definition. #58642

Closed

jtibshirani reviewed Jul 9, 2020

View reviewed changes

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Jul 14, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enab…

e26e617

…led' option and adjusted exception messages. Relates to elastic#59076

martijnvg mentioned this pull request Jul 14, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enabled' option #59503

Merged

martijnvg added a commit that referenced this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enab…

261250d

…led' option (#59503) and adjusted exception messages. Relates to #59076

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enab…

61ea37d

…led' option Backport elastic#59503 to 7.x and adjusted exception messages. Relates to elastic#59076

martijnvg mentioned this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enabled' option #59727

Merged

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enab…

1f511a5

…led' option Backport elastic#59727 to 7.x and adjusted exception messages. Relates to elastic#59076

martijnvg mentioned this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enabled' option #59728

Merged

martijnvg added a commit that referenced this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enab…

0096238

…led' option (#59727) Backport #59503 to 7.x and adjusted exception messages. Relates to #59076

martijnvg added a commit that referenced this pull request Jul 16, 2020

Replaced _data_stream_timestamp meta field's 'path' option with 'enab…

268f4f5

…led' option (#59728) Backport #59727 to 7.x and adjusted exception messages. Relates to #59076

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	return Map.of("_doc", Map.of(TimestampFieldMapper.NAME, Map.of("path", timestampField)));
	return Map.of(MapperService.SINGLE_MAPPING_NAME, Map.of(TimestampFieldMapper.NAME, Map.of("path", timestampField)));


		public TimestampField(String name) {
		assert "@timestamp".equals(name) : "unexpected timestamp field [" + name + "]";

Fix the timestamp field of a data stream to @timestamp #59076

Fix the timestamp field of a data stream to @timestamp #59076

Uh oh!

Conversation

martijnvg commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg commented Jul 6, 2020

Uh oh!

elasticmachine commented Jul 6, 2020

Uh oh!

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

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!

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

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martijnvg commented Jul 6, 2020 •

edited

Loading