-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 Kafka OTEL receiver/ingester #2221
Add Kafka OTEL receiver/ingester #2221
Conversation
@rubenvp8510 or and @objectiser could you please review? |
Codecov Report
@@ Coverage Diff @@
## master #2221 +/- ##
==========================================
+ Coverage 96.16% 96.20% +0.03%
==========================================
Files 219 219
Lines 10632 10632
==========================================
+ Hits 10224 10228 +4
+ Misses 352 350 -2
+ Partials 56 54 -2
Continue to review full report at Codecov.
|
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 - just some minor comments.
Main question, not for this PR, is how we work with/transition to a pure OTel/Kafka impl - would be better if using OTel model for kafka messages.
|
||
kafkaCfg := cfg.Receivers[TypeStr].(*Config) | ||
assert.Equal(t, TypeStr, kafkaCfg.Name()) | ||
assert.Equal(t, "jaeger-prod", kafkaCfg.Topic) |
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.
Might be good to allow one of the flag values to be passed to the final config?
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.
kafkaCfg.Options.Kerberos.Username
is passed from jaeger config file
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 issue is that none of the CLI flag values are making it into the final config - would be good if atleast one did for completeness.
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.
Flags or config file are the same source for viper. A couple of lines above we test that flags are passed to the factory.
Yeah, this is not related to this PR, but it is definitely something on the roadmap. |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
cae7479
to
86fb9ac
Compare
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.
Just a couple of comments to consider.
|
||
kafkaCfg := cfg.Receivers[TypeStr].(*Config) | ||
assert.Equal(t, TypeStr, kafkaCfg.Name()) | ||
assert.Equal(t, "jaeger-prod", kafkaCfg.Topic) |
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 issue is that none of the CLI flag values are making it into the final config - would be good if atleast one did for completeness.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
* Add Kafka OTEL receiver/ingester Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Fix review Signed-off-by: Pavol Loffay <ploffay@redhat.com> * Use tls.cert from flags Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Related to #2205
This PR adds Kafka OTEL receiver. This receiver will be used in OTEL ingester that will come in a follow-up PR.
PR for kafka exporter #2135