Skip to content
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

Update Go modules #479

Merged
merged 2 commits into from May 4, 2023
Merged

Update Go modules #479

merged 2 commits into from May 4, 2023

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented May 3, 2023

  • Replace deprecated github.com/golang/protobuf dependency.
  • Update various Go modules.

@SuperQ SuperQ requested review from roidelapluie, beorn7 and mem May 3, 2023 13:59
@SuperQ
Copy link
Member Author

SuperQ commented May 3, 2023

It's going to take a couple rounds of updates to eliminate the github.com/golang/protobuf dependency from the full go mod graph.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

Nothing works… :-/

@SuperQ
Copy link
Member Author

SuperQ commented May 3, 2023

Not sure what to do about this issue:

  Error: copylocks: assignment copies lock value to *v: github.com/prometheus/client_model/go.MetricFamily contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

Now that we can see the required changes here:

Isn't that exactly the problem that has kept us from doing this kind of upgrade? This will trickle down to prometheus/client_golang, and then users of client_golang that have been importing github.com/golang/protobuf/proto have to change their imports to google.golang.org/protobuf/proto, but the need for that isn't signaled by a major version bump of client_golang.

Not everyone is actually touching any proto types when using client_golang, but there will still be a lot of users affected. @bwplotka @kakkoyun WDYT?

@SuperQ
Copy link
Member Author

SuperQ commented May 3, 2023

I don't think it's actually an issue, we just need to do the update and the circular dependency will resolve. This moves the old library to an indirect dependency, which means it will vanish from client_golang in the next release.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

I think I'm referring to something else.

Look at https://github.com/beorn7/histogram_experiments/blob/master/cmd/float_gauge/main.go as a somewhat weird example (but there are less invasive usages out there that will run into the same problem). It is manipulating the metrics during gathering on the level of the proto types. This program will need to be changed (at least the import line, possibly more) with the first release of client_golang that includes these updates. That's technically a breaking change that is not signaled by a major version bump.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

I mean, we can blame Google for having screwed up the Go implementation of protobuf, and go with it. But it's not an easy call. Therefore ping'ing @bwplotka and @kakkoyun .

@bwplotka
Copy link
Member

bwplotka commented May 3, 2023

Ok, so the breaking change is that github.com/golang/protobuf/proto usages have to change their imports to google.golang.org/protobuf/proto when client_golang will be upgraded.

Is there a way to maintain old behaviour? How this will get bubbled to the user who use our proto code? Compilation error, right?

I would need to check details, but if there is not much we can do it's fine to announce breaking change in release change log and go for it.

Any follow ups on #479 (comment)? I'm busy at the moment, but can take a look on Friday.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

About the copylocks warning: That's to be taken seriously, I think. It's in expfmt/decode.go:135 . Maybe we can explicitly copy just the exported fields? I'll try to propose something.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

Ok, so the breaking change is that github.com/golang/protobuf/proto usages have to change their imports to google.golang.org/protobuf/proto when client_golang will be upgraded.

Yes, but maybe also change code, in the same style as in this PR.
This only affects users who are directly touching types from the proto package, which is not common, but not super uncommon, either.

@SuperQ
Copy link
Member Author

SuperQ commented May 3, 2023

Most users of client_golang don't directly manipulate the proto. So for most users it's just going to be a Go modules change and will happen automatically.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

I have added a commit that should fix the lint warning.

@SuperQ
Copy link
Member Author

SuperQ commented May 3, 2023

I did a quick search, the only prometheus code I could find that directly manipulates the proto is the pushgateway.

@beorn7
Copy link
Member

beorn7 commented May 3, 2023

Yeah, that's a good data point.

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, but the final call should be made by @bwplotka & @kakkoyun.

@SuperQ SuperQ requested review from bwplotka and kakkoyun May 3, 2023 19:18
SuperQ and others added 2 commits May 4, 2023 11:26
* Replace deprecated `github.com/golang/protobuf` dependency.
* Update various Go modules.

Signed-off-by: SuperQ <superq@gmail.com>
Signed-off-by: beorn7 <beorn@grafana.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@SuperQ SuperQ merged commit a2ac260 into main May 4, 2023
8 checks passed
@SuperQ SuperQ deleted the superq/updates branch May 4, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants