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

chore: use Grpc inproc #3856

Merged
merged 25 commits into from
Jan 31, 2025
Merged

chore: use Grpc inproc #3856

merged 25 commits into from
Jan 31, 2025

Conversation

markphelps
Copy link
Collaborator

Re: https://www.bwplotka.dev/2025/go-grpc-inprocess-iter/

Switches to an inprocess channel for routing GRPC gateway requests to the GRPC server

This should be much more performant since we don't need to go over the loopback network as described in above blog post ☝🏻

@markphelps markphelps requested a review from a team as a code owner January 29, 2025 21:50
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 1.26582% with 78 lines in your changes missing coverage. Please review.

Project coverage is 63.61%. Comparing base (9326370) to head (899e730).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/cmd/authn.go 0.00% 38 Missing ⚠️
internal/cmd/grpc.go 0.00% 27 Missing ⚠️
cmd/flipt/main.go 0.00% 7 Missing ⚠️
internal/cmd/http.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3856      +/-   ##
==========================================
- Coverage   64.85%   63.61%   -1.24%     
==========================================
  Files         171      171              
  Lines       17400    17380      -20     
==========================================
- Hits        11284    11057     -227     
- Misses       5418     5657     +239     
+ Partials      698      666      -32     
Flag Coverage Δ
unittests 63.61% <1.26%> (-1.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@markphelps
Copy link
Collaborator Author

Well something doesn't work quite right it seems

@GeorgeMac
Copy link
Contributor

GeorgeMac commented Jan 30, 2025

Well something doesn't work quite right it seems

Im guessing something to do with the middleware as its not failing wholesale to perform operations.
Some things are getting through and I can see e.g. errors being reported back as unknown when they have expected concrete status types (e.g. invalid), where there error messages are still as expected. Makes me think for example that the error status middleware is not registered properly.

The ITs used to have the protocol used in the test reported, but I think that is missing from the test output now.
Would be good to know if these failures are e.g. scoped to HTTP through gateway vs just gRPC.

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 30, 2025
Signed-off-by: Mark Phelps <[email protected]>
Signed-off-by: Mark Phelps <[email protected]>
Signed-off-by: Mark Phelps <[email protected]>
@erka
Copy link
Contributor

erka commented Jan 30, 2025

Somehow etag breaks the response for snapshot

Signed-off-by: Roman Dmytrenko <[email protected]>
@markphelps
Copy link
Collaborator Author

Thank you @erka !

Copy link
Contributor

@erka erka left a comment

Choose a reason for hiding this comment

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

From a performance perspective, the results are very promising. wrk benchmarking on my laptop indicates a doubling of speed for response time.

@markphelps
Copy link
Collaborator Author

Awesome! My next step was gonna be to benchmark and also do some more testing to make sure everything works correctly

@markphelps
Copy link
Collaborator Author

@erka would you mind sharing your benchmark setup and/or results here in a comment and I'll do the same?

@erka
Copy link
Contributor

erka commented Jan 30, 2025

I just use go-wrk for testing. Probably it's possible to use wrk and with extra lua script.

go-wrk -M POST -H "Content-Type: application/json" -H "Accept: application/json" -body '{"flagKey":"color-scheme","entityId":"a","context":{"subscription":"active"},"namespaceKey":"default"}' -d 30 http://localhost:8080/evaluate/v1/variant

@markphelps
Copy link
Collaborator Author

Before (main branch)

129625 requests in 29.967058948s, 50.42MB read
Requests/sec:           4325.58
Transfer/sec:           1.68MB
Overall Requests/sec:   4320.24
Overall Transfer/sec:   1.68MB
Fastest Request:        631µs
Avg Req Time:           2.311ms
Slowest Request:        66.859ms
Number of Errors:       0
10%:                    841µs
50%:                    955µs
75%:                    992µs
99%:                    1.022ms
99.9%:                  1.023ms
99.9999%:               1.023ms
99.99999%:              1.023ms
stddev:                 1.096ms

After (this branch)

147780 requests in 29.967327337s, 51.00MB read
Requests/sec:           4931.37
Transfer/sec:           1.70MB
Overall Requests/sec:   4925.62
Overall Transfer/sec:   1.70MB
Fastest Request:        513µs
Avg Req Time:           2.027ms
Slowest Request:        22.795ms
Number of Errors:       0
10%:                    690µs
50%:                    796µs
75%:                    832µs
99%:                    857µs
99.9%:                  858µs
99.9999%:               858µs
99.99999%:              858µs
stddev:                 751µs

@markphelps markphelps requested a review from GeorgeMac January 31, 2025 15:30
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Very nice 👌

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label Jan 31, 2025
@kodiakhq kodiakhq bot merged commit ca47763 into main Jan 31, 2025
37 of 39 checks passed
@kodiakhq kodiakhq bot deleted the grpc-inproc branch January 31, 2025 16:07
@markphelps markphelps mentioned this pull request Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants