-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[exporter/kafka] Move Authentication Configuration Struct From Internal To Pkg #33180
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
Comments
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
@pavolloffay or @MovieStoreGuy could we get this issue resolved? I am willing to update my fork and merge it if we have some buy in that the PR gets picked up and merged. |
@vincentfree I've got a PR up (#33181) which resolves this issue, I'm trying to get the pipeline to pass so it may be merged as it already has the approvals. |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
Still looking to get some assistance on getting the PR in. Seems like the issues that are occurring in the pipeline checks are related to the timing of changes getting in as I explained here (#33181 (comment)). @MovieStoreGuy, @dmitryax, and @TylerHelmuth if any of you guys could shed some light on this that would be amazing 🙏 |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
I'm still waiting for this to be implemented. The implementation was ready, is there a blocker? |
@vincentfree the blocker was the fact that the pipeline wouldn't pass, I had reached out to some of the code owners for additional eyes but am waiting on that. I had taken a break from rebasing my PR (#33181). I just pushed a rebase to the PR but it is closed and it looks like I don't have the permissions to re-open it. @TylerHelmuth could you please reopen my PR? |
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
Is it only the config struct that you need access to? I've recently been refactoring the config structs and introduced https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/kafka/configkafka. I think it would be reasonable to move this to pkg/kafka/configkafka There are also some functions in internal/kafka for creating Sarama clients. I would prefer to not expose them if we don't need to, since I intend to propose replacing the use of Sarama with franz-go. |
Why would you change to franz? I did see integration with open telemetry in their libraries. Maybe it could improve the internal observability of the collector. |
@vincentfree that's definitely one pro. Aside from that, my team has done performance comparisons in the past and found franz-go to be superior. The API is nicer to work with. There are generally fewer bugs. Anyway, the proposal to switch will come with some supporting evidence. |
Uh oh!
There was an error while loading. Please reload this page.
Component(s)
exporter/kafka
Descrption
Users that are attempting to use the kafka exporter for a custom collector which requires authentication of any kind will encounter errors in doing so. This is because the
Authentication
field is referencing a struct that is in aninternal package
, see here.Desired Resolution
IMHO the desired resolution to this issue is to have
interanl/kafka
moved topkg/kafka
.Note: It seems as though this was discussed and decided that this should be done as per issue 30377, additionally there was a PR for these changes done by @vincentfree but it looks as though those changes were merged into a forked repository and not the original repository.
The text was updated successfully, but these errors were encountered: