-
Notifications
You must be signed in to change notification settings - Fork 182
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
Support RFC3164 syslog entries #1556
Conversation
@clayton-cornell it looks like there's one test failing (really timing out):
I doubt that it's related to this PR per se but it could be from the bump to |
Possible. The macos test fails once in a while. I'll leave it as-is for now. First we need a code review from someone in @grafana/grafana-alloy-maintainers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only two small comments for the Alloy part
internal/component/loki/source/syslog/internal/syslogtarget/syslogtarget.go
Outdated
Show resolved
Hide resolved
@ptodev thanks for triggering tests! I just pushed a fix for the failing one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for docs
@ptodev is this good to merge? |
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
This reverts commit fb83e9a.
Hi @wildum, would you mind reviewing the PR again please? It should be ready to merge, but I pushed a few changes to it and it'd be nice to have some else double check them :) |
Would it be easy to add 'raw' as format, apart from 'RFC3164' and 'RFC5424' formats? So, the parsing of the message can be done in a I have firewall devices sending syslog messages in CEF format. |
@sboschman I'd suggest creating an issue for that, it's probably best as a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I missed the notifications. LGTM, I will merge once the conflicts are resolved
@wildum no worries, thanks for taking a look! I've merged master to eliminate the conflicts. |
There's one failing test but it doesn't look related?
|
yeah seems like a flaky test, restarting the build |
PR Description
Ports the changes from grafana/loki#12810 here.
Which issue(s) this PR fixes
Fixes #305.
Notes to the Reviewer
Suggest reviewing commit-by-commit.
PR Checklist