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

channelz: major cleanup / reorganization #6969

Merged
merged 13 commits into from Mar 15, 2024
Merged

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Feb 7, 2024

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
Copy link

codecov bot commented Feb 7, 2024

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

dialoptions.go Outdated Show resolved Hide resolved
Copy link
Member

@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.

internal/channelz/channel.go Outdated Show resolved Hide resolved
internal/channelz/channelmap.go Show resolved Hide resolved
internal/channelz/channelmap.go Outdated Show resolved Hide resolved
internal/channelz/channelmap.go Outdated Show resolved Hide resolved
internal/channelz/channelmap.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
channelz/service/util_sktopt_amd64_test.go Outdated Show resolved Hide resolved
channelz/service/service_sktopt_test.go Outdated Show resolved Hide resolved
internal/channelz/channelmap.go Outdated Show resolved Hide resolved
internal/channelz/server.go Outdated Show resolved Hide resolved
Copy link
Member

@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

clientconn.go Outdated Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
internal/channelz/trace.go Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
clientconn.go Show resolved Hide resolved
clientconn.go Outdated Show resolved Hide resolved
dialoptions.go Outdated Show resolved Hide resolved
internal/channelz/funcs.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Mar 12, 2024
@arvindbr8
Copy link
Member

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
Member

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

@dfawley
Copy link
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
Copy link
Member

@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
Member Author

dfawley commented Mar 15, 2024

🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants