-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename Redis input read_timestamp to event.created #9924
Conversation
60d28ef
to
4f935a0
Compare
4f935a0
to
4d72fca
Compare
|
||
- from: read_timestamp | ||
to: event.created | ||
alias: false |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
filebeat/input/redis/harvester.go
Outdated
@@ -155,7 +155,9 @@ func (h *Harvester) Run() error { | |||
"redis": common.MapStr{ | |||
"slowlog": subEvent, | |||
}, | |||
"read_timestamp": common.Time(time.Now().UTC()), | |||
"event": common.MapStr{ | |||
"created": common.Time(time.Now().UTC()), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :) ).
There was a problem hiding this comment.
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.
4d72fca
to
aa4c29d
Compare
To follow ECS, read_timestamp was renamed to
event.created
for the Redis slowlog input.