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

[exporter/kafka]Decide the Kafka topic based on the value of the attribute. #31809

Merged
merged 26 commits into from
May 3, 2024

Conversation

pyama86
Copy link
Contributor

@pyama86 pyama86 commented Mar 18, 2024

Description:
I've implemented the feature based on the discussion in the referenced issue. I'm assuming it will suffice in most cases if the text attributes are available, so I haven't planned for arrays or maps.

Link to tracking Issue:
Fixes #31178

Testing:
I've implemented unit tests. For the tests related to kafka_exporter, I followed the existing implementation, but I find it somewhat redundant. If it's okay, I'd like to switch to table-driven tests.

Documentation:

After the discussion in the PR, I believe the final implementation will be determined, so I'll add documentation once we have a clearer idea of the direction.

@pyama86 pyama86 changed the title Decide the Kafka topic based on the value of the attribute. [exporter/kafka]Decide the Kafka topic based on the value of the attribute. Mar 19, 2024
exporter/kafkaexporter/kafka_exporter.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/config.go Show resolved Hide resolved
exporter/kafkaexporter/kafka_exporter.go Outdated Show resolved Hide resolved
.chloggen/attribute-topic.yaml Outdated Show resolved Hide resolved
@pyama86
Copy link
Contributor Author

pyama86 commented Mar 31, 2024

@crobert-1
Thank you for your polite review. I have reflected on the points you made. I wanted to make the implementation of obtaining the first value of an attribute a bit more DRY, but since I couldn't find a suitable existing interface, I've temporarily addressed it with an anonymous function. If it's okay to create an interface, I will do so.

exporter/kafkaexporter/config.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/config.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/config.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/kafka_exporter.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/README.md Outdated Show resolved Hide resolved
exporter/kafkaexporter/config.go Outdated Show resolved Hide resolved
@crobert-1
Copy link
Member

LGTM, thanks for your making all of the updates, and for your contribution here @pyama86!

@pyama86
Copy link
Contributor Author

pyama86 commented Apr 9, 2024

@crobert-1
Thank you very much. I am also grateful for your dedicated review. Looking back, I feel I was too reserved about this repository, so I would like to proceed without hesitation, such as adding interfaces right from the start, next time.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 24, 2024
@crobert-1 crobert-1 added enhancement New feature or request and removed Stale labels Apr 24, 2024
@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Apr 29, 2024
@TylerHelmuth
Copy link
Member

@pyama86 looks like the linter is upset

kafkaexporter/kafka_exporter_test.go:376: File is not `gofmt`-ed with `-s` `-r 'interface{} -> any'` (gofmt)
		resource  interface{}
Error: kafkaexporter/kafka_exporter_test.go:430:22: G601: Implicit memory aliasing in for loop. (gosec)
				topic = getTopic(&tt.cfg, r)
				                 ^
Error: kafkaexporter/kafka_exporter_test.go:432:22: G601: Implicit memory aliasing in for loop. (gosec)
				topic = getTopic(&tt.cfg, r)
				                 ^
Error: kafkaexporter/kafka_exporter_test.go:434:22: G601: Implicit memory aliasing in for loop. (gosec)
				topic = getTopic(&tt.cfg, r)

@codeboten codeboten merged commit 45ef858 into open-telemetry:main May 3, 2024
162 checks passed
@github-actions github-actions bot added this to the next release milestone May 3, 2024
@pyama86 pyama86 deleted the attribute-topic branch May 4, 2024 00:57
@pyama86
Copy link
Contributor Author

pyama86 commented May 4, 2024

Thank you for your appreciation of our daily contributions and support to me.

rimitchell pushed a commit to rimitchell/opentelemetry-collector-contrib that referenced this pull request May 8, 2024
…ibute. (open-telemetry#31809)

I've implemented the feature based on the discussion in the referenced
issue. I'm assuming it will suffice in most cases if the text attributes
are available, so I haven't planned for arrays or maps.

Fixes open-telemetry#31178

I've implemented unit tests. For the tests related to kafka_exporter, I
followed the existing implementation, but I find it somewhat redundant.
If it's okay, I'd like to switch to table-driven tests.

---------

Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: Alex Boten <[email protected]>
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this pull request Jul 11, 2024
…ibute. (open-telemetry#31809)

I've implemented the feature based on the discussion in the referenced
issue. I'm assuming it will suffice in most cases if the text attributes
are available, so I haven't planned for arrays or maps.

Fixes open-telemetry#31178

I've implemented unit tests. For the tests related to kafka_exporter, I
followed the existing implementation, but I find it somewhat redundant.
If it's okay, I'd like to switch to table-driven tests.

---------

Co-authored-by: Curtis Robert <[email protected]>
Co-authored-by: Alex Boten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exporter/kafka ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

You want to dynamically determine the topic when exporting with Kafka Exporter.
5 participants