-
Notifications
You must be signed in to change notification settings - Fork 828
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 to opencensus v0.22 #893
Closed
Closed
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
4592663
Merge pull request #1 from googleforgames/master
pjayara-g eb2470f
Merge pull request #2 from googleforgames/master
pjayara-g 8e167d9
update to opencensus v0.22
pjayara-g cf0f02a
Merge branch 'master' of https://github.com/pjayara-g/agones
pjayara-g af09499
Merge branch 'master' into master
pjayara-g 44abd34
fixing a lint issue
pjayara-g db66ffa
Merge branch 'master' of https://github.com/pjayara-g/agones
pjayara-g 6458d55
Merge branch 'master' into master
pjayara-g 9dc8c45
Merge branch 'master' into master
pjayara-g 658362f
Merge branch 'master' into master
pjayara-g d57365e
Merge branch 'master' into master
pjayara-g f06e709
Merge branch 'master' into master
pjayara-g a535add
Merge branch 'master' into master
pjayara-g ea119f4
Merge branch 'master' into master
pjayara-g File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
grpc bump and protobuf bump. @markmandel is that ok ? does it requires tooling update ?
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.
Good catch. That's a big jump. If we do this, yes - we should regenerate things, and confirm
Do we need this update to update opencensus as well? Are they linked dependencies?
I'm almost wondering if this should be split into some separate PRs to make this easier to review?
Would that be possible?
Chrome actually crashes on this page 😞 I should fire up Firefox, it usually works.
I'll also dig through this this week too.
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.
I suspect they are linked dependencies. The only change I had made was to upgrade to 0.22. The rest came from running go mod vendor and go mod tidy.
I cannot split the test changes. One consequence of moving to 0.22 is that @Kuqd 's trick of unregistering/registering to publish metrics in tests no longer works reliably.
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.
I would like to have first a grpc/protobuf bump to the version you need.
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.
That seems like a good strategy to me 👍 grpc is annoyingly backward compatible, so i expect nothing will break even if we update only one side.
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.
FWIW, if grpc or genproto are not backwards compatible than a lot of things would break. Both libraries need to maintain backwards compatibility to be useful since clients and servers cannot be updated at the same time.
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.
So I would agree with @Kuqd - I think the right step would be:
That should make it a nicer, and safer process. WDYT?
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.
Sure thing.