-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add logs support on Kafka receiver #2944
Add logs support on Kafka receiver #2944
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2944 +/- ##
==========================================
+ Coverage 91.58% 91.63% +0.04%
==========================================
Files 312 312
Lines 15351 15441 +90
==========================================
+ Hits 14059 14149 +90
Misses 883 883
Partials 409 409
Continue to review full report at Codecov.
|
@@ -27,6 +27,15 @@ type Unmarshaller interface { | |||
Encoding() string | |||
} | |||
|
|||
// logsUnmarshaller deserializes the message body. | |||
type logsUnmarshaller interface { |
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 this is private means you don't expect users to implement? Also probably we should rename the previous one to TracesUnmarshaller.
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.
Thanks, @bogdandrutu. Fixed in 0cb875f
And actually, there're many methods to be renamed (adding Traces
). I would like to do this in another PR cuz it's already not a small one. Thoughts?
receiver/kafkareceiver/factory.go
Outdated
@@ -53,18 +53,30 @@ func WithAddUnmarshallers(encodingMarshaller map[string]Unmarshaller) FactoryOpt | |||
} | |||
} | |||
|
|||
// WithAddLogsUnmarshallers adds logsUnmarshallers. | |||
func WithAddLogsUnmarshallers(encodingMarshaller map[string]logsUnmarshaller) FactoryOption { |
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.
Names are inconsistent, probably worth updating the previous one to include traces.
Also I would change the API to:
func WithLogsUnmarshallers(encoding string, unmarshaller logsUnmarshaller) FactoryOption {}
Or even consider to add a name
func on the interface to simplify usage.
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.
Actually there is an Encoding
so we can do:
func WithLogsUnmarshallers(unmarshallers logsUnmarshaller...) FactoryOption {}
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.
The method is actually consistent with the traces' method here:
opentelemetry-collector/receiver/kafkareceiver/factory.go
Lines 47 to 54 in 6458914
// WithAddUnmarshallers adds marshallers. | |
func WithAddUnmarshallers(encodingMarshaller map[string]Unmarshaller) FactoryOption { | |
return func(factory *kafkaReceiverFactory) { | |
for encoding, unmarshaller := range encodingMarshaller { | |
factory.unmarshalers[encoding] = unmarshaller | |
} | |
} | |
} |
I'd like to keep the parameters consistent with trace's method and there are multiple encodings and return one factory in traces' method. But I'd like to rename the method name to be consistent in a subsequent PR. Thoughts?
Description:
Add logs support on Kafka exporter
Link to tracking Issue:
Related to #1331
Related to open-telemetry/opentelemetry-collector-contrib#268
Related to #2943
Testing:
Documentation:
Update Kafka receiver README and Receiver README.