Skip to content

Default to @timestamp in composable template datastream definition#59317

Merged
andreidan merged 29 commits intoelastic:masterfrom
andreidan:default-ds-timestamp-definition
Jul 14, 2020
Merged

Default to @timestamp in composable template datastream definition#59317
andreidan merged 29 commits intoelastic:masterfrom
andreidan:default-ds-timestamp-definition

Conversation

@andreidan
Copy link
Contributor

@andreidan andreidan commented Jul 9, 2020

This removes the data_stream timestamp field specification when
defining a composable template.
The timestamp field is locked to @timestamp for which we provide a default
mapping of type date with default options (added with #59244 ), but the user
can still provide a custom mapping in the template.

The data_stream definition in the composable template is now an empty object:

data_stream: {}

Relates to #53100

This makes the data_stream timestamp field specification optional when
defining a composable template.
When there isn't one specified it will default to `@timestamp`.
@andreidan andreidan added >docs General docs changes v8.0.0 :StorageEngine/Data streams Data streams and their lifecycles v7.9.0 labels Jul 9, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Jul 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs (>docs)

@elasticmachine elasticmachine added the Team:Docs Meta label for docs team label Jul 9, 2020
Copy link
Contributor

@jrodewig jrodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs LGTM.

Since @timestamp will soon be both the required and default timestamp field, I don't see much point in telling users to supply a non-empty data_stream object body, even optionally. Let me know if there is some context I'm missing tho. Thanks!

Comment on lines 508 to 510
(Optional, string) The name of the timestamp field. This field must be present
in all documents indexed into the data stream and must be of type
<<date, `date`>> or <<date_nanos, `date_nanos`>>.
<<date, `date`>> or <<date_nanos, `date_nanos`>>. Defaults to `@timestamp`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the last sentence to:

Must be @timestamp, which is the default.

@dakrone
Copy link
Member

dakrone commented Jul 9, 2020

I don't see much point in telling users to supply a data_stream object body, even optionally. Let me know if there is some context I'm missing tho. Thanks!

@jrodewig if a user doesn't supply the empty data_stream: {} object, the template will create regular indices, instead of creating a data stream when documents are indexed that are matched

@jrodewig
Copy link
Contributor

jrodewig commented Jul 9, 2020

@dakrone Sorry for the confusion. I wasn't clear in my original comment. I meant that we should just tell users to supply:

  "data_stream": { }

Instead of:

  "data_stream": {
    "timestamp_field": "@timestamp"
  }

The defs produce the same results now so there is no point of even telling them about the timestamp_field param, even as an option.

@andreidan andreidan requested a review from martijnvg July 9, 2020 17:17
@andreidan
Copy link
Contributor Author

The defs produce the same results now so there is no point of even telling them about the timestamp_field param, even as an option.

@jrodewig I agree it's rather redundant. However I believe the plan is to eventually allow a custom timestamp field configuration.

Happy to keep it simple for now and not mention it if that's your preference though.

@martijnvg what do you think?

andreidan and others added 2 commits July 9, 2020 18:59
Co-authored-by: James Rodewig <james.rodewig@elastic.co>
@andreidan
Copy link
Contributor Author

@elasticmachine update branch

@jtibshirani
Copy link
Contributor

I meant that we should just tell users to supply: "data_stream": { }

+1, and I think we shouldn't even allow the timestamp_field to be specified in the API. We decided to postpone support for a custom timestamp field name, and we don't yet know exactly how it will look. For example, one option is that instead of specifying timestamp_field, users just specify @timestamp as a field alias to the concrete timestamp field.

@andreidan
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left a question


static {
PARSER.declareString(ConstructingObjectParser.constructorArg(), DataStream.TIMESTAMP_FIELD_FIELD);
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), DataStream.TIMESTAMP_FIELD_FIELD);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe remove this field from the xcontent format? So that the only valid format is an empty json object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point. i'll drop the field completely in the POJO but keep the data_stream empty JSON object

@andreidan andreidan merged commit 5609353 into elastic:master Jul 14, 2020
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Jul 14, 2020
…lastic#59317)

This makes the data_stream timestamp field specification optional when
defining a composable template.
When there isn't one specified it will default to `@timestamp`.

(cherry picked from commit 5609353)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit to andreidan/elasticsearch that referenced this pull request Jul 14, 2020
andreidan added a commit that referenced this pull request Jul 14, 2020
…59317) (#59516)

This makes the data_stream timestamp field specification optional when
defining a composable template.
When there isn't one specified it will default to `@timestamp`.

(cherry picked from commit 5609353)
Signed-off-by: Andrei Dan <andrei.dan@elastic.co>
andreidan added a commit to danhermann/elasticsearch that referenced this pull request Jul 14, 2020
@andreidan andreidan added the Team:Deployment Management Meta label for Management Experience - Deployment Management team label Jul 14, 2020
@cjcenizal
Copy link
Contributor

@andreidan The PR description states that this PR "makes the data_stream timestamp field specification optional", but it looks like it actually removes it completely. Could you please clarify or update the PR description if it's incorrect?

jrodewig added a commit that referenced this pull request Jul 14, 2020
Removes the `@timestamp` field mapping from several data stream index
template snippets.

With #59317, the `@timestamp` field defaults to a `date` field data type
for data streams.
@andreidan
Copy link
Contributor Author

@cjcenizal thanks for pointing it out. changed the description to reflect the actual/final behaviour.

danhermann pushed a commit that referenced this pull request Jul 14, 2020
tylersmalley added a commit to tylersmalley/kibana that referenced this pull request Jul 14, 2020
elastic#71670

Caused by elastic/elasticsearch#59317

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
jrodewig added a commit that referenced this pull request Jul 14, 2020
…9553)

Removes the `@timestamp` field mapping from several data stream index
template snippets.

With #59317, the `@timestamp` field defaults to a `date` field data type
for data streams.
elasticmachine pushed a commit to mistic/kibana that referenced this pull request Jul 15, 2020
elastic#71670

Caused by elastic/elasticsearch#59317

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
tylersmalley added a commit to tylersmalley/kibana that referenced this pull request Jul 15, 2020
elastic#71670

Caused by elastic/elasticsearch#59317

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
tylersmalley added a commit to tylersmalley/kibana that referenced this pull request Jul 15, 2020
elastic#71670

Caused by elastic/elasticsearch#59317

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
tylersmalley added a commit to elastic/kibana that referenced this pull request Jul 15, 2020
#71670

Caused by elastic/elasticsearch#59317

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
tylersmalley added a commit to elastic/kibana that referenced this pull request Jul 15, 2020
#71670

Caused by elastic/elasticsearch#59317

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>docs General docs changes :StorageEngine/Data streams Data streams and their lifecycles Team:Data Management (obsolete) DO NOT USE. This team no longer exists. Team:Deployment Management Meta label for Management Experience - Deployment Management team Team:Docs Meta label for docs team v7.9.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants