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

Support RFC3164 syslog entries #1556

Merged
merged 17 commits into from
Sep 27, 2024
Merged

Support RFC3164 syslog entries #1556

merged 17 commits into from
Sep 27, 2024

Conversation

sushain97
Copy link
Contributor

@sushain97 sushain97 commented Aug 27, 2024

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

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2024

CLA assistant check
All committers have signed the CLA.

@clayton-cornell clayton-cornell requested a review from a team August 27, 2024 14:36
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Aug 27, 2024
@clayton-cornell clayton-cornell requested a review from a team August 28, 2024 21:08
@sushain97
Copy link
Contributor Author

@clayton-cornell it looks like there's one test failing (really timing out):

FAIL	github.com/grafana/alloy/internal/component/loki/process	600.159s

I doubt that it's related to this PR per se but it could be from the bump to github.com/grafana/loki/v3 ?

@clayton-cornell
Copy link
Contributor

it could be from the bump to github.com/grafana/loki/v3 ?

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.

Copy link
Contributor

@wildum wildum left a 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/types.go Outdated Show resolved Hide resolved
internal/component/loki/source/syslog/types.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
internal/component/loki/source/syslog/types.go Outdated Show resolved Hide resolved
@sushain97
Copy link
Contributor Author

@wildum @ptodev Thanks for taking a look! I think I've addressed the comments.

@sushain97
Copy link
Contributor Author

@ptodev thanks for triggering tests! I just pushed a fix for the failing one.

Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Approving for docs

@sushain97
Copy link
Contributor Author

@ptodev is this good to merge?

@ptodev
Copy link
Contributor

ptodev commented Sep 13, 2024

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 :)

@ptodev ptodev requested a review from wildum September 13, 2024 17:27
@sboschman
Copy link

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 loki.process component.

I have firewall devices sending syslog messages in CEF format.

@sushain97
Copy link
Contributor Author

@sboschman I'd suggest creating an issue for that, it's probably best as a separate PR.

@sushain97
Copy link
Contributor Author

@wildum @ptodev please let me know if I should resolve the conflicts with master. Don't want to resolve them just to see it get conflicted again 😅

@sushain97
Copy link
Contributor Author

@ptodev @wildum 👋 Just checking in again on this. I'm happy to resolve the conflicts if someone can merge it afterwards.

Copy link
Contributor

@wildum wildum left a 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

@sushain97
Copy link
Contributor Author

@wildum no worries, thanks for taking a look! I've merged master to eliminate the conflicts.

@sushain97
Copy link
Contributor Author

There's one failing test but it doesn't look related?

--- FAIL: TestAdditionalLabels (3.01s)
    events_test.go:276: 
        	Error Trace:	/drone/src/internal/component/mimir/rules/kubernetes/events_test.go:276
        	            				/usr/local/go/src/runtime/asm_amd64.s:1695
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestAdditionalLabels
    events_test.go:272: 
        	Error Trace:	/drone/src/internal/component/mimir/rules/kubernetes/events_test.go:272
        	Error:      	Condition never satisfied
        	Test:       	TestAdditionalLabels
level=info ts=2024-09-27T07:54:17.90219047Z msg="shutting down event loop"
FAIL
FAIL	github.com/grafana/alloy/internal/component/mimir/rules/kubernetes	3.360s

@wildum
Copy link
Contributor

wildum commented Sep 27, 2024

yeah seems like a flaky test, restarting the build

@wildum wildum merged commit f9054d3 into grafana:main Sep 27, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error parsing syslog stream UDP
6 participants