open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.43k stars 1.46k forks source link

Move implementation checks to test files #9431

Open atoulme opened 9 months ago

atoulme commented 9 months ago

We use implementation checks to make sure an interface is fulfilled by a struct. However, those checks are sometimes made in the code we ship, which introduces dependencies in the code base. Instead, consider moving those to test files.

Additionally, we should add this as a best practice.

yurishkuro commented 9 months ago

I don't think this is a useful exercise. Noop type checks are removed by the compiler, so there's no runtime impact. But when you browse the code it's much more useful to see implemented interfaces declared next to the struct, not in some other place.

atoulme commented 9 months ago

Initial context is from https://github.com/open-telemetry/opentelemetry-collector/pull/9427#pullrequestreview-1851884365

Not necessary impacting this, but would like all the implementation checks to be moved to tests (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configopaque/opaque.go#L17) so that the linker does unnecessary bring that dependency to non-test code if unnecessary. In this case is a system func so not that worry, but want to set a pattern.

yurishkuro commented 9 months ago

If a type implements an interface, it's highly unlikely that it will be used in the context where that interface is not already linked, so the argument seems pretty weak to me.

atoulme commented 7 months ago

I have added the discussion-needed label and will bring it to a SIG meeting. I don't want to do work needlessly.