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

stats: add support for multiple stats handlers in a single client or server #5347

Merged
merged 6 commits into from Jun 3, 2022

Conversation

lidizheng
Copy link
Contributor

@lidizheng lidizheng commented May 12, 2022

As title.

The user-facing interface is unchanged, but the behavior is slightly different. (We discussed this) If a user application intentionally sets StatsHandler repeatedly and only expects the last one to be effective, we will break them.

RELEASE NOTES:

  • stats: add support for multiple stats handlers in a single client or server

@lidizheng lidizheng marked this pull request as ready for review May 12, 2022 21:03
@lidizheng
Copy link
Contributor Author

@dfawley @menghanl PTAL.

@lidizheng
Copy link
Contributor Author

Bump.

@lidizheng
Copy link
Contributor Author

lidizheng commented May 20, 2022

Bump again. PTAL. Let me know if there is any problem with the design or idea or code.

@dfawley dfawley self-assigned this May 24, 2022
@dfawley dfawley self-requested a review May 24, 2022 17:09
1. Pluralize the variable names for slices
2. Remove the unnecessary ifs
3. Simplify the unit test
@lidizheng
Copy link
Contributor Author

@dfawley PTALA.

The check-in-loop in stats_test is actually necessary. After graceful shutdown of server, the ConnEnd events might not be counted, and causing the test to fail: https://github.com/grpc/grpc-go/runs/6699361791?check_suite_focus=true . For client-side, the close of channel doesn't mean all processing logic on the client-side is finished, hence the problem.

Other renaming or optimizations (remove if) are done.

@lidizheng
Copy link
Contributor Author

The failure is unrelated.

dfawley
dfawley approved these changes Jun 2, 2022
server.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned lidizheng and unassigned dfawley Jun 2, 2022
server.go Show resolved Hide resolved
@dfawley dfawley added this to the 1.48 Release milestone Jun 2, 2022
@lidizheng
Copy link
Contributor Author

@dfawley PTALAAA.

@dfawley dfawley changed the title stats: Support multiple StatsHandlers stats: add support for multiple stats handlers in a single client or server Jun 3, 2022
@dfawley dfawley merged commit ea86bf7 into grpc:master Jun 3, 2022
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants