-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[receiver/kafka] Add support for topic and exclude_topic as an array #44575
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
base: main
Are you sure you want to change the base?
Conversation
axw
left a comment
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'm a bit torn on the deprecation.
On the one hand, "topics" seems like the best config name, OTOH it's a bit annoying to deprecate config again. A couple of other options:
- Keep both "topic" and "topics" indefinitely, allow only one or the other. Might be a little bit confusing as to why there are two config options.
- Keep "topic" (don't add "topics"), and allow it to be either an array or a single string. Maybe that's a bit dirty.
@paulojmdias @MovieStoreGuy @pavolloffay any opinions?
|
I’m also a bit torn on the deprecation. I agree with @axw that there won’t be a perfect solution for this change. However, even though it seems a bit dirty, I’d go with option 2, keeping If we want to avoid mixed types but also avoid the topic/topics confusion, we could keep |
|
I personally would prefer if we deprecate I agree, It'll be annoying for that initial version but I think it captures the semantic meaning better than the singular. |
|
After sleeping on it, I'm now inclined to deprecate the existing config and replace with plural. It's annoying, but better now than later when increasingly more users start using the receiver. @paulojmdias are you OK with this? As an aside, we should really look into marking the receiver stable in the not too distant future. It's pretty solid. Once we do that, we'll have less flexibility to make breaking changes though. |
Yes, I'm ok with that. 👍 |
axw
left a comment
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 @khushijain21, looks good overall.
Can you please add a warning level log message (probably to the Start method) if topic``exclude_topic is configured, recommending that topics/exclude_topics be used instead?
Co-authored-by: Andrew Wilkins <[email protected]>
Co-authored-by: Andrew Wilkins <[email protected]>
Co-authored-by: Andrew Wilkins <[email protected]>
receiver/kafkareceiver/config.go
Outdated
| if len(zeroConfig.Logs.Topics) == 0 { | ||
| c.Logs.Topics = []string{zeroConfig.Logs.Topic} | ||
| } else { | ||
| return fmt.Errorf("both logs.topic and logs.topics cannot be set") |
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.
This sort of things should be done in Validate
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 agree, because we were mutating an empty logs.topics in Unmarshall function here https://github.com/khushijain21/opentelemetry-collector-contrib/blob/kafkarec/receiver/kafkareceiver/config.go#L66 - doing this check in Validate would result in a false error. For ex: it would fail even when just "logs.topic" is set. (We are appending logs.topics above so).
We would have to clear logs.Topic in case we really want this to go in Validate.
So I guess we can keep it here if this is okay. Either way, your call
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.
anyway, I moved it to validate in this commit 0fdab34
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.
We would have to clear logs.Topic in case we really want this to go in Validate
I cleared the logs.Topic but when we unmarshal it here again https://github.com/khushijain21/opentelemetry-collector-contrib/blob/kafkarec/receiver/kafkareceiver/config.go#L135 - it sets log.topic back to what user configured.
Hence validate() method returns a false error. I think it is okay to do this check in Unmarshal
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.
ICYMI (#44575 (review)):
Can you please add a warning level log message (probably to the Start method) if topic/exclude_topic is configured, recommending that topics/exclude_topics be used instead?
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.
yeah I missed it, fixed it in 0fdab34
paulojmdias
left a comment
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 version here should be the next one.
| - `resolve_canonical_bootstrap_servers_only` (default = false): Whether to resolve then reverse-lookup broker IPs during startup | ||
| - `logs` | ||
| - `topic` (default = otlp\_logs): The name of the Kafka topic from which to consume logs. | ||
| - `topic` (Deprecated [v0.142.0]: use `topics`) |
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.
| - `topic` (Deprecated [v0.142.0]: use `topics`) | |
| - `topic` (Deprecated [v0.141.0]: use `topics`) |
| - `topics` (default = otlp\_logs): List of kafka topics from which to consume logs | ||
| - `encoding` (default = otlp\_proto): The encoding for the Kafka topic. See [Supported encodings](#supported-encodings). | ||
| - `exclude_topic` (default = ""): When using regex topic patterns (prefix with `^`), this regex pattern excludes matching topics. Only works with franz-go client and when topic uses regex. | ||
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) |
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.
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) | |
| - `exclude_topic` (Deprecated [v0.141.0]: use `exclude_topics`) |
| - `exclude_topics` (default = ""): When using regex topic patterns (prefix with `^`), this regex pattern excludes matching topics. Only works with franz-go client and when topic uses regex. | ||
| - `metrics` | ||
| - `topic` (default = otlp\_metrics): The name of the Kafka topic from which to consume metrics. | ||
| - `topic` (Deprecated [v0.142.0]: use `topics`) |
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.
| - `topic` (Deprecated [v0.142.0]: use `topics`) | |
| - `topic` (Deprecated [v0.141.0]: use `topics`) |
| - `topics` (default = otlp\_metrics): List of Kafka topic from which to consume metrics. | ||
| - `encoding` (default = otlp\_proto): The encoding for the Kafka topic. See [Supported encodings](#supported-encodings). | ||
| - `exclude_topic` (default = ""): When using regex topic patterns (prefix with `^`), this regex pattern excludes matching topics. Only works with franz-go client and when topic uses regex. | ||
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) |
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.
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) | |
| - `exclude_topic` (Deprecated [v0.141.0]: use `exclude_topics`) |
| - `exclude_topics` (default = ""): When using regex topic patterns (prefix with `^`), this regex pattern excludes matching topics. Only works with franz-go client and when topic uses regex. | ||
| - `traces` | ||
| - `topic` (default = otlp\_spans): The name of the Kafka topic from which to consume traces. | ||
| - `topic` (Deprecated [v0.142.0]: use `topics`) |
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.
| - `topic` (Deprecated [v0.142.0]: use `topics`) | |
| - `topic` (Deprecated [v0.141.0]: use `topics`) |
| - `topics` (default = otlp\_spans): List of Kafka topic from which to consume traces. | ||
| - `encoding` (default = otlp\_proto): The encoding for the Kafka topic. See [Supported encodings](#supported-encodings). | ||
| - `exclude_topic` (default = ""): When using regex topic patterns (prefix with `^`), this regex pattern excludes matching topics. Only works with franz-go client and when topic uses regex. | ||
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) |
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.
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) | |
| - `exclude_topic` (Deprecated [v0.141.0]: use `exclude_topics`) |
| - `exclude_topics` (default = ""): When using regex topic patterns (prefix with `^`), this regex pattern excludes matching topics. Only works with franz-go client and when topic uses regex. | ||
| - `profiles` | ||
| - `topic` (default = otlp\_profiles): The name of the Kafka topic from which to consume profiles. | ||
| - `topic` (Deprecated [v0.142.0]: use `topics`) |
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.
| - `topic` (Deprecated [v0.142.0]: use `topics`) | |
| - `topic` (Deprecated [v0.141.0]: use `topics`) |
| If this is set, it will take precedence over the default value for those fields. | ||
| - `encoding` (Deprecated [v0.124.0]: use `logs::encoding`, `traces::encoding`, or `metrics::encoding`). | ||
| If this is set, it will take precedence over the default value for those fields. | ||
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) |
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.
| - `exclude_topic` (Deprecated [v0.142.0]: use `exclude_topics`) | |
| - `exclude_topic` (Deprecated [v0.141.0]: use `exclude_topics`) |
Description
This PR adds support for topics and exclude_topics. (accepts a list instead of a single string).
It also deprecates
topicandexclude_topicin favor of two new config options introduced above.Link to tracking issue
Fixes #44477
Testing
Added UT
Performed manual testing
Documentation
Added