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

server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter #32106

Merged
merged 44 commits into from Feb 1, 2023

Conversation

yousukseung
Copy link
Contributor

Support backend metric sampling and make orca intereptor a channel filter

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks like a good start!

Please let me know if you have any questions. Thanks!

include/grpc/impl/grpc_types.h Outdated Show resolved Hide resolved
include/grpcpp/server.h Show resolved Hide resolved
include/grpcpp/server_builder.h Outdated Show resolved Hide resolved
include/grpcpp/server_context.h Outdated Show resolved Hide resolved
include/grpcpp/server_context.h Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
include/grpcpp/server_context.h Outdated Show resolved Hide resolved
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This is moving in the right direction!

Please let me know if you have any questions. Thanks!

include/grpcpp/server.h Show resolved Hide resolved
include/grpcpp/ext/call_metric_recorder.h Outdated Show resolved Hide resolved
include/grpcpp/server_builder.h Outdated Show resolved Hide resolved
include/grpcpp/ext/call_metric_recorder.h Outdated Show resolved Hide resolved
include/grpcpp/ext/server_metric_recorder.h Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
include/grpcpp/ext/server_metric_recorder.h Show resolved Hide resolved
include/grpcpp/ext/server_metric_recorder.h Outdated Show resolved Hide resolved
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This is getting closer!

Please let me know if you have any questions. Thanks!

include/grpcpp/ext/orca_service.h Outdated Show resolved Hide resolved
include/grpcpp/ext/orca_service.h Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
include/grpcpp/ext/orca_service.h Show resolved Hide resolved
test/cpp/end2end/client_lb_end2end_test.cc Show resolved Hide resolved
test/cpp/end2end/orca_service_end2end_test.cc Outdated Show resolved Hide resolved
test/cpp/end2end/orca_service_end2end_test.cc Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
@markdroth
Copy link
Member

It looks like you did the merge with master wrong. All of the unrelated PRs that you merged in were somehow merged from the original commits in their branches, not from master, so they're showing up as part of the changes in this PR.

To fix this, please do something like this:

git fetch upstream
git merge upstream/master
git push origin backend_metric_sampling

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Just one significant comment remaining, which is the one about the synchronization and caching approach in ServerMetricRecorder. The other comments are more minor things.

Please let me know if you have any questions. Thanks!

include/grpcpp/ext/orca_service.h Outdated Show resolved Hide resolved
include/grpcpp/ext/server_metric_recorder.h Outdated Show resolved Hide resolved
include/grpcpp/ext/server_metric_recorder.h Outdated Show resolved Hide resolved
BUILD Outdated Show resolved Hide resolved
src/cpp/server/backend_metric_recorder.h Outdated Show resolved Hide resolved
src/cpp/server/server_context.cc Outdated Show resolved Hide resolved
test/cpp/end2end/client_lb_end2end_test.cc Outdated Show resolved Hide resolved
test/cpp/end2end/client_lb_end2end_test.cc Show resolved Hide resolved
@markdroth markdroth marked this pull request as ready for review January 25, 2023 17:15
@ctiller ctiller merged commit c7f641d into grpc:master Feb 1, 2023
1 check passed
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Feb 1, 2023
XuanWang-Amos added a commit that referenced this pull request Feb 2, 2023
…reporting from a C++ interceptor to a C-core filter (#32106)"

This reverts commit c7f641d.
XuanWang-Amos added a commit that referenced this pull request Feb 2, 2023
…reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)

This reverts commit c7f641d.
yousukseung added a commit to yousukseung/grpc that referenced this pull request Feb 3, 2023
…er-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)"

PR grpc#32106 caused msan errors in some tests while de-referencing the
server object where async calls are active after the server is
destroyed. Instead cache the ServerMetricRecorder pointer.

This reverts commit deb1e25.
yousukseung added a commit to yousukseung/grpc that referenced this pull request Feb 3, 2023
…er-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)"

This reverts commit deb1e25.
yousukseung added a commit to yousukseung/grpc that referenced this pull request Feb 3, 2023
PR grpc#32106 caused msan errors in some tests while de-referencing the
server object where async calls are active after the server is
destroyed. Instead cache the ServerMetricRecorder pointer.
yousukseung added a commit that referenced this pull request Feb 3, 2023
…er-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)

* Revert "Revert "server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)"

This reverts commit deb1e25.

* Fix by caching call metric recording stuff in async request

PR #32106 caused msan errors in some tests while de-referencing the
server object where async calls are active after the server is
destroyed. Instead cache the ServerMetricRecorder pointer.

* copyright headers fixed

* clang fixes.
ctiller added a commit that referenced this pull request Feb 3, 2023
…d move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)"

This reverts commit 4475e74.
ctiller added a commit that referenced this pull request Feb 3, 2023
…d move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)" (#32293)

This reverts commit 4475e74.
@yousukseung yousukseung deleted the backend_metric_sampling branch February 5, 2023 22:14
yousukseung added a commit to yousukseung/grpc that referenced this pull request Feb 5, 2023
…r API and move per-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)" (grpc#32279)" (grpc#32293)"

This reverts commit 1f96069.
markdroth pushed a commit that referenced this pull request Feb 6, 2023
#32301)

* Revert "Revert "Revert "Revert "server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)" (#32293)"

This reverts commit 1f96069.

* Do not create CallMetricRecorder if call is null.
@markdroth markdroth added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Mar 22, 2023
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
…g from a C++ interceptor to a C-core filter (grpc#32106)

* backend metric sampling

* Comments addressed.

* More comments addressed.

* Pushing changes left behind locally.

* Removed empty lines

* Update OrcaService to use ServerMetricRecorder (no named metrics yet)

* Comments addressed.

* More comments addressed

* More comments addressed.

* Comments fixed

* Comments addressed.

* Test fixed

* make seq returned always up-to-date

* skip atomic load when not cached

* Fixed ABSL_GUARDED_BY

* Comments addressed except client_lb_end2end_test

* test updated

* Comments addressed

* BUILD fix.

* BackendMetricDataState moved to a separate header

* comments addressed

* Fixed clang and buildifier errors

* More sanity check errors fixed.

* Fixed xds tests

* Ran generate_projects.sh

* Comments addressed

* comments addressed.

* generate project

* Build fixed

* generate project

* sanity check errors fixed

* test fixed

* Backup poller period override moved to main()

* Also move cfstream override

* Clang fixes, sanitize

* generate_projects.sh

* portable print format fix

* Removed outdated comment
XuanWang-Amos added a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
…reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)

This reverts commit c7f641d.
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
…er-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)" (grpc#32279)

* Revert "Revert "server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)"

This reverts commit deb1e25.

* Fix by caching call metric recording stuff in async request

PR grpc#32106 caused msan errors in some tests while de-referencing the
server object where async calls are active after the server is
destroyed. Instead cache the ServerMetricRecorder pointer.

* copyright headers fixed

* clang fixes.
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
…d move per-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)" (grpc#32279)" (grpc#32293)

This reverts commit 4475e74.
XuanWang-Amos pushed a commit to XuanWang-Amos/grpc that referenced this pull request May 1, 2023
grpc#32301)

* Revert "Revert "Revert "Revert "server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter (grpc#32106)" (grpc#32272)" (grpc#32279)" (grpc#32293)"

This reverts commit 1f96069.

* Do not create CallMetricRecorder if call is null.
wanlin31 pushed a commit that referenced this pull request May 18, 2023
…g from a C++ interceptor to a C-core filter (#32106)

* backend metric sampling

* Comments addressed.

* More comments addressed.

* Pushing changes left behind locally.

* Removed empty lines

* Update OrcaService to use ServerMetricRecorder (no named metrics yet)

* Comments addressed.

* More comments addressed

* More comments addressed.

* Comments fixed

* Comments addressed.

* Test fixed

* make seq returned always up-to-date

* skip atomic load when not cached

* Fixed ABSL_GUARDED_BY

* Comments addressed except client_lb_end2end_test

* test updated

* Comments addressed

* BUILD fix.

* BackendMetricDataState moved to a separate header

* comments addressed

* Fixed clang and buildifier errors

* More sanity check errors fixed.

* Fixed xds tests

* Ran generate_projects.sh

* Comments addressed

* comments addressed.

* generate project

* Build fixed

* generate project

* sanity check errors fixed

* test fixed

* Backup poller period override moved to main()

* Also move cfstream override

* Clang fixes, sanitize

* generate_projects.sh

* portable print format fix

* Removed outdated comment
wanlin31 pushed a commit that referenced this pull request May 18, 2023
…reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)

This reverts commit c7f641d.
wanlin31 pushed a commit that referenced this pull request May 18, 2023
…er-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)

* Revert "Revert "server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)"

This reverts commit deb1e25.

* Fix by caching call metric recording stuff in async request

PR #32106 caused msan errors in some tests while de-referencing the
server object where async calls are active after the server is
destroyed. Instead cache the ServerMetricRecorder pointer.

* copyright headers fixed

* clang fixes.
wanlin31 pushed a commit that referenced this pull request May 18, 2023
…d move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)" (#32293)

This reverts commit 4475e74.
wanlin31 pushed a commit that referenced this pull request May 18, 2023
#32301)

* Revert "Revert "Revert "Revert "server: introduce ServerMetricRecorder API and move per-call reporting from a C++ interceptor to a C-core filter (#32106)" (#32272)" (#32279)" (#32293)"

This reverts commit 1f96069.

* Do not create CallMetricRecorder if call is null.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants