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

promtail: Add support for using syslog message timestamp #2914

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Nov 10, 2020

What this PR does / why we need it:

Currently promtail sets the timestamp of incoming syslog messages to the
time it was received by promtail. In some cases, it is preferable to use
the source timestamp instead.

This adds a new use_incoming_timestamp option to the syslog target
config, which allows users to opt-in to the behavior of using the
timestamp on the message, if one exists.

Checklist

  • Documentation added
  • Tests updated

@chancez chancez changed the title promtail: Add support for preserving syslog message timestamp promtail: Add support for using syslog message timestamp Nov 10, 2020
@codecov-io
Copy link

codecov-io commented Nov 10, 2020

Codecov Report

Merging #2914 (c2c8c71) into master (bdf8dca) will decrease coverage by 0.02%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2914      +/-   ##
==========================================
- Coverage   61.70%   61.68%   -0.03%     
==========================================
  Files         181      181              
  Lines       14719    14724       +5     
==========================================
  Hits         9083     9083              
- Misses       4808     4811       +3     
- Partials      828      830       +2     
Impacted Files Coverage Δ
pkg/promtail/scrapeconfig/scrapeconfig.go 12.00% <ø> (ø)
pkg/promtail/targets/syslog/syslogtarget.go 73.28% <42.85%> (-1.33%) ⬇️
pkg/querier/queryrange/downstreamer.go 95.29% <0.00%> (-2.36%) ⬇️

@chancez
Copy link
Contributor Author

chancez commented Nov 10, 2020

There doesn't seem to be any mock/testing facilities for the syslog code yet, so I didn't add any tests. I ran this on my own environment with a custom image and it's been working as expected. No out of order issues when using rsyslog with the recommended configuration either. Given this, it might make sense to enable this by default, but that would technically backwards incompatible, so I left it as a default false option.

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Thanks, it seems reasonable and the code looks good. I'll let @cyriltovena or @slim-bean merge as I'm only cursorily familiar with this part of our code.

@slim-bean
Copy link
Collaborator

Fixes #2461

@slim-bean
Copy link
Collaborator

I'm going back and forth a little myself on what the default should be here, I think I agree with leaving the default to use time.Now(), mainly because I don't know how likely out of order messages could be from syslog streams and changing this behavior between releases might cause some big problems for people.

Copy link
Contributor

@achatterjee-grafana achatterjee-grafana left a comment

Choose a reason for hiding this comment

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

Added a copy-edit suggestion.

docs/sources/clients/promtail/configuration.md Outdated Show resolved Hide resolved
Currently promtail sets the timestamp of incoming syslog messages to the
time it was received by promtail. In some cases, it is preferable to use
the source timestamp instead.

This adds a new `use_message_timestamp` option to the syslog target
config, which allows users to opt-in to the behavior of using the
timestamp on the message, if one exists.
@chancez
Copy link
Contributor Author

chancez commented Nov 11, 2020

Added the recommended docs changes, squashed & rebased.

@owen-d owen-d merged commit f0d4adc into grafana:master Nov 11, 2020
@chancez
Copy link
Contributor Author

chancez commented Nov 11, 2020

As another note, for anyone who comes along this, if someone wants to enable this option, but also have some logs set the timestamp to the received time, you can configure rsyslog with a custom template like this:

$template RSYSLOG_SyslogProtocol23FormatTimeGenerated, "<%PRI%>1 %timegenerated:::date-rfc3339% %HOSTNAME% %APP-NAME% %PROCID% %MSGID% %STRUCTURED-DATA% %msg%\n"

This is useful if you have logs incoming with correct timestamps most the time, but perhaps some hosts the time is out of sync, so you can do this inside an if condition for rsyslog.

@chancez chancez deleted the syslog_preserve_timestamp branch November 11, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants