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

[chore] kafkareceiver: embed configkafka types #38817

Merged
merged 1 commit into from
Mar 24, 2025

Conversation

axw
Copy link
Contributor

@axw axw commented Mar 20, 2025

Description

Use configkafka.ClientConfig and configkafka.ConsumerConfig, and the newly extracted consumer client. Redundant validation tests have been removed, as they are now covered by configkafka.

This is a non-functional change, as the config defaults do not change for this component. Nevertheless I have manually tested -- see Testing section below.

Link to tracking issue

Fixes #38411

Testing

Updated unit tests.

Manually tested against Redpanda:

$ docker run --publish=9093:9093 --health-cmd "rpk cluster health | grep 'Healthy:.*true'" docker.redpanda.com/redpandadata/redpanda:v23.1.11 redpanda start --kafka-addr=internal://0.0.0.0:9092,external://0.0.0.0:9093 --smp=1 --memory=1G --mode=dev-container

Ran the collector with kafkametrics -> kafkaexporter -> Redpanda -> kafkareceiver -> debugexporter:

receivers:
  kafka:
    brokers: [localhost:9093]
  kafkametrics:
    brokers: [localhost:9093]
    collection_interval: 10s
    scrapers: [topics, consumers]

exporters:
  debug:
    verbosity: detailed
  kafka:
    brokers: [localhost:9093]

service:
  pipelines:
    metrics/kafka:
      receivers: [kafka]
      exporters: [debug]
    metrics:
      receivers: [kafkametrics]
      exporters: [kafka]

Documentation

Updated README to clarify that protocol_version is not required.

Use configkafka.ClientConfig and configkafka.ConsumerConfig,
and the newly extracted consumer client. This is a non-functional
change, as the config defaults do not change for this component.

Relates to open-telemetry#38411
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -4,17 +4,8 @@ kafka:
- "foo:123"
- "bar:456"
resolve_canonical_bootstrap_servers_only: true
client_id: otel-collector
Copy link
Member

Choose a reason for hiding this comment

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

could we keep this to make sure the API does not change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean auth and metadata? I removed them because we have coverage for the ClientConfig type in the internal/kafka/configkafka package. We're still verifying client_id and group_id below, as a minimal subset of the CientConfig and ConsumerConfig types - to verify that we're embedding them correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it would be great to know which API you mean here @pavolloffay

Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

These changes look great to me and really simplifies the components and maintaining them.

@atoulme atoulme merged commit 9fe267f into open-telemetry:main Mar 24, 2025
184 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 24, 2025
@axw axw deleted the kafkareceiver-configkafka branch March 25, 2025 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consolidate common Kafka configuration within internal/kafka
5 participants