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.

Sign up for GitHub

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

Jump to bottom

[exporter/kafka] Add ability to specifiy Kafka Client ID #30145

Merged
merged 6 commits into from
Dec 29, 2023
Merged

[exporter/kafka] Add ability to specifiy Kafka Client ID #30145

merged 6 commits into from
Dec 29, 2023

Conversation

richardartoul
Copy link
Contributor

@richardartoul richardartoul commented Dec 20, 2023 •
edited
Loading

Description: This P.R adds the ability to specify the Kafka client ID in the Sarama Kafka producer when constructing the client.

Link to tracking Issue: Resolves 30144

Testing: I didn't add any tests because its just propagating a configuration value, and the Sarama client (including its mock) does not expose any mechanism for asserting on which client ID was propagated with each produce request because it happens at the TCP protocol level and is not part of the application level payload.

https://github.com/IBM/sarama/blob/main/mocks/mocks.go#L40

Documentation: I've updated the documentation and change log to document this feature.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Dec 20, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

crobert-1
crobert-1 reviewed Dec 20, 2023
View reviewed changes
Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

It would be good to add some tests to ensure the config option is picked up properly, and if possible, exported with the proper client ID.

exporter/kafkaexporter/kafka_exporter.go Show resolved Hide resolved
@richardartoul
Copy link
Contributor Author

@crobert-1 good call, it was broken haha. Sorry my first time contributing to this codebase. I've added tests that the YAML is parsed properly. I'm not sure how to do any further validation though cause the SaramaSyncProducer interface doesn't expose a method that will tell you which client ID has been configured, and the mock used in the tests does not have the ability to inspect the configured client ID either.

@richardartoul
Copy link
Contributor Author

Ok I also figured out how to compile the program from scratch and I tested it with a custom yaml configuration file and I was able to confirm that the default of sarama is being sent over the wire when its left unconfigured, and when I set a custom client ID, I observed that one being transmitted at the protocol layer so it definitely works.

@crobert-1
Copy link
Member

Link to tracking Issue: 30144

Minor request: Can you change the above line in your description to:

Link to tracking issue: Resolves #30144

This will allow our GitHub CI/CD automation to automatically close the referenced issue 👍

@fatsheep9146 fatsheep9146 added the ready to merge Code review completed; ready to merge by maintainers label Dec 22, 2023
@richardartoul
Copy link
Contributor Author

Link to tracking Issue: 30144

Minor request: Can you change the above line in your description to:

Link to tracking issue: Resolves #30144

This will allow our GitHub CI/CD automation to automatically close the referenced issue 👍

Done!

@fatsheep9146
Copy link
Contributor

@open-telemetry/collector-maintainers could you help final review and merge this pr?

@github-actions github-actions bot mentioned this pull request Dec 26, 2023
Closed
@MovieStoreGuy
Copy link
Contributor

Seems good to me :)

Resolved #30144

@MovieStoreGuy MovieStoreGuy merged commit f089aa2 into open-telemetry:main Dec 29, 2023
90 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 29, 2023
@richardartoul richardartoul deleted the ra/add-support-client-id branch December 29, 2023 17:16
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jan 10, 2024
@richardartoul @cparkins
[exporter/kafka] Add ability to specifiy Kafka Client ID (open-teleme…
20cabf7 
…try#30145)

**Description:** This P.R adds the ability to specify the Kafka client
ID in the Sarama Kafka producer when constructing the client.

**Link to tracking Issue:** Resolves 30144

**Testing:** I didn't add any tests because its just propagating a
configuration value, and the Sarama client (including its mock) does not
expose any mechanism for asserting on which client ID was propagated
with each produce request because it happens at the TCP protocol level
and is not part of the application level payload.

https://github.com/IBM/sarama/blob/main/mocks/mocks.go#L40

**Documentation:** I've updated the documentation and change log to
document this feature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavolloffay pavolloffay pavolloffay approved these changes

@fatsheep9146 fatsheep9146 fatsheep9146 approved these changes

@crobert-1 crobert-1 crobert-1 approved these changes

@MovieStoreGuy MovieStoreGuy Awaiting requested review from MovieStoreGuy MovieStoreGuy is a code owner

Assignees

@fatsheep9146 fatsheep9146

Labels
exporter/kafka ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Milestone
v0.92.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@richardartoul @crobert-1 @fatsheep9146 @MovieStoreGuy @pavolloffay