Skip to content

[chore] internal/kafka: introduce kafkatest package #38882

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

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

axw
Copy link
Contributor

@axw axw commented Mar 23, 2025

Description

This package will hold test-related code for Kafka components.

Initially, the package provides a single function for creating a fake Kafka cluster using franz-go's kfake, along with a configkafka.ClientConfig for connecting to it. We use this in new tests for NewSaramaClient.

Link to tracking issue

N/A

Testing

N/A, only adding test code.

Documentation

N/A

This package will hold test-related code for Kafka components.
Initially is provides a single function for creating a fake
Kafka cluster using franz-go's kfake, along with a configkafka
ClientConfig for connecting to it. We use this in new tests
for NewSaramaClient.
Comment on lines 26 to 30
func init() {
// Disable the go-metrics registry, as there's a goroutine
// leak in the Sarama code that uses it.
metrics.UseNilMetrics = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am never a big fan of seeing init statements, is there a way that this could be raised as in issue in the upstream repo to address the leak of the go routine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Once we have the issue created, can you comment it here so we can follow up later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was an issue, it went stale: IBM/sarama#1321

My fix would be to swap to franz-go ;)

I'll see if we can work around the bug another way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

P.S. this is also the documented by Sarama as the way to disable metrics: https://github.com/IBM/sarama/blob/d6ca80a198c4fbc467c67f7430182eda6881b12c/config.go#L513

So I thought it made sense for the tests, but not non-test code because that might inadvertently disable metrics in other components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion in IBM/sarama#1321 (comment) to explicitly unregister metrics doesn't help. I can't see a better alternative.

Copy link
Contributor

@MovieStoreGuy MovieStoreGuy Mar 24, 2025

Choose a reason for hiding this comment

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

Mm, I'd still like a reference in the comment to the issue so I can come back to it once I get some free time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 added a comment

@MovieStoreGuy MovieStoreGuy merged commit 2ab9d69 into open-telemetry:main Mar 25, 2025
180 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 25, 2025
@axw axw deleted the kafkatest branch March 26, 2025 05:25
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
…8882)

#### Description

This package will hold test-related code for Kafka components.

Initially, the package provides a single function for creating a fake
Kafka cluster using franz-go's kfake, along with a
configkafka.ClientConfig for connecting to it. We use this in new tests
for NewSaramaClient.

#### Link to tracking issue

N/A

#### Testing

N/A, only adding test code.

#### Documentation

N/A
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.

4 participants