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/targets/syslog: Enable best effort parsing for Syslog messages #5409

Merged
merged 2 commits into from
Feb 25, 2022
Merged

Conversation

ldb
Copy link
Contributor

@ldb ldb commented Feb 16, 2022

What this PR does / why we need it:
This change enables best effort parsing for Syslog messages in the Syslog target within Promtail.
Right now, a malformed message will stop the syslog parser. However, the criteria for a message to be considered "valid" is very low.
By using the best effort option, a malformed or partially parsed message will not stop the parsing loop anymore, which otherwise would eventually close the TCP connection.
Parsing errors will still be logged and tracked in metrics.
Malformed messages will not be passed back to Promtail.

Which issue(s) this PR fixes:
Fixes #5395
Fixes #4270

Also presumably:
Fixes #4871
Fixes #4808

Special notes for your reviewer:

Just checked this in our setup and was able to verify that this change indeed fixes TCP connection problems in Rsyslog (with error -2027).

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@ldb ldb requested a review from a team as a code owner February 16, 2022 11:08
@stephenafamo
Copy link

I really need this merged. The current syslog parser is extremely picky.

Thanks for this @ldb

@ldb
Copy link
Contributor Author

ldb commented Feb 23, 2022

@stephenafamo just to be clear, this PR does not change the parser to accept malformed messages. It just makes it so that a malformed Syslog message does not close the underlying transport anymore. Malformed messages will still not be passed to Loki in this way.

@stephenafamo
Copy link

I would try to test this by myself, but my issue was that the syslog messages generated by the log/syslog package was rejected by promtail.

  1. log/syslog does not include the version
  2. promtail seems to require every field including does that are allowed to be nil according to RFC5424. So even if you fork the package add the version (which I did), promtail still complains that procid, msgid, structured-data e.t.c. are missing even if they are not required to be present.

I am not sure I have an easy way to test your changes, but I'd like to see if the best-effort parsing is more usable -- at least with log/syslog.

@ldb
Copy link
Contributor Author

ldb commented Feb 23, 2022

There is no mention about log/syslog being compliant with RFC5424, the Syslog standard.
Looking at the code, it seems to be one of the earliest packages in the stdlib and actually has been frozen some while ago. Your best bet would likely be migrating away from it.

@stephenafamo
Copy link

Sadly I couldn't find a maintained alternative. Any suggestions would be welcome.

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

This LGTM and seems low risk, thanks for the contribution!

@slim-bean slim-bean merged commit 6c84304 into grafana:main Feb 25, 2022
@ldb
Copy link
Contributor Author

ldb commented Feb 25, 2022

@slim-bean I did not add this to the CHANGELOG.md yet. Is this something I should update this branch for and make a new PR or what would be the best way?

ldb added a commit to ldb/loki that referenced this pull request Feb 25, 2022
@stephenafamo
Copy link

Update: Tried with these changes, still rejects the logs from the standard library's log/syslog package. This doesn't seem to be promtail's fault at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants