From e0ce0795b44f27539611327efce7c7c004511daa Mon Sep 17 00:00:00 2001 From: pavel-raykov <165708424+pavel-raykov@users.noreply.github.com> Date: Fri, 21 Jun 2024 02:06:14 +0200 Subject: [PATCH] Add loggercheck linter to verify that \*w logging methods have even number of args. (#13599) * Add loggercheck linter to verify that \*w logging methods have even number of args. * Wrong changes. * Add .changeset file. * Add chainlink-common logger linting. * Remove non-existing logging functions from the linter checks. * Update the list of logging functions to be linted. * Remove wrong With linter check. * Set the .changeset tag to internal. * Use criticalf, instead of criticalw. --- .changeset/wise-wasps-drum.md | 5 +++ .golangci.yml | 33 +++++++++++++++++++ common/txmgr/confirmer.go | 4 +-- common/txmgr/txmgr.go | 2 +- .../evm/forwarders/forwarder_manager.go | 2 +- core/chains/evm/gas/rollups/op_l1_oracle.go | 2 +- .../evm/gas/suggested_price_estimator.go | 2 +- core/chains/evm/logpoller/log_poller.go | 2 +- core/chains/evm/txmgr/stuck_tx_detector.go | 2 +- core/services/feeds/service.go | 12 +++---- .../relay/evm/functions/logpoller_wrapper.go | 2 +- .../vrf/v2/listener_v2_log_processor.go | 2 +- 12 files changed, 54 insertions(+), 16 deletions(-) create mode 100644 .changeset/wise-wasps-drum.md diff --git a/.changeset/wise-wasps-drum.md b/.changeset/wise-wasps-drum.md new file mode 100644 index 00000000000..0a4a6eaf665 --- /dev/null +++ b/.changeset/wise-wasps-drum.md @@ -0,0 +1,5 @@ +--- +"chainlink": patch +--- + +#internal Add loggercheck linter to verify that \*w logging methods have even number of args. diff --git a/.golangci.yml b/.golangci.yml index 4f66d59ec2a..7155cff2d51 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -18,6 +18,7 @@ linters: - containedctx - fatcontext - mirror + - loggercheck linters-settings: exhaustive: default-signifies-exhaustive: true @@ -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 + - (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 @@ -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 diff --git a/common/txmgr/confirmer.go b/common/txmgr/confirmer.go index 294e922c1c0..a9e30ffff1e 100644 --- a/common/txmgr/confirmer.go +++ b/common/txmgr/confirmer.go @@ -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 { @@ -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 diff --git a/common/txmgr/txmgr.go b/common/txmgr/txmgr.go index 9cefa5c15ba..3ac0d2e1d68 100644 --- a/common/txmgr/txmgr.go +++ b/common/txmgr/txmgr.go @@ -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()) } } diff --git a/core/chains/evm/forwarders/forwarder_manager.go b/core/chains/evm/forwarders/forwarder_manager.go index 638836094dc..3624cdf7a2b 100644 --- a/core/chains/evm/forwarders/forwarder_manager.go +++ b/core/chains/evm/forwarders/forwarder_manager.go @@ -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) diff --git a/core/chains/evm/gas/rollups/op_l1_oracle.go b/core/chains/evm/gas/rollups/op_l1_oracle.go index 1c4db432cea..1977d139f9c 100644 --- a/core/chains/evm/gas/rollups/op_l1_oracle.go +++ b/core/chains/evm/gas/rollups/op_l1_oracle.go @@ -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 } diff --git a/core/chains/evm/gas/suggested_price_estimator.go b/core/chains/evm/gas/suggested_price_estimator.go index e947e9109d1..2c8798d6320 100644 --- a/core/chains/evm/gas/suggested_price_estimator.go +++ b/core/chains/evm/gas/suggested_price_estimator.go @@ -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") diff --git a/core/chains/evm/logpoller/log_poller.go b/core/chains/evm/logpoller/log_poller.go index 26978b18d48..1c8996a46ec 100644 --- a/core/chains/evm/logpoller/log_poller.go +++ b/core/chains/evm/logpoller/log_poller.go @@ -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 { diff --git a/core/chains/evm/txmgr/stuck_tx_detector.go b/core/chains/evm/txmgr/stuck_tx_detector.go index baadda8a5f9..1beb857af8f 100644 --- a/core/chains/evm/txmgr/stuck_tx_detector.go +++ b/core/chains/evm/txmgr/stuck_tx_detector.go @@ -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] diff --git a/core/services/feeds/service.go b/core/services/feeds/service.go index bab37c8e716..a58137eb11b 100644 --- a/core/services/feeds/service.go +++ b/core/services/feeds/service.go @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/core/services/relay/evm/functions/logpoller_wrapper.go b/core/services/relay/evm/functions/logpoller_wrapper.go index 4e37770f90e..559b1ec33f5 100644 --- a/core/services/relay/evm/functions/logpoller_wrapper.go +++ b/core/services/relay/evm/functions/logpoller_wrapper.go @@ -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{ diff --git a/core/services/vrf/v2/listener_v2_log_processor.go b/core/services/vrf/v2/listener_v2_log_processor.go index 673f8618c0b..145e72ed2bc 100644 --- a/core/services/vrf/v2/listener_v2_log_processor.go +++ b/core/services/vrf/v2/listener_v2_log_processor.go @@ -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{