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

feat: debug log gRPC queries #19049

Merged
merged 10 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ Every Module contains its own CHANGELOG.md. Please refer to the module you are i
* (runtime) [#18475](https://github.com/cosmos/cosmos-sdk/pull/18475) Adds an implementation for core.branch.Service.
* (baseapp) [#18499](https://github.com/cosmos/cosmos-sdk/pull/18499) Add `MsgRouter` response type from message name function.
* (types) [#18768](https://github.com/cosmos/cosmos-sdk/pull/18768) Add MustValAddressFromBech32 function.
* (gRPC) [#19049](https://github.com/cosmos/cosmos-sdk/pull/19049) Add config option to log gRPC queries.

### Improvements

Expand Down Expand Up @@ -129,6 +130,7 @@ Every Module contains its own CHANGELOG.md. Please refer to the module you are i
* (types) [#18372](https://github.com/cosmos/cosmos-sdk/pull/18372) Removed global configuration for coin type and purpose. Setters and getters should be removed and access directly to defined types.
* (types) [#18695](https://github.com/cosmos/cosmos-sdk/pull/18695) Removed global configuration for txEncoder.
* (server) [#18909](https://github.com/cosmos/cosmos-sdk/pull/18909) Remove configuration endpoint on grpc reflection endpoint in favour of auth module bech32prefix endpoint already exposed.
* (baseapp) [#19049](https://github.com/cosmos/cosmos-sdk/pull/19049) `RegisterGRPCServer` now also takes a `logQueries` boolean.

### Client Breaking Changes

Expand Down
7 changes: 6 additions & 1 deletion baseapp/grpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package baseapp

import (
"context"
"fmt"
"strconv"

gogogrpc "github.com/cosmos/gogoproto/grpc"
Expand All @@ -23,7 +24,7 @@ import (
func (app *BaseApp) GRPCQueryRouter() *GRPCQueryRouter { return app.grpcQueryRouter }

// RegisterGRPCServer registers gRPC services directly with the gRPC server.
func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) {
func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server, logQueries bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an API breaking changelog?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here e82a084

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would instead directly supply grpcConfig here instead. This future proofs us (even though Baseapp will be sunsetted soon) in case we need other fields or data when calling RegisterGRPCServer.

// Define an interceptor for all gRPC queries: this interceptor will create
// a new sdk.Context, and pass it into the query handler.
interceptor := func(grpcCtx context.Context, req interface{}, _ *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
Expand Down Expand Up @@ -67,6 +68,10 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) {
app.logger.Error("failed to set gRPC header", "err", err)
}

if logQueries {
app.logger.Info("gRPC query received of type: " + fmt.Sprintf("%#v", req))
Copy link
Member

Choose a reason for hiding this comment

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

why not put this as debug and use log filtering for the info you need? another config variable seems overkill here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thought here is, this will only be enabled in the event we are actively debugging something, and have an integrator to send me their info logs is much easier than asking to send massive debug logs and/or filter it themselves.

Happy to change it in this PR for the sdk, will probably keep as info for the fork for now though.

Copy link
Member

Choose a reason for hiding this comment

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

i think this is what filter logging was created for or one of the reasons. It would be much cleaner to ask an integrator to turn on debugging and paste "....." in the filter logs. This way they only get the logs you want. Currently this approach you will need to ask for a snapshot or they will need to search for the log along side all the other infos.

The filter would be to see blocks are being created and this debug log. So you as the debugger would make your life easier.

This is encroaching on devops territory which there are tools for, we dont want to make the sdk more complicated to run and debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Np, will change sdk side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

sorry i meant no config changes, just a debug statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a node under heavy load like ours (250 rps), this might make the debug logs impossible to grok. I know one can filter but it seems helpful for the logs to be somewhat manageable, and there are rare edge cases where this info is needed. I think a config makes the most sense.

Copy link
Member

Choose a reason for hiding this comment

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

this introduces two ways to do the same thing. feels like this will be confusing to users as if they try to filter for this log it wont show up since its blocked on another config value. We can add it but I feel its over kill for something already suported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it, I just wanted to explain why I added it as a config in the first place. I am actually not familiar with the debug filtering at all so that's why this wasn't clear to me. Happy to remove it

}

return handler(grpcCtx, req)
}

Expand Down
8 changes: 8 additions & 0 deletions server/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ const (
// DefaultGRPCMaxSendMsgSize defines the default gRPC max message size in
// bytes the server can send.
DefaultGRPCMaxSendMsgSize = math.MaxInt32

// DefaultLogQueries defines the default value for the log_queries parameter.
// Should be set to false unless debugging.
DefaultLogQueries = false
)

// BaseConfig defines the server's basic configuration
Expand Down Expand Up @@ -139,6 +143,9 @@ type GRPCConfig struct {
// MaxSendMsgSize defines the max message size in bytes the server can send.
// The default value is math.MaxInt32.
MaxSendMsgSize int `mapstructure:"max-send-msg-size"`

// LogQueries logs every gRPC query to the console as an info log.
LogQueries bool `mapstructure:"log-queries"`
}

// GRPCWebConfig defines configuration for the gRPC-web server.
Expand Down Expand Up @@ -248,6 +255,7 @@ func DefaultConfig() *Config {
Address: DefaultGRPCAddress,
MaxRecvMsgSize: DefaultGRPCMaxRecvMsgSize,
MaxSendMsgSize: DefaultGRPCMaxSendMsgSize,
LogQueries: DefaultLogQueries,
},
GRPCWeb: GRPCWebConfig{
Enable: true,
Expand Down
5 changes: 5 additions & 0 deletions server/config/toml.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ max-recv-msg-size = "{{ .GRPC.MaxRecvMsgSize }}"
# The default value is math.MaxInt32.
max-send-msg-size = "{{ .GRPC.MaxSendMsgSize }}"

# LogQueries if enabled will print an info log containing the query request
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the following here to keep confix up to date: https://github.com/cosmos/cosmos-sdk/blob/main/tools/confix/data/v0.51-app.toml ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know this existed! Added here 8e56944

# that was submitted to this node on every submission.
# This is useful strictly for debugging purposes and should be disabled otherwise.
log-queries = {{ .GRPC.LogQueries }}

###############################################################################
### gRPC Web Configuration ###
###############################################################################
Expand Down
2 changes: 1 addition & 1 deletion server/grpc/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func NewGRPCServer(clientCtx client.Context, app types.Application, cfg config.G
grpc.MaxRecvMsgSize(maxRecvMsgSize),
)

app.RegisterGRPCServer(grpcSrv)
app.RegisterGRPCServer(grpcSrv, cfg.LogQueries)

// Reflection allows consumers to build dynamic clients that can write to any
// Cosmos SDK application without relying on application packages at compile
Expand Down
2 changes: 1 addition & 1 deletion server/types/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type (

// RegisterGRPCServer registers gRPC services directly with the gRPC
// server.
RegisterGRPCServer(grpc.Server)
RegisterGRPCServer(grpc.Server, bool)

// RegisterTxService registers the gRPC Query service for tx (such as tx
// simulation, fetching txs by hash...).
Expand Down
5 changes: 5 additions & 0 deletions tools/confix/data/v0.51-app.toml
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,11 @@ max-recv-msg-size = "10485760"
# The default value is math.MaxInt32.
max-send-msg-size = "2147483647"

# LogQueries if enabled will print an info log containing the query request
# that was submitted to this node on every submission.
# This is useful strictly for debugging purposes and should be disabled otherwise.
log-queries = false

###############################################################################
### gRPC Web Configuration ###
###############################################################################
Expand Down
Loading