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.

Sign up for GitHub

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

Jump to bottom

examples: add an example to illustrate the usage of stats handler #5657

Merged
merged 17 commits into from Nov 29, 2022
Merged

examples: add an example to illustrate the usage of stats handler #5657

merged 17 commits into from Nov 29, 2022

Conversation

Yash-Handa
Copy link
Contributor

@Yash-Handa Yash-Handa commented Sep 15, 2022 •
edited by dfawley

RELEASE NOTES:

  • examples: add an example to illustrate the usage of stats handler

@Yash-Handa
Draft to illustrate the usage of stats handler
1aa4994 
 Changes to be committed:
	new file:   examples/features/stats_monitoring/README.md
	new file:   examples/features/stats_monitoring/client/main.go
	new file:   examples/features/stats_monitoring/server/main.go
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 15, 2022 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Yash-Handa / name: Yash Handa (1aa4994)

easwars
easwars reviewed Sep 23, 2022
View reviewed changes
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Apologize for the delay in the review.

examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/client/main.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/server/main.go Outdated Show resolved Hide resolved
@easwars easwars assigned Yash-Handa and unassigned easwars Sep 23, 2022
@Yash-Handa
Merge branch 'master' into stats_example
7d6e7e6
@Yash-Handa
Made trivial changes and demonstrated ctx passing usage
2198964 
 Changes to be committed:
	modified:   examples/features/stats_monitoring/client/main.go
	modified:   examples/features/stats_monitoring/server/main.go
@Yash-Handa
Copy link
Contributor Author

Hi @easwars,

Thanks for the review. I made some trivial changes and added usage of ctx from the TagRPC and TagConn methods (client side, for now).
I require some suggestions though,

Please print the complete Begin struct, and the same comment applies for other cases as well. Thanks.

The example prints custom messages for different stats.ConnStats so that a switch block can be used (It clarifies different types of stats.ConnStats). But if a general message is printed for all the types like: log.Printf("[HandleRPC] [%T]: %+[1]v", stat), then the switch cases will become redundant.

A plus point is: a single package (implementation) of statsHandler could be easily shared between the server and the client as requested.

Let me know if a general print statement would be enough (instead of the switch case block) so that I can implement the same changes on the server code and extract the statsHandler in another package.

@github-actions
Copy link

github-actions bot commented Oct 3, 2022

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Oct 3, 2022
@easwars
Copy link
Contributor

easwars commented Oct 3, 2022

Let me know if a general print statement would be enough (instead of the switch case block) so that I can implement the same changes on the server code and extract the statsHandler in another package.

@Yash-Handa : Yes, a general print statement should be good enough. Thank you for doing this.

@easwars easwars added the Type: Documentation Documentation or examples label Oct 3, 2022
@easwars easwars added this to the 1.50 Release milestone Oct 3, 2022
@dfawley dfawley modified the milestones: 1.50 Release, 1.51 Release Oct 4, 2022
@easwars
Copy link
Contributor

easwars commented Oct 10, 2022

I saw a commit recently on this PR. Please let me know when this is ready to looked at again. Thanks for doing this.

@Yash-Handa
Abstracted stats.Handler implementation to seprate package with general
eb9e0b7 
logs.

 Changes to be committed:
	modified:   examples/features/stats_monitoring/README.md
	modified:   examples/features/stats_monitoring/client/main.go
	modified:   examples/features/stats_monitoring/server/main.go
	new file:   examples/features/stats_monitoring/statshandler/handler.go
@Yash-Handa
Merge branch 'master' into stats_example
fbadcc0
@Yash-Handa
Copy link
Contributor Author

Hi @easwars, apologies for the delay.

Updated the example with the following:

  • Abstract the stats.Handler interface into the statshandler package.
  • Generalized log statements for HandleRPC and HandleConn.
  • Other minor fixes.

Let me know if further changes are required : )

@github-actions
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Oct 18, 2022
@easwars easwars assigned dfawley and unassigned Yash-Handa Oct 31, 2022
@easwars
Copy link
Contributor

easwars commented Oct 31, 2022

@dfawley : For second set of eyes. Thank you.

dfawley
dfawley reviewed Nov 4, 2022
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Looks mostly good, but I have a few comments on text and comments.

examples/features/stats_monitoring/README.md Outdated Show resolved Hide resolved
examples/features/stats_monitoring/README.md Outdated Show resolved Hide resolved
examples/features/stats_monitoring/README.md Outdated Show resolved Hide resolved
examples/features/stats_monitoring/README.md Outdated Show resolved Hide resolved
examples/features/stats_monitoring/statshandler/handler.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/statshandler/handler.go Outdated Show resolved Hide resolved
examples/features/stats_monitoring/statshandler/handler.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned Yash-Handa and unassigned dfawley Nov 4, 2022
@zasweq zasweq modified the milestones: 1.51 Release, 1.52 Release Nov 8, 2022
@Yash-Handa
Added TagRPC and TagConn to README.md plus minor edits.
c79be03 
 Changes to be committed:
	modified:   examples/features/stats_monitoring/README.md
	modified:   examples/features/stats_monitoring/statshandler/handler.go
@Yash-Handa
Merge branch 'master' into stats_example
12fee3b
@Yash-Handa
Copy link
Contributor Author

Hi @dfawley and @easwars,

Added TagRPC(context.Context, *RPCTagInfo) context.Context and TagConn(context.Context, *ConnTagInfo) context.Context methods to README plus did other minor edits.

Let me know if any other changes are required : )

@easwars easwars assigned dfawley and unassigned Yash-Handa Nov 15, 2022
dfawley
dfawley approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM except for the long lines in the README.

examples/features/stats_monitoring/README.md Outdated Show resolved Hide resolved
@dfawley dfawley assigned Yash-Handa and unassigned dfawley Nov 23, 2022
@dfawley
Copy link
Member

dfawley commented Nov 23, 2022

cc @temawi as FYI.

@Yash-Handa
Merge branch 'master' into stats_example
7d53b16
@Yash-Handa
wrapped README.md to 80 columns.
d4f5f01 
 Changes to be committed:
	modified:   examples/features/stats_monitoring/README.md
@Yash-Handa
Copy link
Contributor Author

Hi @dfawley and @easwars,

Wrapped the README.md file to 80 columns manually (using editor column count and entering carriage returns where required).

Let me know if it works or if something else is required : )

@dfawley
Copy link
Member

dfawley commented Nov 29, 2022

Thanks for the contribution!

@dfawley dfawley merged commit be202a2 into grpc:master Nov 29, 2022
1 check passed
@easwars easwars mentioned this pull request Mar 7, 2023
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@easwars easwars easwars approved these changes

@dfawley dfawley dfawley approved these changes

Assignees

@Yash-Handa Yash-Handa

Labels
Status: Requires Reporter Clarification Type: Documentation Documentation or examples
Projects
None yet
Milestone
1.52 Release
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Yash-Handa @easwars @dfawley @zasweq