Skip to content

Add option for loki to use system timestamp#9114

Closed
loganmc10 wants to merge 1 commit intoinfluxdata:masterfrom
loganmc10:loki_time
Closed

Add option for loki to use system timestamp#9114
loganmc10 wants to merge 1 commit intoinfluxdata:masterfrom
loganmc10:loki_time

Conversation

@loganmc10
Copy link
Copy Markdown
Contributor

@loganmc10 loganmc10 commented Apr 9, 2021

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests. (I think the unit tests are still pretty well covered)

Loki has very strict ordering requirements. You cannot submit a log line with a timestamp older than what already exists (for a matching label set).

I've been getting HTTP 400 errors from the Loki output plugin (SNMP trap input). I added a file output and I noticed that items are written to the file slightly out of order (timestamps are not always ascending).

This change adds an option (system_timestamp) to the Loki output plugin to allow it to use the system timestamp in the Loki message, rather than the metrics provided timestamp. This should mean that timestamp values are always ascending.

@eraac since you wrote the original Loki plugin

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Apr 9, 2021
Copy link
Copy Markdown
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@loganmc10
Copy link
Copy Markdown
Contributor Author

!signed-cla

Copy link
Copy Markdown
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

Copy link
Copy Markdown
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Why hasn't CI ran on this branch?

I don't think we should be adding this to outputs. it belongs at the processor layer so we don't have to repeat it for every output. You can do this now with processors.Starlark

@loganmc10
Copy link
Copy Markdown
Contributor Author

Fair enough, I didn't realize there was a processor that could modify the timestamp of a metric, thanks for the info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants