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

[Filebeat] Fix RFC5424 date format in system/syslog in pipeline #12529

Merged
merged 5 commits into from
Jun 14, 2019

Conversation

adriansr
Copy link
Contributor

The date format used by the system/syslog pipeline has a couple of issues with RFC5424 messages:

  • Since Elasticsearch switched from Joda time to Java time, it won't correctly parse timezone offsets that use a colon to separate hours and minutes.

  • RFC5424 allows for 1 to 6 digits for the fractional second. This updates the pipeline to also accept 3 decimals (milliseconds).

The date format used by the system/syslog pipeline has a couple of
issues:

- Since Elasticsearch switched to Java time from Joda time, it won't
  correctly parse timezone offsets that use a colon to separate hours
  and minutes.

- RFC5424 allows for 1 to 6 digits for the fractional second. This
  updates the pipeline to also accept 3 decimals (milliseconds).
@adriansr adriansr requested review from a team as code owners June 13, 2019 14:03
@adriansr adriansr added the needs_backport PR is waiting to be backported to other branches. label Jun 13, 2019
delete_key(obj, "@timestamp")

# Remove timestamp from syslog tests except for the log file that needs it.
if (obj["event.dataset"] == "system.syslog" and filename != "tz-offset.log"):
delete_key(obj, "@timestamp")

Copy link
Contributor

Choose a reason for hiding this comment

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

its a bit hacky but I think for now we don't have choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely hacky :)

Note to reviewers: This is needed because in most syslog logs, the year is missing from the timestamp, so Elasticsearch will set it to the current year and that will cause the tests to fail every new year.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah should the explanation you gave be in the comment in the code? This is hacky enough after a while we probably won't remember why it's added 😄(If there is no better solution.)

Copy link
Contributor Author

@adriansr adriansr Jun 13, 2019

Choose a reason for hiding this comment

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

should the explanation you gave be in the comment in the code?

Right!

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Lets wait for CI

@adriansr
Copy link
Contributor Author

adriansr commented Jun 13, 2019

@ph @kaiyan-sheng
This didn't pass CI because I didn't test it properly (GENERATE=0 doesn't do what I expected it to do).

Turns out that in order for it to work, the hack needs to be a little uglier. Are you still OK with it? (see the latest commits). Otherwise, we need to find out a more elegant way of having tests keep the timestamp selectively.

@adriansr adriansr merged commit d30b4dc into elastic:master Jun 14, 2019
adriansr added a commit to adriansr/beats that referenced this pull request Jun 14, 2019
…stic#12529)

The date format used by the system/syslog pipeline has a couple of
issues:

- Since Elasticsearch switched to Java time from Joda time, it won't
  correctly parse timezone offsets that use a colon to separate hours
  and minutes.

- RFC5424 allows for 1 to 6 digits for the fractional second. This
  updates the pipeline to also accept 3 decimals (milliseconds).

(cherry picked from commit d30b4dc)
@adriansr adriansr added v7.2.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jun 14, 2019
adriansr added a commit that referenced this pull request Jun 14, 2019
) (#12543)

The date format used by the system/syslog pipeline has a couple of
issues:

- Since Elasticsearch switched to Java time from Joda time, it won't
  correctly parse timezone offsets that use a colon to separate hours
  and minutes.

- RFC5424 allows for 1 to 6 digits for the fractional second. This
  updates the pipeline to also accept 3 decimals (milliseconds).

(cherry picked from commit d30b4dc)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…stic#12529) (elastic#12543)

The date format used by the system/syslog pipeline has a couple of
issues:

- Since Elasticsearch switched to Java time from Joda time, it won't
  correctly parse timezone offsets that use a colon to separate hours
  and minutes.

- RFC5424 allows for 1 to 6 digits for the fractional second. This
  updates the pipeline to also accept 3 decimals (milliseconds).

(cherry picked from commit dd4d52f)
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.

4 participants