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

Migrate RPC to ConnectRPC #703

Merged
merged 33 commits into from
Dec 15, 2023
Merged

Migrate RPC to ConnectRPC #703

merged 33 commits into from
Dec 15, 2023

Conversation

krapie
Copy link
Member

@krapie krapie commented Nov 28, 2023

What this PR does / why we need it:

Migrate RPC to ConnecRPC.

ConnectRPC supports following features:

  • Supports interoperation and multiplexing of grpc, grpc-web, and Connect protocols.
  • Supports language-familiar primitives like net/http and fetch.
  • Supports simple and no-boilerplate codes.

With this support, you can resolve several issues:

  • Removes dependency with Envoy proxy to communicate with JS SDK web client by grpc-web.
  • Removes dependency with deprecated gogo/protobuf.
  • Supports JSON based REST API which will satisfy these needs.

Which issue(s) this PR fixes:

Related to #668

Special notes for your reviewer:

Note

After merging this PR, I will remove envoy proxy on docker configuration on other repositories too.

Note

JS SDK compatibility: Since connect-go does not support grpc-web-text which current JS SDK gRPC-web client uses, we need to migrate JS SDK's RPC to ConnectRPC as well. We can temporary change JS SDK's docker-compose settings (keep envoy proxy and change server rpc port to 11101) to support compatibility with ConnectRPC server and JS SDK gRPC-web client before finishing migration of ConnectRPC on JS SDK.

Note

Server configuration: We need to find corresponding net/http server configurations that matches with gRPC server options such as MaxRequestBytes, MaxConnectAge and MaxConnectionAgeGrace. Because of this issue, watch document test is temporary commented.

Note

gRPC Metrics: Since we cannot use go-grpc-prometheus that measured gRPC server metrics, I have implemented RPC metrics by referencing go-grpc-prometheus. Because the syntax of newly implemented metrics are different from go-grpc-prometheus, we also need to update Grafana query (ex: grpc_server_handled_total to yorkie_rpc_server_handled_total, grpc_type to rpc_type).

Note

Since we no longer have Envoy proxy to handle CORS, I have implemented CORS on RPC server. But for cluster mode, we have ingressgateway to handle CORS. So we need to implement disable option for CORS on server.

Does this PR introduce a user-facing change?:

- Server port has been changed to `8080` from `11101` for RPC port, and `8081` from `11102` for profiling port.
- User can now also send HTTP requests and receive JSON response. For example:
  curl -X POST \
      --url http://localhost:8080/yorkie.v1.AdminService/GetDocument \
      --header "Content-Type: application/json" \
      --header "Authorization: eyJBDB...." \
      --data '{"project_name": "default", "document_key": "codemirror"}

Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@krapie krapie added the protocol changed 📝 Whether the protocol has changed label Nov 30, 2023
@krapie krapie force-pushed the connectrpc branch 2 times, most recently from cff9804 to b85c9ff Compare December 2, 2023 10:42
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 160 lines in your changes are missing coverage. Please review.

Comparison is base (1006b64) 49.11% compared to head (34795af) 49.27%.
Report is 2 commits behind head on main.

Files Patch % Lines
client/client.go 25.00% 56 Missing and 1 partial ⚠️
api/converter/to_pb.go 0.00% 21 Missing ⚠️
server/rpc/admin_server.go 74.32% 19 Missing ⚠️
api/converter/from_pb.go 0.00% 18 Missing ⚠️
client/auth.go 39.13% 14 Missing ⚠️
server/rpc/server.go 75.00% 12 Missing and 2 partials ⚠️
server/rpc/yorkie_server.go 73.58% 10 Missing and 4 partials ⚠️
server/backend/database/change_info.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #703      +/-   ##
==========================================
+ Coverage   49.11%   49.27%   +0.15%     
==========================================
  Files          69       69              
  Lines       10194    10081     -113     
==========================================
- Hits         5007     4967      -40     
+ Misses       4656     4598      -58     
+ Partials      531      516      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@krapie krapie self-assigned this Dec 7, 2023
@krapie krapie marked this pull request as ready for review December 12, 2023 04:25
@krapie krapie requested a review from hackerwins December 12, 2023 04:25
@hackerwins
Copy link
Member

hackerwins commented Dec 13, 2023

@7hong13, @humdrum

Before merging this PR, we need to ensure that mobile SDKs can communicate with the server. Yorkie Go Server already multiplexes the existing grpc protocol, we expect no issues.

@chacha912

Could we test dashboard works well too?

@chacha912
Copy link
Contributor

When testing on the dashboard, the following error occurs:

@krapie
Copy link
Member Author

krapie commented Dec 13, 2023

@chacha912 This is because dashboard also uses grpc-web-text format which ConnectRPC server does not support.
We also need to update dashboard's RPC to ConnectRPC, just like JS SDK.

@hackerwins
Copy link
Member

Related to yorkie-team/dashboard#42

@hackerwins hackerwins requested a review from blurfx December 14, 2023 09:57
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@hackerwins hackerwins merged commit 224ea4d into main Dec 15, 2023
3 checks passed
@hackerwins hackerwins deleted the connectrpc branch December 15, 2023 07:34
hackerwins pushed a commit that referenced this pull request Dec 15, 2023
ConnectRPC supports following features:

- Supports interoperation and multiplexing of grpc, grpc-web, and
  Connect protocols.
- Supports language-familiar primitives like net/http and fetch.
- Supports simple and no-boilerplate codes.

With this support, we can resolve several issues:

- Removes dependency with Envoy proxy to communicate with JS SDK web
  client by grpc-web.
- Removes dependency with deprecated gogo/protobuf.
- Supports JSON based REST API which will satisfy these needs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol changed 📝 Whether the protocol has changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants