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.

Sign up for GitHub

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

Jump to bottom

feat: debug log gRPC queries #19049

Merged
merged 10 commits into from
Jan 26, 2024
Merged

feat: debug log gRPC queries #19049

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8b3db7c
feat: config option to log gRPC queries (#503)
czarcas7ic Jan 14, 2024
f6a0135
changelog
czarcas7ic Jan 14, 2024
e82a084
add API breaking changelog
czarcas7ic Jan 14, 2024
8e56944
add log-queries to confix v0.51 app.toml template
czarcas7ic Jan 14, 2024
69590db
Update toml.go
czarcas7ic Jan 14, 2024
fde2d27
Update grpcserver.go
czarcas7ic Jan 15, 2024
cf94348
no more config, just debug log
czarcas7ic Jan 21, 2024
50120cd
Merge branch 'main' into adam/grpc-logs
czarcas7ic Jan 21, 2024
1c00bb7
add baseapp module key to logger
czarcas7ic Jan 24, 2024
20e1822
lint fix
julienrbrt Jan 26, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
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 debug log prints for each gRPC request.

### Improvements

Expand Down
3 changes: 3 additions & 0 deletions baseapp/grpcserver.go
View file
Open in desktop
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 Down Expand Up @@ -67,6 +68,8 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) {
app.logger.Error("failed to set gRPC header", "err", err)
}

app.logger.Debug("gRPC query received of type: " + fmt.Sprintf("%#v", req))
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Given that the logger is passed directly from the app, maybe we should add a module key here:

cosmos-sdk/baseapp/baseapp.go

Line 196 in 2cde8df

logger: logger,
and maybe name it baseapp (logger.With(log.ModuleKey, "baseapp")). This will make the filtering easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, the app.logger should already have the module key set when it's set on BaseApp.

Copy link
Member

Choose a reason for hiding this comment

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

Works too, then it should be added in

cosmos-sdk/runtime/builder.go

Line 32 in 6d10c50

bApp := baseapp.NewBaseApp(a.app.config.AppName, a.app.logger, db, nil, baseAppOptions...)
and

cosmos-sdk/simapp/app.go

Line 251 in 6d10c50

bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), baseAppOptions...)

Copy link
Contributor

Choose a reason for hiding this comment

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

@czarcas7ic could you update this real quick and we'll get it merged 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @alexanderbez, I think this is what you guys were asking for 1c00bb7


return handler(grpcCtx, req)
}

Expand Down
3 changes: 2 additions & 1 deletion runtime/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"io"

"cosmossdk.io/log"
dbm "github.com/cosmos/cosmos-db"

"github.com/cosmos/cosmos-sdk/baseapp"
Expand All @@ -29,7 +30,7 @@ func (a *AppBuilder) Build(db dbm.DB, traceStore io.Writer, baseAppOptions ...fu
baseAppOptions = append(baseAppOptions, option)
}

bApp := baseapp.NewBaseApp(a.app.config.AppName, a.app.logger, db, nil, baseAppOptions...)
bApp := baseapp.NewBaseApp(a.app.config.AppName, a.app.logger.With(log.ModuleKey, "baseapp"), db, nil, baseAppOptions...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do it automatically in NewBaseApp itself

bApp.SetMsgServiceRouter(a.app.msgServiceRouter)
bApp.SetCommitMultiStoreTracer(traceStore)
bApp.SetVersion(version.Version)
Expand Down
2 changes: 1 addition & 1 deletion simapp/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ func NewSimApp(
}
baseAppOptions = append(baseAppOptions, voteExtOp, baseapp.SetOptimisticExecution())

bApp := baseapp.NewBaseApp(appName, logger, db, txConfig.TxDecoder(), baseAppOptions...)
bApp := baseapp.NewBaseApp(appName, logger.With(log.ModuleKey, "baseapp"), db, txConfig.TxDecoder(), baseAppOptions...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The verification process has identified that the logger with the module key "baseapp" is used in simapp/app.go and also in simapp/app_test.go. However, the usage in app_test.go uses a different key ("instance" instead of log.ModuleKey). This indicates that the logger is not used consistently with the module key "baseapp" across the application.

Analysis chain

The NewSimApp function has been modified to include a logger with a module key "baseapp". This change is consistent with the PR objectives and should enhance logging within the baseapp instance. Ensure that the logger is correctly instantiated and that the module key "baseapp" is used consistently across the application.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the logger with the module key "baseapp" is used consistently across the application.
ast-grep --lang go --pattern $'logger.With("ModuleKey", "baseapp")'

Length of output: 68

Script:

#!/bin/bash
# Search for any usage of logger.With with any string literal as the module key to check for "baseapp" usage.
ast-grep --lang go --pattern $'logger.With($_, "baseapp")'

Length of output: 332

bApp.SetCommitMultiStoreTracer(traceStore)
bApp.SetVersion(version.Version)
bApp.SetInterfaceRegistry(interfaceRegistry)
Expand Down
Loading