Enable all disabled checks options in golangci-lint, see #2786

[bogdandrutu]

In #2786 we started to enable some of the disabled checks in golangci-lint, but not all of them are enabled because of the amount of work required.

This is a tracking issue to ensure all are enabled.

[mx-psi]

If this is up for grabs I can work on this, remaining checks to enable that I can see seem to be

• go vet fieldalignment check
• errcheck?
• EXC0002
• EXC0007

[bogdandrutu]

In terms of priority I would start exactly in the opposite order that you listed :)

[bogdandrutu]

Downgrade the priority for this, we can do it in phase 3

[mx-psi]

I tried enabling fieldalignment, I see a few issues:

1. fieldalignment automatic fixes are broken in several ways, which makes it hard to fix things everywhere:
   • the fix option doesn't handle unkeyed struct literals when reordering fields, so the code after a fix can be invalid (even worse, it can be silently invalid if it reordered two fields of the same type),
   • the fix option removes doc comments for fields (some of this can be seen at the test I did at mx-psi/opentelemetry-collector@1950238),
   • for some reason the fix option does not fix everything in a single-pass. This is not necessarily bad but I think it is weird and reflects that this tool is not frequently used and thus is more buggy.
2. Applying this everywhere sacrifices some readability, since reordering the fields means we can't use spacing to convey relations between fields (e.g. we can't do this

   opentelemetry-collector/service/service.go

   Lines 31 to 44 in bdcb989

   	type service struct {
   	factories component.Factories
   	buildInfo component.BuildInfo
   	config *config.Config
   	telemetry component.TelemetrySettings
   	zPagesSpanProcessor *zpages.SpanProcessor
   	asyncErrorChannel chan error
   	builtExporters builder.Exporters
   	builtReceivers builder.Receivers
   	builtPipelines builder.BuiltPipelines
   	builtExtensions extensions.Extensions
   	}

   ) to separate fields. Sometimes field ordering itself is also an aid to readability which we would also lose.
3. The gains are usually not very big since we use most structs only a few times.

My take from this is that if we enable this we should only enable it for specific modules/packages that are interesting because we expect their structs to be used a lot of times (so, for example, it may be interesting to enable this just for pdata or the model module).

[atoulme]

I have tried to renew the experiment from @mx-psi with #9133 and have the same results. Most of the code is not in the hot path.

The fieldalignment linter picked up interesting improvements on the protobuf code, however, and this might be something to pursue down the road in the opentelemetry-proto project.

I have reviewed and tried to apply this to different parts of the code, but I don't see a good target for improvement because none of the code is in the hot path. I identified that the generated protobuf code could use a boost. Unfortunately protobuf values the ordering of fields.

Traces:

/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/trace/v1/trace.pb.go:179:20: struct with 88 pointer bytes could be 72
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/trace/v1/trace.pb.go:253:17: struct with 96 pointer bytes could be 80
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/trace/v1/trace.pb.go:321:11: struct of size 208 could be 192
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/trace/v1/trace.pb.go:525:17: struct with 32 pointer bytes could be 24
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/trace/v1/trace.pb.go:605:16: struct with 48 pointer bytes could be 24

Metrics:

/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:219:22: struct with 88 pointer bytes could be 72
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:293:19: struct with 96 pointer bytes could be 80
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:445:13: struct with 64 pointer bytes could be 56
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:883:22: struct with 64 pointer bytes could be 48
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:1038:25: struct with 176 pointer bytes could be 128
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:1283:36: struct of size 216 could be 208
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:1561:44: struct with 16 pointer bytes could be 8
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:1627:23: struct with 64 pointer bytes could be 32
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/metrics/v1/metrics.pb.go:1814:15: struct with 48 pointer bytes could be 24

Logs:

/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/logs/v1/logs.pb.go:221:19: struct with 88 pointer bytes could be 72
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/logs/v1/logs.pb.go:295:16: struct with 96 pointer bytes could be 80
/Users/atoulme/w/opentelemetry-collector/pdata/internal/data/protogen/logs/v1/logs.pb.go:362:16: struct with 64 pointer bytes could be 40

In the meantime, I move that we remove the TODO in the .golangci-lint.yaml file and close the task to revisit the fieldalignment linter.

[atoulme]

#9174 is open to follow up.