Skip to content

channelz: major cleanup / reorganization#6969

Merged
dfawley merged 13 commits intogrpc:masterfrom
dfawley:channelz
Mar 15, 2024
Merged

channelz: major cleanup / reorganization#6969
dfawley merged 13 commits intogrpc:masterfrom
dfawley:channelz

Conversation

@dfawley
Copy link
Copy Markdown
Member

@dfawley dfawley commented Feb 7, 2024 •
edited
Loading

Several major changes are included:

  • Stop using an "ID" to reference the channelz object from the channel/etc that contains it. Instead get a pointer to the object directly.
  • Stop storing the channelz metrics in the channel/etc and instead store them in the channelz struct representation of that thing.
  • Reorganize files so they are categorized by type instead of "func" vs. "types"
  • Lots of small things

To be done:

  • Represent tree of channelz objects using a tree rooted on the top-level types (channel, server) instead of a bunch of flat buffers storing each type with references between them. We still need the flat buffers to look things up through the channelz service, but this might remove the need for manual reference counting, e.g.
  • Cleanup the trace logs to be simpler. There's a pretty complex interaction between the trace logs and the things they reference right now.

RELEASE NOTES: none

@dfawley dfawley added the Type: Internal Cleanup Refactors, etc label Feb 7, 2024
@dfawley dfawley added this to the 1.62 Release milestone Feb 7, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 7, 2024 •
edited
Loading

Codecov Report

Merging #6969 (3de5f23) into master (4f43d2e) will increase coverage by 0.01%.
The diff coverage is 87.60%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6969      +/-   ##
==========================================
+ Coverage   82.44%   82.45%   +0.01%     
==========================================
  Files         296      299       +3     
  Lines       31475    31317     -158     
==========================================
- Hits        25948    25822     -126     
+ Misses       4471     4450      -21     
+ Partials     1056     1045      -11
Files Coverage Δ
balancer/balancer.go 95.83% <ø> (ø)
balancer/grpclb/grpclb_remote_balancer.go 83.69% <100.00%> (ø)
channelz/service/func_linux.go 85.88% <100.00%> (+0.51%) ⬆️
dialoptions.go 89.07% <100.00%> (ø)
internal/channelz/funcs.go 100.00% <100.00%> (+4.97%) ⬆️
internal/transport/http2_client.go 90.82% <100.00%> (-0.13%) ⬇️
internal/transport/http2_server.go 90.74% <100.00%> (-0.06%) ⬇️
internal/transport/transport.go 94.07% <ø> (+1.18%) ⬆️
resolver_wrapper.go 85.18% <100.00%> (ø)
rpc_util.go 77.77% <ø> (ø)
... and 14 more

... and 14 files with indirect coverage changes

@ginayeh ginayeh requested a review from zasweq February 8, 2024 18:09
@ginayeh ginayeh modified the milestones: 1.62 Release, 1.63 Release Feb 8, 2024
@zasweq zasweq requested a review from arvindbr8 February 20, 2024 21:53
@zasweq zasweq assigned arvindbr8 and unassigned arvindbr8 and zasweq Feb 20, 2024
@zasweq zasweq removed their request for review February 20, 2024 21:53
This was referenced Feb 27, 2024
Closed
Closed
Closed
ash2k
ash2k reviewed Mar 6, 2024
View reviewed changes
Comment thread dialoptions.go Outdated
arvindbr8
arvindbr8 suggested changes Mar 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

I've made a first pass -- which was basically a file by file review. However I might need more iterations here as all of this is new to me.

I believe this needs to be a change not visible to the user. How do we verify that UX doesnt change for channelz here? A channelz example might have been useful to detect the change possibly.

Comment thread internal/channelz/channel.go Outdated
Comment thread internal/channelz/channelmap.go
Comment thread internal/channelz/channelmap.go Outdated
Comment thread internal/channelz/channelmap.go Outdated
Comment thread internal/channelz/channelmap.go Outdated
Comment thread clientconn.go Outdated
Comment thread channelz/service/util_sktopt_amd64_test.go
Comment thread channelz/service/service_sktopt_test.go Outdated
Comment thread internal/channelz/channelmap.go Outdated
Comment thread internal/channelz/server.go Outdated
arvindbr8
arvindbr8 suggested changes Mar 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

Seems like everything LGTM modulo a few comments. PTAL

Comment thread clientconn.go Outdated
Comment thread clientconn.go Outdated
Comment thread internal/channelz/trace.go
Comment thread clientconn.go Outdated
Comment thread clientconn.go
Comment thread clientconn.go Outdated
Comment thread dialoptions.go Outdated
Comment thread internal/channelz/funcs.go
@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Mar 12, 2024
@arvindbr8
Copy link
Copy Markdown
Contributor

arvindbr8 commented Mar 13, 2024

I discussed with @dfawley offline. The general refactor and the changes look good to me. There is however still more scope of cleanup - which can be categorized as future improvements to channelz in general.

LGTM, you can ignore the nit formatting comments from above. Also the branch requires a rebase.

@arvindbr8
Copy link
Copy Markdown
Contributor

arvindbr8 commented Mar 13, 2024

I've resolved the convos for nits. Could you take a look at the rest and see if makes sense?

@dfawley
Copy link
Copy Markdown
Member Author

dfawley commented Mar 15, 2024

Thanks for the review! I also merged from master ( 🤯) to fix the conflicts.

@dfawley dfawley assigned arvindbr8 and unassigned dfawley Mar 15, 2024
arvindbr8
arvindbr8 approved these changes Mar 15, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Mar 15, 2024
@dfawley
Copy link
Copy Markdown
Member Author

dfawley commented Mar 15, 2024

🤞

@dfawley dfawley merged commit 55cd7a6 into grpc:master Mar 15, 2024
@dfawley dfawley deleted the channelz branch March 15, 2024 18:14
dfawley added a commit to dfawley/grpc-go that referenced this pull request Mar 18, 2024
@dfawley
Revert "channelz: major cleanup / reorganization (grpc#6969)"
fe83104 
This reverts commit 55cd7a6.
@sergiitk sergiitk mentioned this pull request Mar 19, 2024
Merged
This was referenced Mar 20, 2024
Merged
Merged
This was referenced Mar 21, 2024
Merged
Merged
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ash2k ash2k ash2k left review comments

@arvindbr8 arvindbr8 arvindbr8 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dfawley dfawley

Labels

Type: Internal Cleanup Refactors, etc

Projects

None yet

Milestone

1.63 Release

Development

Successfully merging this pull request may close these issues.

5 participants

@dfawley @arvindbr8 @ash2k @ginayeh @zasweq