change feature registry as a prototype to make test friendly

[yankay]

change feature registry as a prototype to make test friendly.

• change type registry to public
• create a public func "GetRegistry" to return the prototype
• remove the golbal function: List, IsEnabled, Apply
• change the unit test

Link to tracking Issue: #4917

Signed-off-by: Kay Yan [email protected]

[bogdandrutu]

To see the benefit of this change, we should split init into something like:

func init() {
	initWithRegistry(featuregate.GetRegistry())
}

func initWithRegistry(registry *featuregate.Registry) {
	registry.Register(featuregate.Gate{
		ID:          useOtelForInternalMetricsfeatureGateID,
		Description: "controls whether the collector to uses open telemetry for internal metrics",
		Enabled:     configtelemetry.UseOpenTelemetryForInternalMetrics,
	})
}

Then change all the tests to run initWithRegistry and test using a custom "Registry" per test. Does it make sense?

[yankay]

HI @bogdandrutu

We add a new func 'newRegistry()' to create object in test.
and add the func 'initWithRegistry' with unit test.

Would you please review it?

[github-actions]

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

[bogdandrutu]

Please rebase.

@open-telemetry/collector-approvers please review

[Aneurysm9]

Does this actually provide any advantage? If the Registry is to be exposed and the package-global accessor and mutator removed, then consumers should be provided with a Registry to use rather than using the global Registry. Setting the global Registry in tests is no different than setting and testing flags on the global Registry as was previously done.

If we are to provide a Registry for consumers to use we need to be very careful to ensure that it is available to all consumers that may need to test flag status at all points where flag status may need to be checked. It might make sense to expose it through the Host interface, but I don't believe that is available to factory instances when creating components. Putting it in *CreateSettings structs might make it available to component factories, but not to the factory initializers.

[bogdandrutu]

@Aneurysm9 consider please to read the PR and the issue associated.

This is mostly to help testing by having the possibility to design internally the funcs to accept a registry, and don't have to deal with globals in unit-tests.

func init() {
	initWithRegistry(featuregate.GetRegistry())
}

func initWithRegistry(registry *featuregate.Registry) {
	// ... do work
}

With this you can write unittests for initWithRegistry that do not change the global state, and importantly they can be run in parallel and guarantee to be isolated.

[Aneurysm9]

@Aneurysm9 consider please to read the PR and the issue associated.

I have read the PR and associated issue. Why would you assume that I hadn't done so before commenting on the implementation in the PR?

This is mostly to help testing by having the possibility to design internally the funcs to accept a registry, and don't have to deal with globals in unit-tests.

With this you can write unittests for initWithRegistry that do not change the global state, and importantly they can be run in parallel and guarantee to be isolated.

How does this help with testing? At most it allows testing of the init() implementation, which should be calling a function with a statically defined Gate struct. Testing that is not the interesting part. Testing the use of the gate is the interesting part and, without doing the sort of dependency injection that I mentioned above, that still depends on the global instance and is in no way improved by this PR. This comes at the cost of breaking the API for all consumers of the package.

[codecov]

Codecov Report

Merging #4969 (0c300b6) into main (f980c9e) will decrease coverage by 0.21%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #4969      +/-   ##
==========================================
- Coverage   90.99%   90.78%   -0.22%     
==========================================
  Files         178      182       +4     
  Lines       10632    10678      +46     
==========================================
+ Hits         9675     9694      +19     
- Misses        739      768      +29     
+ Partials      218      216       -2     

Impacted Files	Coverage Δ
service/zpages.go	70.33% <0.00%> (ø)
service/telemetry.go	63.82% <75.00%> (-12.43%)	⬇️
service/command.go	82.35% <100.00%> (ø)
service/featuregate/gates.go	100.00% <100.00%> (ø)
model/internal/pdata/traces.go	84.00% <0.00%> (-8.60%)	⬇️
model/internal/pdata/logs.go	88.00% <0.00%> (-8.30%)	⬇️
model/internal/pdata/metrics.go	95.74% <0.00%> (-2.18%)	⬇️
service/collector.go	77.10% <0.00%> (-0.34%)	⬇️
config/confighttp/confighttp.go	100.00% <0.00%> (ø)
... and 21 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 f980c9e...0c300b6. Read the comment docs.

[bogdandrutu]

@Aneurysm9

How does this help with testing? At most it allows testing of the init() implementation, which should be calling a function with a statically defined Gate struct.

If we are to provide a Registry for consumers to use we need to be very careful to ensure that it is available to all consumers that may need to test flag status at all points where flag status may need to be checked.

All these are making a wrong assumption that we need to "provide" the registry. I trust that engineers can design/refactor their code (similarly to the init example) in a way that the interesting functionality that use the "Gate"/"calls the registry" can be extracted into a separate func that accepts a registry as an argument. So then they can write unit tests. Same pattern by the way is used in the "go sdk for TracerProvider vs global", idea being you write the instrumentation plugins (code that uses the gates) to accept a "Registry" but if not provided then use the global.

[bogdandrutu]

@Aneurysm9 here is actually the continuation of this PR and how tests can be improved f1b821c (part of a cloned PR #5160)

[bogdandrutu]

Closing this, since we have the continuation #5160