-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[receiver/gelfreceiver] Added Support for GELF log receiver. #33858
base: main
Are you sure you want to change the base?
[receiver/gelfreceiver] Added Support for GELF log receiver. #33858
Conversation
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.
In order to accept a new component you must formally propose it in an issue and find a sponsor.
0db0868
to
d187f9d
Compare
@djaglowski I don't have a sponsor, is it possible for you to help with that part? or is there a process to get sponsor, then please share the readme/doc for it? I know about the list of sponsors mentioned here: Link - but how do I ask someone to become sponsor/reviewer for this PR? I guess it's voluntary so I cannot really ask randomly. Also there's a Slack thread running for it, we can communicate there as well: thread link |
I don't have capacity to voluntarily sponsor any additional components. Hopefully someone else is interested. Otherwise, you can always host the component elsewhere and others can still build it into their collectors using the Collector Builder. |
@djaglowski Instead of creating a separate receiver, if I add this as a part of UDP receiver then will that be okay in terms of sponsorship as you're the contributor? GELF will be like a CODEC. |
I don't believe GELF support should be a concern of the UDP receiver. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
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.
I don't see any tests, could you please add some to ensure the receiver works as expected?
@@ -214,6 +214,7 @@ receivers: | |||
- gomod: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/windowseventlogreceiver v0.102.0 | |||
- gomod: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/zipkinreceiver v0.102.0 | |||
- gomod: github.com/open-telemetry/opentelemetry-collector-contrib/receiver/zookeeperreceiver v0.102.0 | |||
- gomod: gelfreceiver latest |
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.
Please peg to a specific version (commit hash works in absence of tags) instead of using latest
@@ -141,6 +139,7 @@ require ( | |||
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/filestatsreceiver v0.102.0 | |||
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/flinkmetricsreceiver v0.102.0 | |||
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/fluentforwardreceiver v0.102.0 | |||
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/gelfreceiver v0.0.0 |
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.
I believe this should be v0.102.0
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.
yes, and same in the builder yaml. If anything, you should defer adding this receiver to cmd/otelcontribcol to a second PR.
defaultReaders = 1 | ||
defaultProcessors = 1 | ||
defaultUDPMaxQueueLength = 100 | ||
defaultListenAddress = "0.0.0.0:31250" |
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.
Please use localhost as the default binding instead of 0.0.0.0
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
@hughesjj I tried talking to folks in SIG otel-collector meeting, they said that we need sponsor for this PR and that it won't be accepted unless there's someone sponsoring this, so before I spend more effort on it - wanted to know if you would be able to sponsor this PR or get this accepted given that all comments are fixed? I don't want my efforts to go into vain. 😞 (Also I didn't realize there were more comments, sorry for the late response, missed notifications on it.) |
@@ -0,0 +1,3 @@ | |||
//go:generate mdatagen metadata.yaml |
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.
copyright header is missing
go 1.21.0 | ||
|
||
toolchain go1.21.11 | ||
go 1.22.0 |
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.
please revert
asyncProcessors: c.AsyncProcessors, | ||
} | ||
|
||
// fmt.Println("GELF receiver config validated.") |
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.
// fmt.Println("GELF receiver config validated.") |
} | ||
|
||
if c.ListenAddress == "" { | ||
return nil, fmt.Errorf("missing required parameter 'listen_address'") |
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.
return nil, fmt.Errorf("missing required parameter 'listen_address'") | |
return nil, errors.New("missing required parameter 'listen_address'") |
if c.Protocol != "udp" { | ||
return nil, fmt.Errorf("supported protocols - udp, invalid protocol: %s", c.Protocol) | ||
} | ||
if c.AsyncReaders <= 0 && c.AsyncReaders < 20 { |
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.
are you sure of these conditions?
if c.AsyncProcessors <= 0 && c.AsyncProcessors < 20 { | ||
return nil, fmt.Errorf("invalid async_processors: %d", c.AsyncProcessors) | ||
} | ||
if c.UDPMaxQueueLength <= 0 && c.UDPMaxQueueLength < 65535 { |
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.
if c.UDPMaxQueueLength <= 0 && c.UDPMaxQueueLength < 65535 { | |
if c.UDPMaxQueueLength <= 0 || c.UDPMaxQueueLength > 65535 { |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Description:
Added support for GELF based logs - please read this document to understand GELF implementation.
Tremor's implementation for GELF receiver
Otel-collector's GELF implementation document: https://docs.google.com/document/d/1P9JcrXrORqI0wsnRgrcUroluutf3rVrcUCIgLeSpFm0/edit?usp=sharing
Link to tracking Issue: #33861
Testing:
Documentation: Work-in-progress.