Move initialization of component factories after flag parsing

[dashpole]

Description:

Fixes #4967

Instead of assembling factories before creating and executing the cobra service, construct factories within the cobra run command so it happens after feature flag parsing.

Testing:

Manually tested by adding a feature gate to the OTLP receiver, and checking its value during the NewFactory() call.

[codecov]

Codecov Report

Merging #4977 (1271f24) into main (699a81b) will increase coverage by 0.89%.
The diff coverage is 66.66%.

❗ Current head 1271f24 differs from pull request most recent head e3b5a66. Consider uploading reports for the commit e3b5a66 to get more accurate results

@@            Coverage Diff             @@
##             main    #4977      +/-   ##
==========================================
+ Coverage   89.96%   90.86%   +0.89%     
==========================================
  Files         183      181       -2     
  Lines       11104    10640     -464     
==========================================
- Hits         9990     9668     -322     
+ Misses        889      753     -136     
+ Partials      225      219       -6     

Impacted Files	Coverage Δ
service/collector.go	76.60% <66.66%> (-0.51%)	⬇️
model/otlp/json_unmarshaler.go	76.92% <0.00%> (-2.39%)	⬇️
service/flags.go	86.36% <0.00%> (-2.10%)	⬇️
model/internal/pdata/generated_metrics.go	97.14% <0.00%> (-0.07%)	⬇️
service/config_provider.go	94.17% <0.00%> (-0.06%)	⬇️
service/command.go	82.35% <0.00%> (ø)
service/featuregate/flags.go	100.00% <0.00%> (ø)
config/confighttp/confighttp.go	100.00% <0.00%> (ø)
exporter/exporterhelper/logs.go	84.74% <0.00%> (ø)
exporter/exporterhelper/common.go	94.25% <0.00%> (ø)
... and 47 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 699a81b...e3b5a66. Read the comment docs.

[bogdandrutu]

I understand the problem, and the solution is nice. I want to propose for thinking another alternative, can we only keep as our public API the Collector and let the generator deal with flags etc.? I think that way we don't have to change settings. The reason to keep only that is to separate concerns collector is a lib and builder creates the main func.

[dashpole]

The issue i've run into is that flag parsing is done as part of cmd.Execute() on the cobra command.

Currently, component.Factories is an input to service.CollectorSettings, which is an input to service.NewCommand(). We call cmd.Execute on the command returned by service.NewCommand(), which calls command.ParseFlags.

So as long as completed factories is an input to the command, we will not have parsed flags prior to instantiating factories.

I think options are:

1. Switch to instantiating factories within the command (this PR, or something similar).
2. Don't use cobra for the feature gate flag, and just parse it prior to instantiating factories. We lose the flag-related benefits of cobra, and (may?) have similar issues with other flags, if the values are referenced during factory creation.
3. Stop using cobra all-together (not a real option, just for completeness)
4. Leave it as-is, and document that feature-gates should not be used from within NewFactory() calls.

[dashpole]

Note: it looks like I would need to make the API change in one PR, then update the builder, and then switch to the new API in a second PR here.

[bogdandrutu]

Don't use cobra for the feature gate flag, and just parse it prior to instantiating factories. We lose the flag-related benefits of cobra, and (may?) have similar issues with other flags, if the values are referenced during factory creation.

I still believe that removing "NewCommand" (this is a simple func anyway) and "NewWindowsService" from our public "offering" and let people use the builder to create the main func is a valid option. I am on PTO and will look into this more when back on 16th.

[dashpole]

I was able to implement your suggestion above by moving NewService to the builder. One issue is that the builder integration tests pin a version of the collector (currently 0.38.0). Since this changes some functions in service (GetConfigFlag(), GetSetFlag(), Flags()), we will need to make those changes, cut a release, and then merge the rest of this PR.

[dashpole]

If the changes here look reasonable, i'll open a separate PR to make the changes to service flag functions

[bogdandrutu]

@dashpole for me this looks right. I think as part of generating the "cmd" (via the builder) directory we should generate the NewCommand as well as the WindowsHandler. I would be curious about what others think? A different option is to move these to a different package than the current "service" and maybe consider to use different Settings definition where we can have a "provider" for factories as your initial example.

[dashpole]

I think the approach here is simpler, and has less of an API surface than doing the latter. I'll open the change to the public interfaces.

[dashpole]

Windows probably doesn't need to Reimplement the entire NewCommand. It can just perform flag parsing from the builder.

[dashpole]

This will continue to fail until it can be rebased on #5130

[github-actions]

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

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.