Add govet with atomicalign

[pellared]

What

• Add atomicalign analyzer as an additional safety net against https://golang.org/pkg/sync/atomic/#pkg-note-BUG
• Execute lint workflow additionally with GOARCH=386

Remarks

1. atomicalign analyzer works only on 32bit arch: https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.3:go/analysis/passes/atomicalign/atomicalign.go;l=33-35. Here I checked that it works on x86: https://github.com/open-telemetry/opentelemetry-go/pull/2008/checks?check_run_id=2838943826
2. atomicalign only finds simple cases. We should not rely on it
3. In general most problems with atomic usage should be detected by ci / compatibility-test jobs (runtime panics)

[pellared]

Please [...], or add the "Skip Changelog" label if not required.

😉

[MrAlias]

atomicalign only finds simple cases. We should not rely on it
In general most problems with atomic usage should be detected by ci / compatibility-test jobs (runtime panics)

I'm not following this. The project added atomic alignment tests to avoid such panics at runtime. They were non-descriptive, not deterministically triggered, and caused a lot of issues.

Currently we have curated alignment tests. If we want to switch to this analyzer it should support more than just simple cases and be a replacement for the existing solution. If it is not, I do not agree that the switch is worth making.

[codecov]

Codecov Report

Merging #2008 (1d0d6b3) into main (80ca2b1) will increase coverage by 0.2%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main   #2008     +/-   ##
=======================================
+ Coverage   72.8%   73.0%   +0.2%     
=======================================
  Files        169     171      +2     
  Lines       8667    8687     +20     
=======================================
+ Hits        6311    6348     +37     
+ Misses      2084    2070     -14     
+ Partials     272     269      -3     

Impacted Files	Coverage Δ
exporters/jaeger/reconnecting_udp_client.go	89.6% <ø> (ø)
.../otlp/otlpmetric/internal/connection/connection.go	2.3% <ø> (ø)
...s/otlp/otlptrace/internal/connection/connection.go	2.3% <ø> (ø)
exporters/trace/jaeger/reconnecting_udp_client.go	89.6% <ø> (ø)
metric/metric_instrument.go	87.4% <ø> (ø)
metric/metrictest/meter.go	98.6% <ø> (ø)
sdk/metric/aggregator/aggregatortest/test.go	84.8% <ø> (ø)
sdk/metric/aggregator/lastvalue/lastvalue.go	87.0% <ø> (ø)
sdk/metric/aggregator/sum/sum.go	78.5% <ø> (ø)
sdk/metric/refcount_mapped.go	77.7% <ø> (ø)
... and 17 more

[pellared]

@MrAlias

I'm not following this. The project added atomic alignment tests to avoid such panics at runtime. They were non-descriptive, not deterministically triggered, and caused a lot of issues.

The panic at runtime can still happen. Here is an example: https://github.com/open-telemetry/opentelemetry-go/runs/2839025678. However, the panic is very descriptive. But still, I think that alignment tests are VERY valuable. As one may know that there could be a problem with an alignment test when looking at: https://github.com/open-telemetry/opentelemetry-go/runs/2839026086

Currently we have curated alignment tests. If we want to switch to this analyzer it should support more than just simple cases and be a replacement for the existing solution. If it is not, I do not agree that the switch is worth making.

I do not say that we should get rid of the alignment tests. I just think that we can use the static analyzer as an additional safety net.

The atomicalign MAY help prevent bugs if one forgets to write an alignment test. E.g. I think that this line misses an alignment test

opentelemetry-go/exporters/jaeger/reconnecting_udp_client.go

Line 31 in 39fe809

bufferBytes int64

Moreover, I think that the analyzer's output is far superior. See an example here: https://github.com/open-telemetry/opentelemetry-go/runs/2851467579 - it detected a misalignment for

opentelemetry-go/exporters/jaeger/reconnecting_udp_client.go

Line 31 in 39fe809

bufferBytes int64

- and here the test run panic was ugly.

After a second look, I see that it can detect complex cases. But still, it does not detect every possible use case as it can be seen in the previously mentioned GH workflow run: https://github.com/open-telemetry/opentelemetry-go/runs/2839026086

[pellared]

Please let me know if you do think that introducing the atomicalign analyzer is worth its value.

If yes, then I can update the branch and get rid of intentional bugs.

If not, then feel free to close the PR.

[MrAlias]

Thanks for opening this, but I do not see the necessary motivation to include these changes at this time. There appears an added benefit to including the analyzer as it provides dynamic inventory of the code and then validation. However, given we have a working solution to this and the included changes are not being submitted with the intent that and documentation of how this is a drop in replacement of the existing solution I do not want to include these changes at this point.

Please feel free to resubmit the changes if they can be verified to replace the existing solution and provide additional benefit to the project. Ideally, this would be documented and presented in a SIG meeting agenda item or an issue.