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

Migrate kafkaexporter from Collector core to Collector contrib #4656

Merged
merged 8 commits into from
Aug 23, 2021

Conversation

Saber-W
Copy link
Member

@Saber-W Saber-W commented Aug 16, 2021

Description:
This PR migrates the kafkaexporter component from the Collector core repository to the Collector contrib repository.

This PR retains the commit history from core and includes plumbing fixes for go.mod, makefile, components.go.

Link to tracking Issue:
opentelemetry-collector/#3474
opentelemetry-collector/#3649

@Saber-W Saber-W requested review from a team and djaglowski August 16, 2021 19:22
@alolita alolita added release:required-for-ga Must be resolved before GA release spec:trace labels Aug 16, 2021
@alolita
Copy link
Member

alolita commented Aug 16, 2021

Hi @bogdandrutu @tigrannajaryan based on our discussion last week, we're starting to file PRs to move each core component to contrib. Please review and merge. Thanks!

@Saber-W Saber-W force-pushed the kafkaexporter-migration branch 2 times, most recently from 3ee8cf2 to 790d406 Compare August 17, 2021 00:26
@Saber-W Saber-W marked this pull request as draft August 17, 2021 00:27
@Saber-W Saber-W force-pushed the kafkaexporter-migration branch from 2209ae3 to a5edf06 Compare August 21, 2021 03:45
@Saber-W Saber-W marked this pull request as ready for review August 21, 2021 04:07
@Saber-W Saber-W force-pushed the kafkaexporter-migration branch from b1d3d1e to e5df631 Compare August 21, 2021 04:29
@bogdandrutu
Copy link
Member

Please make sure you use the new translator module that you created for opencensus/jaeger if needed instead of the one in collector.

@Saber-W Saber-W requested a review from Aneurysm9 August 21, 2021 23:19
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

I'd recommend writing a pinned issue describing this migration effort, what/why/how. In particular, I just noticed the approach of copying the opentelemetry-core repo into internal/core in advance to then start integrating, which is a neat trick. But documenting this for reviewer benefit and also to keep a record for future archaeology is important. There is #3474, if it's an appropriate issue for this, than I would suggest rewriting the first comment in the issue to include details and the linked issues, etc, presenting it for users. /cc @alolita

It's unfortunate that the commit message in c713647, which is what will show up in git blame for years to come, is basically empty - that one deserved a lot of detail. At the least though GitHub links it to the PR which links to #3474 so we can at least flesh that issue out.

@Aneurysm9 Aneurysm9 merged commit b824828 into open-telemetry:main Aug 23, 2021
hex1848 pushed a commit to hex1848/opentelemetry-collector-contrib that referenced this pull request Jun 2, 2022
…ry#4656)

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:required-for-ga Must be resolved before GA release spec:trace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants