Skip to content
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

Remove event.timezone from events from some json logs #13918

Merged

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Oct 4, 2019

Filebeat modules for Elasticsearch and Logstash support two different
log formats, the JSON one contains timezones, so it doesn't need the
event.timezone added by add_locale for date parsing. Also having
this added event.timezone can be inconsistent in some cases as it
may be different to the timezone of the logs parsed. Don't add this
field when the log message is in JSON format.

More context about this in #13874 (review)

Part of #13877

Filebeat modules for Elasticsearch and Logstash support two different
log formats, the JSON one contains timezones, so it doesn't need the
`event.timezone` added by `add_locale` for date parsing. Also having
this added `event.timezone` can be inconsistent in some cases as it
may be different to the timezone of the logs parsed. Don't add this
field when the log message is in JSON format.
@jsoriano jsoriano requested a review from ycombinator October 4, 2019 12:16
@jsoriano jsoriano requested a review from a team as a code owner October 4, 2019 12:16
@jsoriano jsoriano self-assigned this Oct 4, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring (Stack monitoring)

@@ -6,4 +6,5 @@ paths:
exclude_files: [".gz$"]

processors:
- add_locale: ~
# Locale for timezone is only needed in non-json logs
- add_locale.when.not.regexp.message: "^{"
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we discussed (on Zoom) about leaving this as-is but modifying the JSON processing ingest pipelines to remove event.timezone? That way we wouldn't be duplicating the regexp check in multiple places.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but while doing the change I thought that we would be removing event.timezone even if a user adds it for any other reason, what would be counter-intuitive and probably hard to debug (though probably this is a quite corner case).

If we avoid executing add_locale when not needed as is done here, we still keep the possibility of using add_locale or manually adding this field if a user wants it.
Even if we have the regexp in two places, any change there will make tests with example logs fail, and I don't expect to change these patterns so much.

I'd slightly prefer to don't run add_locale if not needed, but I don't have a strong opinion, both options look fine to me, so as you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I prefer the option of not having to unnecessarily run this processor if we don't need to either. I just wish there was a way not to duplicate the same check in multiple places, but I don't see a way around that given that some processing happens on the beats side and some on the ES side.

Let's go with what you have here.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jsoriano!

@jsoriano
Copy link
Member Author

jsoriano commented Oct 4, 2019

Thanks @ycombinator for your feedback!

@jsoriano jsoriano merged commit 1af0403 into elastic:master Oct 4, 2019
@jsoriano jsoriano deleted the remove-event-timezone-json-stack-logs branch October 4, 2019 17:51
@urso urso added the v7.5.0 label Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants