[Merged by Bors] - Use ctxzap in GRPC services #4816
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- log start of GRPC call with middleware - use context-aware logging in GRPC services with ctxzap - give time to send response on AdminService/Recover before shutting down - shutdown with os.Exit instead of panic on /Recover
Codecov Report
@@ Coverage Diff @@
## develop #4816 +/- ##
=========================================
- Coverage 76.9% 76.8% -0.1%
=========================================
Files 261 261
Lines 30259 30215 -44
=========================================
- Hits 23274 23212 -62
- Misses 5485 5501 +16
- Partials 1500 1502 +2
|
fasmat
requested changes
Aug 11, 2023
|
@fasmat is there anything else you require changes for? Regarding |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Aug 17, 2023
## Motivation The start of GRPC calls was manually logged. It can be automated and enriched with contextual information about the GRPC connection using middleware. We already use Zap's [UnaryServerInterceptor](https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware@v1.4.0/logging/zap#UnaryServerInterceptor) that appends a logger into the `Context` of a GRPC call. It can be later extracted with [ctxzap](https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap). ## Changes - log start of GRPC call with middleware - use context-aware logging in GRPC services with ctxzap - give time to send a response on AdminService/Recover before shutting down - shutdown with os.Exit instead of panic on /Recover ## Test Plan - existing tests pass - added test for AdminService/Recover
|
Build failed: |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Aug 18, 2023
## Motivation The start of GRPC calls was manually logged. It can be automated and enriched with contextual information about the GRPC connection using middleware. We already use Zap's [UnaryServerInterceptor](https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware@v1.4.0/logging/zap#UnaryServerInterceptor) that appends a logger into the `Context` of a GRPC call. It can be later extracted with [ctxzap](https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap). ## Changes - log start of GRPC call with middleware - use context-aware logging in GRPC services with ctxzap - give time to send a response on AdminService/Recover before shutting down - shutdown with os.Exit instead of panic on /Recover ## Test Plan - existing tests pass - added test for AdminService/Recover
|
Build failed: |
|
bors try |
|
@countvonzero could you please check my changes around recovery? I changed it to quit asynchronously after 1 second to give time to send a GRPC response. Do you see any potential problems with it? |
tryBuild failed: |
|
bors try |
tryBuild failed: |
thanks for double-checking. there is no problem with Recover() RPC. it's also only used for systest. |
poszu
force-pushed
the
use-middleware-to-log-grpc-start
branch
from
dd39156
to
c7100ec
Compare
poszu
force-pushed
the
use-middleware-to-log-grpc-start
branch
from
15a89e2
to
ce3c3dd
Compare
|
Bors merge |
bors bot
pushed a commit
that referenced
this pull request
Aug 21, 2023
## Motivation The start of GRPC calls was manually logged. It can be automated and enriched with contextual information about the GRPC connection using middleware. We already use Zap's [UnaryServerInterceptor](https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware@v1.4.0/logging/zap#UnaryServerInterceptor) that appends a logger into the `Context` of a GRPC call. It can be later extracted with [ctxzap](https://pkg.go.dev/github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap). ## Changes - log start of GRPC call with middleware - use context-aware logging in GRPC services with ctxzap - give time to send a response on AdminService/Recover before shutting down - shutdown with os.Exit instead of panic on /Recover ## Test Plan - existing tests pass - added test for AdminService/Recover
|
Pull request successfully merged into develop. Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The start of GRPC calls was manually logged. It can be automated and enriched with contextual information about the GRPC connection using middleware. We already use Zap's UnaryServerInterceptor that appends a logger into the
Contextof a GRPC call. It can be later extracted with ctxzap.Changes
Test Plan