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

Add loggercheck linter to verify that \*w logging methods have even number of args. #13599

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/wise-wasps-drum.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": patch
---

#internal Add loggercheck linter to verify that \*w logging methods have even number of args.
33 changes: 33 additions & 0 deletions .golangci.yml
pavel-raykov marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ linters:
- containedctx
- fatcontext
- mirror
- loggercheck
linters-settings:
exhaustive:
default-signifies-exhaustive: true
Expand Down Expand Up @@ -47,6 +48,16 @@ linters-settings:
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Criticalf
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Panicf
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Fatalf
- (github.com/smartcontractkit/chainlink/v2/core/logger.SugaredLogger).AssumptionViolationf
jmank88 marked this conversation as resolved.
Show resolved Hide resolved
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Debugf
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Infof
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Warnf
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Errorf
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Panicf
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Fatalf
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).AssumptionViolationf
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).Tracef
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).Criticalf
errorlint:
# Allow formatting of errors without %w
errorf: false
Expand Down Expand Up @@ -127,6 +138,28 @@ linters-settings:
desc: Use gopkg.in/guregu/null.v4 instead
- pkg: github.com/go-gorm/gorm
desc: Use github.com/jmoiron/sqlx directly instead
loggercheck:
# Check that *w logging functions have even number of args (i.e., well formed key-value pairs).
rules:
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Tracew
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Debugw
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Infow
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Warnw
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Errorw
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Criticalw
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Panicw
- (github.com/smartcontractkit/chainlink/v2/core/logger.Logger).Fatalw
- (github.com/smartcontractkit/chainlink/v2/core/logger.SugaredLogger).AssumptionViolationw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Debugw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Infow
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Warnw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Errorw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Panicw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.Logger).Fatalw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).AssumptionViolationw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).Tracew
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).Criticalw
- (github.com/smartcontractkit/chainlink-common/pkg/logger.SugaredLogger).With
issues:
exclude-rules:
- path: test
Expand Down
4 changes: 2 additions & 2 deletions common/txmgr/confirmer.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) Che
}
}
if err != nil {
ec.lggr.Debugw("Batch sending transactions failed", err)
ec.lggr.Debugw("Batch sending transactions failed", "err", err)
}
var txIDsToUnconfirm []int64
for idx, txErr := range txErrs {
Expand Down Expand Up @@ -1160,7 +1160,7 @@ func (ec *Confirmer[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) For
continue
}
attempt.Tx = *etx // for logging
ec.lggr.Debugw("Sending transaction", "txAttemptID", attempt.ID, "txHash", attempt.Hash, "err", err, "meta", etx.Meta, "feeLimit", attempt.ChainSpecificFeeLimit, "callerProvidedFeeLimit", etx.FeeLimit, attempt)
ec.lggr.Debugw("Sending transaction", "txAttemptID", attempt.ID, "txHash", attempt.Hash, "err", err, "meta", etx.Meta, "feeLimit", attempt.ChainSpecificFeeLimit, "callerProvidedFeeLimit", etx.FeeLimit, "attempt", attempt)
if errCode, err := ec.client.SendTransactionReturnCode(ctx, *etx, attempt, ec.lggr); errCode != client.Successful && err != nil {
ec.lggr.Errorw(fmt.Sprintf("ForceRebroadcast: failed to rebroadcast tx %v with sequence %v, gas limit %v, and caller provided fee Limit %v : %s", etx.ID, *etx.Sequence, attempt.ChainSpecificFeeLimit, etx.FeeLimit, err.Error()), "err", err, "fee", attempt.TxFee)
continue
Expand Down
2 changes: 1 addition & 1 deletion common/txmgr/txmgr.go
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ func (b *Txm[CHAIN_ID, HEAD, ADDR, TX_HASH, BLOCK_HASH, R, SEQ, FEE]) CreateTran
txRequest.ToAddress = txRequest.ForwarderAddress
txRequest.EncodedPayload = fwdPayload
} else {
b.logger.Errorf("Failed to use forwarder set upstream: %w", fwdErr.Error())
b.logger.Errorf("Failed to use forwarder set upstream: %v", fwdErr.Error())
}
}

Expand Down
2 changes: 1 addition & 1 deletion core/chains/evm/forwarders/forwarder_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func (f *FwdMgr) initForwardersCache(ctx context.Context, fwdrs []Forwarder) {
for _, fwdr := range fwdrs {
senders, err := f.getAuthorizedSenders(ctx, fwdr.Address)
if err != nil {
f.logger.Warnw("Failed to call getAuthorizedSenders on forwarder", fwdr, "err", err)
f.logger.Warnw("Failed to call getAuthorizedSenders on forwarder", "forwarder", fwdr.Address, "err", err)
continue
}
f.setCachedSenders(fwdr.Address, senders)
Expand Down
2 changes: 1 addition & 1 deletion core/chains/evm/gas/rollups/op_l1_oracle.go
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ func (o *OptimismL1Oracle) checkIsEcotone(ctx context.Context) (bool, error) {

// if the chain has not upgraded to Ecotone, the isEcotone call will revert, this would be expected
if err != nil {
o.logger.Infof("isEcotone() call failed, this can happen if chain has not upgraded: %w", err)
o.logger.Infof("isEcotone() call failed, this can happen if chain has not upgraded: %v", err)
return false, nil
}

Expand Down
2 changes: 1 addition & 1 deletion core/chains/evm/gas/suggested_price_estimator.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func (o *SuggestedPriceEstimator) BumpLegacyGas(ctx context.Context, originalFee
err = pkgerrors.New("failed to refresh and return gas; gas price not set")
return
}
o.logger.Debugw("GasPrice", newGasPrice, "GasLimit", feeLimit)
o.logger.Debugw("BumpLegacyGas", "GasPrice", newGasPrice, "GasLimit", feeLimit)
})
if !ok {
return nil, 0, pkgerrors.New("estimator is not started")
Expand Down
2 changes: 1 addition & 1 deletion core/chains/evm/logpoller/log_poller.go
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,7 @@ func (lp *logPoller) getCurrentBlockMaybeHandleReorg(ctx context.Context, curren
}
// Additional sanity checks, don't necessarily trust the RPC.
if currentBlock == nil {
lp.lggr.Errorf("Unexpected nil block from RPC", "currentBlockNumber", currentBlockNumber)
lp.lggr.Errorw("Unexpected nil block from RPC", "currentBlockNumber", currentBlockNumber)
return nil, pkgerrors.Errorf("Got nil block for %d", currentBlockNumber)
}
if currentBlock.Number != currentBlockNumber {
Expand Down
2 changes: 1 addition & 1 deletion core/chains/evm/txmgr/stuck_tx_detector.go
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ func (d *stuckTxDetector) detectStuckTransactionsZkEVM(ctx context.Context, txs
for i, req := range txReqs {
txHash := req.Args[0].(common.Hash)
if req.Error != nil {
d.lggr.Debugf("failed to get transaction by hash (%s): %w", txHash.String(), req.Error)
d.lggr.Debugf("failed to get transaction by hash (%s): %v", txHash.String(), req.Error)
continue
}
result := *txRes[i]
Expand Down
12 changes: 6 additions & 6 deletions core/services/feeds/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ func (s *service) DeleteJob(ctx context.Context, args *DeleteJobArgs) (int64, er
}

if err = s.observeJobProposalCounts(ctx); err != nil {
logger.Errorw("Failed to push metrics for job proposal deletion", err)
logger.Errorw("Failed to push metrics for job proposal deletion", "err", err)
}

return proposal.ID, nil
Expand Down Expand Up @@ -518,7 +518,7 @@ func (s *service) RevokeJob(ctx context.Context, args *RevokeJobArgs) (int64, er
)

if err = s.observeJobProposalCounts(ctx); err != nil {
logger.Errorw("Failed to push metrics for revoke job", err)
logger.Errorw("Failed to push metrics for revoke job", "err", err)
}

return proposal.ID, nil
Expand Down Expand Up @@ -632,7 +632,7 @@ func (s *service) ProposeJob(ctx context.Context, args *ProposeJobArgs) (int64,
}

if err = s.observeJobProposalCounts(ctx); err != nil {
logger.Errorw("Failed to push metrics for propose job", err)
logger.Errorw("Failed to push metrics for propose job", "err", err)
}

return id, nil
Expand Down Expand Up @@ -704,7 +704,7 @@ func (s *service) RejectSpec(ctx context.Context, id int64) error {
}

if err = s.observeJobProposalCounts(ctx); err != nil {
logger.Errorw("Failed to push metrics for job rejection", err)
logger.Errorw("Failed to push metrics for job rejection", "err", err)
}

return nil
Expand Down Expand Up @@ -883,7 +883,7 @@ func (s *service) ApproveSpec(ctx context.Context, id int64, force bool) error {
}

if err = s.observeJobProposalCounts(ctx); err != nil {
logger.Errorw("Failed to push metrics for job approval", err)
logger.Errorw("Failed to push metrics for job approval", "err", err)
}

return nil
Expand Down Expand Up @@ -973,7 +973,7 @@ func (s *service) CancelSpec(ctx context.Context, id int64) error {
}

if err = s.observeJobProposalCounts(ctx); err != nil {
logger.Errorw("Failed to push metrics for job cancellation", err)
logger.Errorw("Failed to push metrics for job cancellation", "err", err)
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion core/services/relay/evm/functions/logpoller_wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func (l *logPollerWrapper) LatestEvents(ctx context.Context) ([]evmRelayTypes.Or
oracleRequest.Commitment.TimeoutTimestamp,
)
if err != nil {
l.lggr.Errorw("LatestEvents: failed to pack commitment bytes, skipping", err)
l.lggr.Errorw("LatestEvents: failed to pack commitment bytes, skipping", "err", err)
}

resultsReq = append(resultsReq, evmRelayTypes.OracleRequest{
Expand Down
2 changes: 1 addition & 1 deletion core/services/vrf/v2/listener_v2_log_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (lsn *listenerV2) processPendingVRFRequests(ctx context.Context, pendingReq
)
sID, ok := new(big.Int).SetString(subID, 10)
if !ok {
l.Criticalw("Unable to convert %s to Int", subID)
l.Criticalf("Unable to convert %s to Int", subID)
return
}
sub, err := lsn.coordinator.GetSubscription(&bind.CallOpts{
Expand Down
Loading