-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for go 1.20 #617
Conversation
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
0370739
to
b758cb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Not directly relevant to that PR, but we should call out where we do support older Go versions. |
This PR is focused on enabling client_golang, which promises support for the 3 latest versions. I joined the team after this decision was made, I don't know the reason behind it, to be honest. Happy to discuss if that makes sense to you @kakkoyun and @bwplotka |
Signed-off-by: Arthur Silva Sens <arthur.sens@coralogix.com>
b758cb8
to
1822e0c
Compare
Has this changed recently? I was under the impression that the upstream Go supported the latest three major versions. Thus, client_golang supports the same. We should always follow the same upstream release policy. This was a rule that was in place before @bwplotka joined the team. If Go didn't change the release policy after this rule was implemented, there could be another historical reason behind it. @beorn7, I'm curious: Does this ring any bells? |
Where is this written down, please? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ArthurSens!
Yea, it's 3 releases for client_golang and it is a bit fair (IMO). If we want to change that, let' perhaps have a dedicated topic on client_golang/DevSummit.
We can also drag the discussion until the next Go release and we would be good (no sarcasm here, this happened a lot and that's ok, just funny =D)
I would merge this to unblock client_golang for now -- until we consider changing policy/new release happens (summer time).
I don't think the support promise was ever modeled on promises by any support for older Go version by the Go team. It might have just been a rule of thumb we made up, knowing from experience that many users must use older Go toolchains for one reason or another, even if that isn't a good situation to be in. But rules can be changed if they don't seem useful anymore. |
for name := range l { | ||
labelNames = append(labelNames, string(name)) | ||
} | ||
sort.Strings(labelNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this was:
slices.Sort(labelNames)
Which makes a difference to performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, sorry for the copy paste mistake!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? Changing the type of lna
from LabelName
to string
looks quite deliberate to me.
Since #590 (released in v0.49.0), common doesn't work for go 1.20 or lower versions since it uses functions that are only available since go 1.21.
Several features that do not require go 1.21 were introduced in common, but cannot be used by downstream projects that are still stuck with go 1.20 for a while, e.g. client_golang that wants to use the new OpenMetrics created timestamps and units.
This PR rescues the old implementation of
LabelSet.String()
under build tags. When using 1.20, common would still work even without the optimizations made in #590