Skip to content

Rename Redis input read_timestamp to event.created#9924

Merged
ruflin merged 3 commits intoelastic:masterfrom
ruflin:rename-redis-timestamp
Jan 15, 2019
Merged

Rename Redis input read_timestamp to event.created#9924
ruflin merged 3 commits intoelastic:masterfrom
ruflin:rename-redis-timestamp

Conversation

@ruflin
Copy link
Copy Markdown
Contributor

@ruflin ruflin commented Jan 7, 2019

To follow ECS, read_timestamp was renamed to event.created for the Redis slowlog input.

@ruflin ruflin requested a review from a team as a code owner January 7, 2019 14:03
@ruflin ruflin force-pushed the rename-redis-timestamp branch from 60d28ef to 4f935a0 Compare January 7, 2019 14:18
@ruflin ruflin self-assigned this Jan 7, 2019
@ruflin ruflin added the Team:Integrations Label for the Integrations team label Jan 7, 2019
@ruflin ruflin force-pushed the rename-redis-timestamp branch from 4f935a0 to 4d72fca Compare January 7, 2019 15:11
Copy link
Copy Markdown
Contributor

@webmat webmat Jan 7, 2019

Choose a reason for hiding this comment

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

Why alias false? I think we use read_timestamp in a few places. That could be a 1:1 mapping that catches most cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For now it's false as there are still a few modules which use read_timestamp as a field. As soon as we completed the migration, we can probably make it an alias. Will add a checkbox to our migration issue.

@ruflin ruflin mentioned this pull request Jan 7, 2019
Copy link
Copy Markdown
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

I'm good with this, although I'd like to know why you're going with alias: false.

Note that I added a checkbox in #8655 to address all of the Filebeat modules that save the original @timestamp to read_timestamp prior to parsing the event.

I've also added an entry to elastic/ecs#181 to encapsulate this pattern in a small pipeline

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we don't need common.Time anymore.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You mean just using time.Now().UTC() instead? Did something change recently that we don't need it anymore? We use common.Time still quite a lot in the code base.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we used to use common.Time for JSON formatting only. But with the introduction of go-structform for JSON encoding (in 6.0) we started to encode time.Time and common.Time the very same way in all our outputs.
These are the timestamp encoders for time.Time and common.Time used in all outputs: https://github.com/elastic/beats/blob/master/libbeat/outputs/codec/common.go#L28

The encoders always call UTC() + format string: yyyy-MM-dd'T'HH:mm:ss.SSS'Z'

We don't care which type you use and often check for both time types. common.Time still exists for legacy reasons (we should start to remove it at some point :) ).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the details. +1 on removing all the current usage.

I removed common.Time and UTC conversion in the most recent commit.

To follow ECS, read_timestamp was renamed to `event.created` for the Redis slowlog input.
@ruflin ruflin force-pushed the rename-redis-timestamp branch from 4d72fca to aa4c29d Compare January 8, 2019 08:58
@ruflin ruflin merged commit 57d44ca into elastic:master Jan 15, 2019
@ruflin ruflin deleted the rename-redis-timestamp branch January 15, 2019 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ecs Filebeat Filebeat review Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants