Fix featuregate late initialization #8478

frzifus · 2023-09-16T03:54:21Z

Description:
The factories for the components are created before the CLI flags are evaluated. Therefore, featuregate flags are ignored during the early startup phase.
~~This PR simply shifts the time of creation of the factory map after the CLI flag evaluation.~~

Link to tracking Issue:
Closes #4967

Testing:
Short manual testing via cli. Adding the following line in cobra.Command.RunE.

featuregate.GlobalRegistry().VisitAll(func(fg *featuregate.Gate) { fmt.Println(fg.ID(), fg.IsEnabled()) })

Changing + & -:

./bin/otelcorecol_linux_amd64 --config=config.yaml --feature-gates=+telemetry.disableHighCardinalityMetrics

codecov · 2023-09-16T04:01:53Z

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
cmd/otelcorecol/main.go	`0.00% <0.00%> (ø)`
otelcol/command_components.go	`92.10% <89.28%> (-3.73%)`	⬇️
otelcol/collector.go	`77.50% <60.00%> (-2.77%)`	⬇️

📢 Thoughts on this report? Let us know!.

otelcol/command.go

codeboten · 2023-09-19T22:43:38Z

cmd/otelcorecol/main.go

@@ -11,8 +11,7 @@ import (
 )

 func main() {
-	factories, err := components()
-	if err != nil {
+	if _, err := components(); err != nil {


With components() called twice, it's possible the second call will swap out a component the second time around if a feature gate is set and end up error'ing differently at line 25 right? Would it be possible to move the initialization of the feature gates up instead of calling this twice?

With components() called twice, it's possible the second call will swap out a component the second time around if a feature gate is set and end up error'ing differently at line 25 right?

I did not consider that. But yes, that might be the case. Making the first call somehow useless.

Would it be possible to move the initialization of the feature gates up instead of calling this twice?

Defiantly, that is what I tried first. Any recommendations how to do that? What I had in mind was creating another root command and parse only the featuregate flags. This only worked, when I ignored the config flag using spf13/cobra#662.

But since I moved the featureflag evaluation out, the original root command started to fail.

frzifus · 2023-09-20T07:25:51Z

otelcol/command.go

@@ -12,6 +12,14 @@ import (
 	"go.opentelemetry.io/collector/featuregate"
 )

+// NewCommandFeatureGate constructs a new cobra.Command used to parse given FeatureGates.
+func NewCommandFeatureGate() *cobra.Command {
+	flagSet := flags(featuregate.GlobalRegistry())


I guess here we could add something like flagFeatureGate() instead and add FParseErrWhitelist: cobra.FParseErrWhitelist{UnknownFlags: false} to the root command and add a No-op FeatureGate flag to the actual root command? Or simply parse them twice. Wdyt?

frzifus · 2023-09-26T11:30:36Z

Ping @codeboten @jpkrohling

bogdandrutu

This seem to me as a big hack, I would prefer the version that I think @dashpole proposed, to provide a func that returns the factories to the settings instead.

But still not sure what factories need this to justify this change. I understand that it is a problem/limitation but I don't understand why is important.

dashpole · 2023-09-27T18:02:47Z

But still not sure what factories need this to justify this change. I understand that it is a problem/limitation but I don't understand why is important.

My memory is fuzzy, but from what I recall, I wanted to use a feature gate for a refactor that entirely replaced the factory implementation. The natural place to do that was in NewFactory(), but it took a long time to realize that the feature gate wasn't being respected. The eventual workaround: open-telemetry/opentelemetry-collector-contrib#9116. I would be satisfied if there was an easy way to detect that case.

It is also definitely possible it isn't worth the effort if no one else has encountered this case. We could just document this shortcoming, and close the issue if that is our conclusion.

frzifus · 2023-09-28T09:30:12Z

This seem to me as a big hack, I would prefer the version that I think @dashpole proposed, to provide a func that returns the factories to the settings instead.

@bogdandrutu I had chosen this way, because the first approach to pass a constructor func through changes the signature of the otelcon.NewCommand. This means that new collector versions can only be built with the upcoming builder version. (Was mentioned by @codeboten #8478 (comment))

But still not sure what factories need this to justify this change. I understand that it is a problem/limitation but I don't understand why is important.

From my point of view, it is somewhat misleading that the feature gates in the factory do not work. I have run into the same problems as @dashpole. We also wanted to use a feature gate for an entirely replaced factory implementation. (Jaeger Remote Sampling and Jaeger Receiver)

--

I think some kind of tradeoff has to be made. Leaving it as it is and documenting it is already one. It seems like other alternatives are a lot of refactor work, breaking downward compatibility or double parsing the feature gate flags.

frzifus · 2023-10-05T13:13:46Z

@bogdandrutu based on yesterday's conversation, does it match the concept?

I checked the initialization of featuregates and found none inside a factory.

.chloggen/fix_featuregate_late_initialization.yaml

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Co-authored-by: Ben B. <bongartz@klimlive.de>

Signed-off-by: Alex Boten <aboten@lightstep.com>

…ectorSettings open-telemetry/opentelemetry-collector#8478

* feat(processor/remoteobserver)!: rename `remoteobserver` processor to `remotetap` * chore: update otel core to `v0.89.0` * fix(otelcolbuilder): adjust after API breaking change in otelcol.CollectorSettings open-telemetry/opentelemetry-collector#8478

frzifus force-pushed the fix_featuregate_late_initialization branch from ebdc19f to a2ece5b Compare September 16, 2023 03:57

frzifus force-pushed the fix_featuregate_late_initialization branch from a2ece5b to 16e1027 Compare September 16, 2023 04:04

frzifus marked this pull request as ready for review September 16, 2023 08:45

frzifus requested review from a team and jpkrohling September 16, 2023 08:45

codeboten reviewed Sep 19, 2023

View reviewed changes

otelcol/command.go Outdated Show resolved Hide resolved

codeboten reviewed Sep 19, 2023

View reviewed changes

frzifus force-pushed the fix_featuregate_late_initialization branch from 16e1027 to 5436454 Compare September 20, 2023 07:22

frzifus commented Sep 20, 2023

View reviewed changes

frzifus requested a review from codeboten September 20, 2023 07:26

frzifus force-pushed the fix_featuregate_late_initialization branch 2 times, most recently from 7788615 to 45ef1ad Compare September 20, 2023 07:40

frzifus mentioned this pull request Sep 21, 2023

[jaegerreceiver,jaegerremotesampling]: thrift-gen, proto-gen featuregate does not work open-telemetry/opentelemetry-collector-contrib#27046

Closed

jpkrohling approved these changes Sep 27, 2023

View reviewed changes

bogdandrutu requested changes Sep 27, 2023

View reviewed changes

frzifus force-pushed the fix_featuregate_late_initialization branch 4 times, most recently from 2279213 to 81a0b34 Compare October 5, 2023 07:55

frzifus requested a review from bogdandrutu October 5, 2023 13:11

sfc-gh-bdrutu approved these changes Oct 19, 2023

View reviewed changes

bogdandrutu approved these changes Oct 19, 2023

View reviewed changes

.chloggen/fix_featuregate_late_initialization.yaml Show resolved Hide resolved

frzifus mentioned this pull request Nov 3, 2023

[receiver/jaeger] mark featuregates as stable open-telemetry/opentelemetry-collector-contrib#27636

Merged

codeboten approved these changes Nov 3, 2023

View reviewed changes

builder: start the collector with the parsing of featuregates

741c013

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus and others added 6 commits November 3, 2023 11:31

otelcorecol: regenerate

9c5f741

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

otelcol: only validate featuregate flag at before creating factories

2b78b3d

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

otelcol: carry factory func instead of object in settings

44a37a2

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Update .chloggen/fix_featuregate_late_initialization.yaml

de6f46d

Co-authored-by: Ben B. <bongartz@klimlive.de>

update changelog

b84985f

Signed-off-by: Alex Boten <aboten@lightstep.com>

fix test after rebase

593700b

Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten force-pushed the fix_featuregate_late_initialization branch from c6d085c to 593700b Compare November 3, 2023 18:32

codeboten mentioned this pull request Nov 3, 2023

[pkg/translator/jaeger] Bump github.com/jaegertracing/jaeger from 1.41.0 to 1.49.0 broke tests open-telemetry/opentelemetry-collector-contrib#26567

Closed

codeboten merged commit ecd837e into open-telemetry:main Nov 3, 2023
31 of 32 checks passed

github-actions bot added this to the next release milestone Nov 3, 2023

frzifus deleted the fix_featuregate_late_initialization branch November 4, 2023 07:57

andrzej-stencel added a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Nov 16, 2023

fix(otelcolbuilder): adjust after API breaking change in otelcol.Coll…

ddf71cb

…ectorSettings open-telemetry/opentelemetry-collector#8478

andrzej-stencel added a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Nov 17, 2023

fix(otelcolbuilder): adjust after API breaking change in otelcol.Coll…

f97cbea

…ectorSettings open-telemetry/opentelemetry-collector#8478

andrzej-stencel added a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Nov 17, 2023

fix(otelcolbuilder): adjust after API breaking change in otelcol.Coll…

afe854b

…ectorSettings open-telemetry/opentelemetry-collector#8478

mx-psi mentioned this pull request Nov 30, 2023

[cmd/builder] Provide better error or have main.go template be conditional on otelcol_version #9019

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix featuregate late initialization #8478

Fix featuregate late initialization #8478

frzifus commented Sep 16, 2023 •

edited

Loading

codecov bot commented Sep 16, 2023 •

edited

Loading

codeboten Sep 19, 2023

frzifus Sep 20, 2023

frzifus Sep 20, 2023

frzifus commented Sep 26, 2023

bogdandrutu left a comment

dashpole commented Sep 27, 2023

frzifus commented Sep 28, 2023 •

edited

Loading

frzifus commented Oct 5, 2023

Fix featuregate late initialization #8478

Fix featuregate late initialization #8478

Conversation

frzifus commented Sep 16, 2023 • edited Loading

codecov bot commented Sep 16, 2023 • edited Loading

Codecov Report

codeboten Sep 19, 2023

Choose a reason for hiding this comment

frzifus Sep 20, 2023

Choose a reason for hiding this comment

frzifus Sep 20, 2023

Choose a reason for hiding this comment

frzifus commented Sep 26, 2023

bogdandrutu left a comment

Choose a reason for hiding this comment

dashpole commented Sep 27, 2023

frzifus commented Sep 28, 2023 • edited Loading

frzifus commented Oct 5, 2023

frzifus commented Sep 16, 2023 •

edited

Loading

codecov bot commented Sep 16, 2023 •

edited

Loading

frzifus commented Sep 28, 2023 •

edited

Loading