[exporter/kafka] move kafka configures authentication to internal pkg #27289

sakulali · 2023-10-01T17:52:07Z

Description:
Move kafka configures authentication to internal pkg, make references become clearer. Additionally, avoid to use export function kafkaexporter.ConfigureAuthentication to pass checkapi.

Link to tracking Issue:
#27093

Testing:
make gotidy
make golint
make crosslink
make multimod-verify
make gendependabot
make genotelcontribcol
make docker-otelcontribcol
go test for kafkaexporter
go test for kafkametricsreceiver
go test for kafkareceiver
go test for internal/kafka

Documentation:

.chloggen/chore-kafka-configures-authentication.yaml

fatsheep9146 · 2023-10-03T00:51:33Z

.github/dependabot.yml

@@ -1097,8 +1102,3 @@ updates:
    schedule:
      interval: "weekly"
      day: "wednesday"
-  - package-ecosystem: "gomod"
-    directory: "/receiver/solacereceiver"


Why this part is removed?

I did not deleted it purposely, it
seems make gendependabot do that?

I suggest you revert this unrelated change if possible

Thanks! I've no idea whether we need to revert this merged PR, as mentioned by @dmitryax below, there are different suggestions.

fatsheep9146 · 2023-10-03T01:00:40Z

exporter/kafkaexporter/config.go

@@ -103,7 +105,7 @@ func (cfg *Config) Validate() error {
 	return validateSASLConfig(cfg.Authentication.SASL)
 }

-func validateSASLConfig(c *SASLConfig) error {
+func validateSASLConfig(c *kafka.SASLConfig) error {


Speaking of validation, how about defining a common validation function for kafka.Authentication in internal/kafka package, which can be reused by kafkametricsreceiver, kafkareceiver and kafkaexporter.

It sounds great, it seems duplicate logic
in between validateSASLConfig and configureSASL, maybe we can defining a common validation function as you said :), and we can make a separate issue to follow up

could you help create an issue for this?

Sure! I am glad to do that, i will make it a few days later since without bringing laptop currently

could you help create an issue for this?

Sorry for delay, #27486

fatsheep9146 · 2023-10-09T01:33:57Z

@MovieStoreGuy @dmitryax could you help review about this pr?

dmitryax

LGTM. Thanks @sakulali!

dmitryax · 2023-10-09T05:03:01Z

I just realized I missed something important thing after merging the PR.

It definitely makes to remove the dependency on the exporter from other components and move the code to a common package, but I don't believe we can move configuration parts to internal package. Doing it, we are making it impossible to setup the kafka components programmatically, which I believe we should allow

cc @atoulme, I don't think checkapi tool should disallow exporting structs used in components configuration.

To properly reflect change in open-telemetry#27289

atoulme · 2023-10-09T06:37:59Z

Checkapi only checks for functions, not structs.

sakulali · 2023-10-09T07:36:47Z

I just realized I missed something important thing after merging the PR.

It definitely makes to remove the dependency on the exporter from other components and move the code to a common package, but I don't believe we can move configuration parts to internal package. Doing it, we are making it impossible to setup the kafka components programmatically, which I believe we should allow

cc @atoulme, I don't think checkapi tool should disallow exporting structs used in components configuration.

Thanks for the thorough review @dmitryax! If you believe that moving the configuration to the internal package is not a reasonable approach, should I revert the merged PR and consider placing the configuration in a different package or location? I think that if we place it in the pkg module, it can be used by other repositories as well.

To properly reflect change in #27289

dmitryax · 2023-10-09T16:28:00Z

Should I revert the merged PR and consider placing the configuration in a different package or location?

No, I think it's not that big of an issue. I see we have other components with config options in the internal package, e.g. k8s ones. Let's see if someone complains after this change :)

I think that if we place it in the pkg module, it can be used by other repositories as well.

Right, pkg would be probably the best location, but we need to export only config structs not helpers like ConfigureAuthentication. So probably we'll have to be split it into two packages.

I'd suggest we handle this generically with other components like kubestats. Probably we should have pkg/config/k8s and pkg/config/kafka. Let's start with an issue and continue discussion there? Maybe it's not something that other folks see importance in having.

…open-telemetry#27289) **Description:** Move kafka configures authentication to internal pkg, make references become clearer. Additionally, avoid to use export function `kafkaexporter.ConfigureAuthentication` to pass checkapi. **Link to tracking Issue:** open-telemetry#27093

To properly reflect change in open-telemetry#27289

vincentfree · 2024-01-09T19:26:42Z

Should I revert the merged PR and consider placing the configuration in a different package or location?

No, I think it's not that big of an issue. I see we have other components with config options in the internal package, e.g. k8s ones. Let's see if someone complains after this change :)

I think that if we place it in the pkg module, it can be used by other repositories as well.

Right, pkg would be probably the best location, but we need to export only config structs not helpers like ConfigureAuthentication. So probably we'll have to be split it into two packages.

I'd suggest we handle this generically with other components like kubestats. Probably we should have pkg/config/k8s and pkg/config/kafka. Let's start with an issue and continue discussion there? Maybe it's not something that other folks see importance in having.

I was just writing an issue about this, I'm trying to simplify a project that I've made by using structs from the collector project directly instead of having the serialize and make my own struct replicating what the project has already created. I'm making something that simplifies the process of configuring the collector for the company I work for, I want to do prefills and have the option for either grpc or kafka. With the core structs for grpc I can just use all for the config structs without any issues which helps me reduce complexity.

With the kafka implementation I find that the Authentication field breaks due to it being in a internal folder. Thus im either stuck at an older version or have to revert to my own struct that implements the same fields as the struct im trying to use from receiver/kafka.

Could we revisit this and move the code to a pkg/ folder instead of internal?
For me it doesn't make a lot of sense hiding one field in a struct making it unusable for others to use in downstream applications.

wicklander-bryant · 2024-05-21T22:06:09Z

I just realized I missed something important thing after merging the PR.
It definitely makes to remove the dependency on the exporter from other components and move the code to a common package, but I don't believe we can move configuration parts to internal package. Doing it, we are making it impossible to setup the kafka components programmatically, which I believe we should allow
cc @atoulme, I don't think checkapi tool should disallow exporting structs used in components configuration.

Thanks for the thorough review @dmitryax! If you believe that moving the configuration to the internal package is not a reasonable approach, should I revert the merged PR and consider placing the configuration in a different package or location? I think that if we place it in the pkg module, it can be used by other repositories as well.

I am encountering this very issue. I'm attempting to use the kafka exporter for a custom collector that I'm building and I want to export to AWS MSK, only issue is that I this requires token authentication which I can't configure because the Authentication field is referencing a struct that is in an internal package, see here.

IMHO I think it makes sense to have this authentication logic in /pkg or somewhere that exports it publicly since the publicly exported kafkaexporter config struct references it.

sakulali requested a review from MovieStoreGuy as a code owner October 1, 2023 17:52

sakulali requested a review from a team October 1, 2023 17:52

sakulali requested review from dmitryax and mx-psi as code owners October 1, 2023 17:52

github-actions bot assigned fatsheep9146 Oct 1, 2023

github-actions bot added cmd/configschema configschema command cmd/otelcontribcol otelcontribcol command exporter/kafka receiver/kafkametrics receiver/kafka labels Oct 1, 2023

github-actions bot requested review from pavolloffay and pmcollins October 1, 2023 17:52

fatsheep9146 reviewed Oct 2, 2023

View reviewed changes

.chloggen/chore-kafka-configures-authentication.yaml Outdated Show resolved Hide resolved

sakulali force-pushed the chore-kafka-config-auth branch from 19b488c to 03255c4 Compare October 2, 2023 12:08

fatsheep9146 reviewed Oct 3, 2023

View reviewed changes

sakulali force-pushed the chore-kafka-config-auth branch from 03255c4 to 7e7e45f Compare October 8, 2023 04:20

[exporter/kafka] move kafka configures authentication to internal pkg

f9fb48b

sakulali force-pushed the chore-kafka-config-auth branch from c965d6b to f9fb48b Compare October 8, 2023 15:04

dmitryax approved these changes Oct 9, 2023

View reviewed changes

dmitryax merged commit 96d53a2 into open-telemetry:main Oct 9, 2023
82 checks passed

github-actions bot added this to the next release milestone Oct 9, 2023

dmitryax added a commit to dmitryax/opentelemetry-collector-contrib that referenced this pull request Oct 9, 2023

[chore] Fix changelog entry for kafka components

f838247

To properly reflect change in open-telemetry#27289

dmitryax mentioned this pull request Oct 9, 2023

[chore] Fix changelog entry for kafka components #27479

Merged

sakulali mentioned this pull request Oct 9, 2023

[internal/kafka] define a common validation function for kafka authentication #27486

Closed

djaglowski pushed a commit that referenced this pull request Oct 9, 2023

[chore] Fix changelog entry for kafka components (#27479)

c25279d

To properly reflect change in #27289

sakulali deleted the chore-kafka-config-auth branch November 2, 2023 13:58

jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this pull request Nov 12, 2023

[chore] Fix changelog entry for kafka components (open-telemetry#27479)

7593343

To properly reflect change in open-telemetry#27289

vincentfree mentioned this pull request Jan 9, 2024

[receiver/kafka] Move Authentication configuration struct from internal to pkg/ #30377

Closed

This was referenced May 21, 2024

Moves internal/kafka To pkg/kafka #33177

Closed

Moves internal/kafka To pkg/kafka #33178

Closed

Moves internal/kafka To pkg/kafka wicklander-bryant/opentelemetry-collector-contrib#1

Merged

chore: Moves internal/kafka To pkg/kafka #33181

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/kafka] move kafka configures authentication to internal pkg #27289

[exporter/kafka] move kafka configures authentication to internal pkg #27289

sakulali commented Oct 1, 2023 •

edited

Loading

fatsheep9146 Oct 3, 2023

sakulali Oct 3, 2023

fatsheep9146 Oct 9, 2023 •

edited

Loading

sakulali Oct 9, 2023

fatsheep9146 Oct 3, 2023

sakulali Oct 3, 2023

fatsheep9146 Oct 5, 2023

sakulali Oct 5, 2023

sakulali Oct 9, 2023

fatsheep9146 commented Oct 9, 2023

dmitryax left a comment

dmitryax commented Oct 9, 2023 •

edited

Loading

atoulme commented Oct 9, 2023

sakulali commented Oct 9, 2023

dmitryax commented Oct 9, 2023 •

edited

Loading

vincentfree commented Jan 9, 2024

wicklander-bryant commented May 21, 2024

[exporter/kafka] move kafka configures authentication to internal pkg #27289

[exporter/kafka] move kafka configures authentication to internal pkg #27289

Conversation

sakulali commented Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatsheep9146 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatsheep9146 commented Oct 9, 2023

dmitryax left a comment

Choose a reason for hiding this comment

dmitryax commented Oct 9, 2023 • edited Loading

atoulme commented Oct 9, 2023

sakulali commented Oct 9, 2023

dmitryax commented Oct 9, 2023 • edited Loading

vincentfree commented Jan 9, 2024

wicklander-bryant commented May 21, 2024

sakulali commented Oct 1, 2023 •

edited

Loading

fatsheep9146 Oct 9, 2023 •

edited

Loading

dmitryax commented Oct 9, 2023 •

edited

Loading

dmitryax commented Oct 9, 2023 •

edited

Loading