fix: set version from IBM/sarama, not main app #3415
Conversation
adamdecaf
force-pushed
the
fix-clientSoftwareVersion
branch
from
d3bc2d9 to
8f9b318
Compare
version.go
Outdated
| if ok { | ||
| v = bi.Main.Version | ||
| for _, dep := range bi.Deps { | ||
| if dep.Path == "github.com/IBM/sarama" { |
There was a problem hiding this comment.
🤔 huh… it would probably be way better if we had some sort of “give me the build info for this module itself” rather than relying on the name here… otherwise this becomes a brittle magic string.
Actually… maybe this might work?: https://go.dev/play/p/LC1Q8tpP83T
type getPackageName struct{}
thisPackagePath := reflect.TypeFor[getPackageName]().PkgPath()There was a problem hiding this comment.
That's a good idea. I've updated the PR.
There was a problem hiding this comment.
I've verified this works when compiled into a binary as well. Using replace to point to a local copy of IBM/sarama still finds github.com/IBM/sarama and v1.46.3 when running as a library in a binary.
There was a problem hiding this comment.
Hmm. The reflect approach adds an extra import, a dummy struct type and a reflect call at runtime and the only benefit would come if the module was ever renamed in the future – but that rename would already comprise rewriting all the import path strings anyway, so it’s not immediately obvious to me that it’s preferable to the simple hardcoded string comparison that you originally had?
There was a problem hiding this comment.
The reflect call at runtime is minimal, since it’s called inside of a sync.Once. mockbroker.go imports "reflect" already, and it’s a standard library, and the dummy type is stuck in a very limited scope within the function itself.
I would say the difference here is that the imports that need to be rewritten would generally be found during compilation, as they would break. While without reflection this code would continue to function without any apparent error; a silent error.
Centrally, the only reason I proposed any of this is because it’s a one-time sync.Once thing. I would have had different concerns (especially the reflection) if it were any sort of a regular call.
adamdecaf
force-pushed
the
fix-clientSoftwareVersion
branch
2 times, most recently
from
5235762 to
877151c
Compare
|
👍 I don’t see anything of note. |
dnwe
left a comment
dnwe
left a comment
There was a problem hiding this comment.
Thanks for spotting this bug! I’m surprised this is the first time it has been noticed
We are experiencing connection errors with a hosted Kafka vendor who is inspecting ClientSoftwareVersion. Currently this version is "v0.34.1" -- our application's version, but Sarama is on v1.46.3 which is what the vendor expects to see. This behavior matches franz-go as well: https://github.com/twmb/franz-go/blob/v1.20.6/pkg/kgo/config.go#L480 Signed-off-by: Adam Shannon <[email protected]>
adamdecaf
force-pushed
the
fix-clientSoftwareVersion
branch
from
91a29e5 to
c535127
Compare
|
Is there anything I need to do before this can be merged? |
3 participants
We are experiencing connection errors with a hosted Kafka vendor who is inspecting ClientSoftwareVersion. Currently this version is "v0.34.1" -- our application's version, but Sarama is on v1.46.3 which is what the vendor expects to see.
This behavior matches franz-go as well:
https://github.com/twmb/franz-go/blob/v1.20.6/pkg/kgo/config.go#L480