Upgrade go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp
#4507
Conversation
00b6c6e to
9f56e84
Compare
|
Please sign your commit as per the contribution guidelines |
|
Verified locally: image builds, spam in logs gone, traces are exported to a collector correctly |
Signed-off-by: krynju <krystian.gulinski@juliahub.com>
| go 1.22.0 | ||
| go 1.22.7 | ||
|
|
||
| toolchain go1.23.2 |
There was a problem hiding this comment.
I think we should stick with 1.22 toolchain for now.
There was a problem hiding this comment.
This is due to the version in this Dockerfile. Should I downgrade it? This is already there on main, not sure how it passed CI (1.23 forces this toolchain entry and causes a diff)
https://github.com/distribution/distribution/blob/main/dockerfiles%2Fvendor.Dockerfile#L3
There was a problem hiding this comment.
The CI works thanks to this
distribution/.github/workflows/build.yml
Line 30 in 3ddd142
Which uses the matrix Go versions:
distribution/.github/workflows/build.yml
Lines 35 to 37 in 3ddd142
There was a problem hiding this comment.
When I had this toolchain line removed make validate-vendor failed in the validate run and I pushed the actual output of make vendor instead.
For make validate-vendor the toolchain version is dictated by that Dockerfile I linked above. I can downgrade it to 1.22 in the dockerfile if needed, but that generates:
go 1.22.7
toolchain go1.22.9
I suppose it changes it to 1.22.7 as the lower bound set by go.opentelemetry.io/contrib/exporters/autoexport
Let me know which one you preffer:
- dockerfile with 1.23 +
go 1.22.7; toolchain go1.23.2(current) - dockerfile with 1.22 +
go 1.22.7; toolchain go1.22.9(need to change vendor.Dockerfile)
There was a problem hiding this comment.
When I had this toolchain line removed make validate-vendor failed in the validate run and I pushed the actual output of make vendor instead.
ok, this is interesting, because as it is now (i.e. on the latest commit), make validate-vendor succeeds, which makes me think - and I'm speculating here - the otel modules are causing some new sadness (not for the first time 🙃 ) in this codebase; maybe they require 1.23 toolchain? 🫠
For make validate-vendor the toolchain version is dictated by that Dockerfile I linked above. I can downgrade it to 1.22 in the dockerfile if needed, but that generates:
Why does this require patch release number? Doesn't specifying minor only use the latest patch release e.g.1.22?
There was a problem hiding this comment.
tracked it down and grpc introduced 1.22.7 lower bound here grpc/grpc-go@393fbc3
then autoexport got it here open-telemetry/opentelemetry-go-contrib@bb01131
go 1.22.7 means it's the lower bound https://go.dev/ref/mod#go-mod-file-go:~:text=The%20go%20directive%20sets%20the%20minimum%20version%20of%20Go%20required%20to%20use%20this%20module
toolchain just notes whatever go version was used to resolve the go.mod/go.sum https://go.dev/ref/mod#go-mod-file-go:~:text=For%20reproducibility%2C%20the%20go%20command%20writes%20its%20own%20toolchain%20name%20in%20a%20toolchain%20line%20any%20time%20it%20is%20updating%20the%20go%20version%20in%20the%20go.mod%20file%20(usually%20during%20go%20get).
We can control the toolchain directive just by changing vendor.Dockerfile to ARG GO_VERSION=1.22 and it will show up as toolchain go1.22.9 or if you use ARG GO_VERSION=1.22.7 it will be gone (as it's the same as the go directive)
There was a problem hiding this comment.
Aha! Yeah it's starting to make more sense now. I think. The toolchain IIRC is not mandatory for all modules but if one of the deps requires a specific patch version it gets inserted into go.mod to met build constraints, which I think is what what we're seeing here 🤔
There was a problem hiding this comment.
I set the vendor.Dockerfile to 1.22.7 and the toolchain directive is gone. Go is still at 1.22.7 due to that minimum set by the grpc package.
Should be good to go once CI passes. make validate-vendor passes locally"
commit b6981b8
There was a problem hiding this comment.
I say, let's go with this option:
dockerfile with 1.23 + go 1.22.7; toolchain go1.23.2 (current)
Mostly due to what I said in #4507 (comment)
I appreciate that was your original commit, and I'm sorry I didn't think about it properly at the time. Thanks for clarifying it.
Once you've made the changes, mind squashing your commits, please. Then we're good to got.
There was a problem hiding this comment.
Dropped b6981b8
It's back to the original commit, so good to go
dockerfiles/vendor.Dockerfile
Outdated
| @@ -1,6 +1,6 @@ | |||
| # syntax=docker/dockerfile:1< 8000 /span> | |||
|
|
|||
| ARG GO_VERSION=1.23.2 | |||
| ARG GO_VERSION=1.22.7 | |||
There was a problem hiding this comment.
I am wondering if sticking to 1.23.2 across all Dockerfiles makes things more maintainable...I'm leaning towards that rather than having this specific Dockerfile Go version be set to a different version than the other ones. I get that means having the toolchain stanza in the go.mod but I think that'll probably be the most "sane" solution for the maintainers 🤔 any thoughts on this @Jamstah @thaJeztah ?
There was a problem hiding this comment.
Yeah, not sure what the solution is to that. It's... weird. Basically says; "you can build this code with go1.22, but you must use the go1.23 version of go1.22 to use it?". The patch versions make even less sense, but I guess those are forces upon us by upstreams 😞
|
Will there be a new release including this fix? We cannot use v3.0.0-rc.1 at the moment as it is affected by the issue this PR is fixing. As described in #4507 (comment), there is a huge log spam. |
|
we can cut |
|
Hi @milosgajdos , Can we now cut the |
|
@ialidzhikov see: https://github.com/distribution/distribution/releases/tag/v3.0.0-rc.2 Official image pending review (outta our hands): docker-library/official-images#18137 The images are available from GH registry and distribution/distribution on Docker Hub |
|
Thank you very much, @milosgajdos ! |
|
@milosgajdos , linux/amd64 image is not pushed. See distribution/distribution-library-image#181. |
|
Yeah, we noticed that this morning; the DOI PR was merged late night yesterday. Unfortunately those are out of our hands |
In v0.55.0 this issue is fixed open-telemetry/opentelemetry-go-contrib#6053
commit link
I keep getting spammed in logs due to this issue in 3.0.0:rc.1
Example of log spam (thousands of lines per second):
Upgrades the below + any deps that needed upgrades alongside these