Add log capitalization analyzer and apply changes (#15452)

* Add log capitalization analyzer and apply fixes across codebase

Implements a new nogo analyzer to enforce proper log message capitalization and applies the fixes to all affected log statements throughout the beacon chain, validator, and supporting components.

Co-Authored-By: Claude <noreply@anthropic.com>

* Radek's feedback

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
terence
2025-07-10 06:43:38 -07:00
committed by GitHub
parent 5c1d827335
commit 16b567f6af
71 changed files with 616 additions and 159 deletions

View File

@@ -0,0 +1,26 @@
load("@prysm//tools/go:def.bzl", "go_library", "go_test")
go_library(
name = "go_default_library",
srcs = ["analyzer.go"],
importpath = "github.com/OffchainLabs/prysm/v6/tools/analyzers/logcapitalization",
visibility = ["//visibility:public"],
deps = [
"@org_golang_x_tools//go/analysis:go_default_library",
"@org_golang_x_tools//go/analysis/passes/inspect:go_default_library",
"@org_golang_x_tools//go/ast/inspector:go_default_library",
],
)
go_test(
name = "go_default_test",
srcs = ["analyzer_test.go"],
data = glob(["testdata/**"]) + [
"@go_sdk//:files",
],
deps = [
":go_default_library",
"//build/bazel:go_default_library",
"@org_golang_x_tools//go/analysis/analysistest:go_default_library",
],
)

View File

@@ -0,0 +1,333 @@
// Package logcapitalization implements a static analyzer to ensure all log messages
// start with a capitalized letter for consistent log formatting.
package logcapitalization
import (
"errors"
"go/ast"
"go/token"
"strconv"
"strings"
"unicode"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/ast/inspector"
)
// Doc explaining the tool.
const Doc = "Tool to enforce that all log messages start with a capitalized letter"
var errLogNotCapitalized = errors.New("log message should start with a capitalized letter for consistent formatting")
// Analyzer runs static analysis.
var Analyzer = &analysis.Analyzer{
Name: "logcapitalization",
Doc: Doc,
Requires: []*analysis.Analyzer{inspect.Analyzer},
Run: run,
}
func run(pass *analysis.Pass) (interface{}, error) {
inspection, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
if !ok {
return nil, errors.New("analyzer is not type *inspector.Inspector")
}
nodeFilter := []ast.Node{
(*ast.CallExpr)(nil),
(*ast.File)(nil),
}
// Track imports that might be used for logging
hasLogImport := false
logPackageAliases := make(map[string]bool)
// Common logging functions that output messages
logFunctions := []string{
// logrus
"Info", "Infof", "InfoWithFields",
"Debug", "Debugf", "DebugWithFields",
"Warn", "Warnf", "WarnWithFields",
"Error", "ErrorWithFields",
"Fatal", "Fatalf", "FatalWithFields",
"Panic", "Panicf", "PanicWithFields",
"Print", "Printf", "Println",
"Log", "Logf",
// standard log
"Print", "Printf", "Println",
"Fatal", "Fatalf", "Fatalln",
"Panic", "Panicf", "Panicln",
// fmt excluded - often used for user prompts, not logging
}
inspection.Preorder(nodeFilter, func(node ast.Node) {
switch stmt := node.(type) {
case *ast.File:
// Reset per file
hasLogImport = false
logPackageAliases = make(map[string]bool)
// Check imports for logging packages
for _, imp := range stmt.Imports {
if imp.Path != nil {
path := strings.Trim(imp.Path.Value, "\"")
if isLoggingPackage(path) {
hasLogImport = true
// Track package alias
if imp.Name != nil {
logPackageAliases[imp.Name.Name] = true
} else {
// Default package name from path
parts := strings.Split(path, "/")
if len(parts) > 0 {
logPackageAliases[parts[len(parts)-1]] = true
}
}
}
}
}
case *ast.CallExpr:
if !hasLogImport {
return
}
// Check if this is a logging function call
if !isLoggingCall(stmt, logFunctions, logPackageAliases) {
return
}
// Check the first argument (message) for capitalization
if len(stmt.Args) > 0 {
firstArg := stmt.Args[0]
// Check if it's a format function (like Printf, Infof)
if isFormatFunction(stmt) {
checkFormatStringCapitalization(firstArg, pass, node)
} else {
checkMessageCapitalization(firstArg, pass, node)
}
}
}
})
return nil, nil
}
// isLoggingPackage checks if the import path is a logging package
func isLoggingPackage(path string) bool {
loggingPaths := []string{
"github.com/sirupsen/logrus",
"log",
"github.com/rs/zerolog",
"go.uber.org/zap",
"github.com/golang/glog",
"k8s.io/klog",
}
for _, logPath := range loggingPaths {
if strings.Contains(path, logPath) {
return true
}
}
return false
}
// isLoggingCall checks if the call expression is a logging function
func isLoggingCall(call *ast.CallExpr, logFunctions []string, aliases map[string]bool) bool {
var functionName string
var packageName string
switch fun := call.Fun.(type) {
case *ast.Ident:
// Direct function call
functionName = fun.Name
case *ast.SelectorExpr:
// Package.Function call
functionName = fun.Sel.Name
if ident, ok := fun.X.(*ast.Ident); ok {
packageName = ident.Name
}
default:
return false
}
// Check if it's a logging function
for _, logFunc := range logFunctions {
if functionName == logFunc {
// If no package specified, could be a logging call
if packageName == "" {
return true
}
// Check if package is a known logging package alias
if aliases[packageName] {
return true
}
// Check for common logging package names
if isCommonLogPackage(packageName) {
return true
}
}
}
return false
}
// isCommonLogPackage checks for common logging package names
func isCommonLogPackage(pkg string) bool {
common := []string{"log", "logrus", "zerolog", "zap", "glog", "klog"}
for _, c := range common {
if pkg == c {
return true
}
}
return false
}
// isFormatFunction checks if this is a format function (ending with 'f')
func isFormatFunction(call *ast.CallExpr) bool {
switch fun := call.Fun.(type) {
case *ast.Ident:
return strings.HasSuffix(fun.Name, "f")
case *ast.SelectorExpr:
return strings.HasSuffix(fun.Sel.Name, "f")
}
return false
}
// checkFormatStringCapitalization checks if format strings start with capital letter
func checkFormatStringCapitalization(expr ast.Expr, pass *analysis.Pass, node ast.Node) {
if basicLit, ok := expr.(*ast.BasicLit); ok && basicLit.Kind == token.STRING {
if len(basicLit.Value) >= 3 { // At least quotes + one character
unquoted, err := strconv.Unquote(basicLit.Value)
if err != nil {
return
}
if !isCapitalized(unquoted) {
pass.Reportf(expr.Pos(),
"%s: format string should start with a capital letter (found: %q)",
errLogNotCapitalized.Error(),
getFirstWord(unquoted))
}
}
}
}
// checkMessageCapitalization checks if message strings start with capital letter
func checkMessageCapitalization(expr ast.Expr, pass *analysis.Pass, node ast.Node) {
switch e := expr.(type) {
case *ast.BasicLit:
if e.Kind == token.STRING && len(e.Value) >= 3 {
unquoted, err := strconv.Unquote(e.Value)
if err != nil {
return
}
if !isCapitalized(unquoted) {
pass.Reportf(expr.Pos(),
"%s: log message should start with a capital letter (found: %q)",
errLogNotCapitalized.Error(),
getFirstWord(unquoted))
}
}
case *ast.BinaryExpr:
// For string concatenation, check the first part
if e.Op == token.ADD {
checkMessageCapitalization(e.X, pass, node)
}
}
}
// isCapitalized checks if a string starts with a capital letter
func isCapitalized(s string) bool {
if len(s) == 0 {
return true // Empty strings are OK
}
// Skip leading whitespace
trimmed := strings.TrimLeft(s, " \t\n\r")
if len(trimmed) == 0 {
return true // Only whitespace is OK
}
// Get the first character
firstRune := []rune(trimmed)[0]
// Check for special cases that are acceptable
if isAcceptableStart(firstRune, trimmed) {
return true
}
// Must be uppercase letter
return unicode.IsUpper(firstRune)
}
// isAcceptableStart checks for acceptable ways to start log messages
func isAcceptableStart(firstRune rune, s string) bool {
// Numbers are OK
if unicode.IsDigit(firstRune) {
return true
}
// Special characters that are OK to start with
acceptableChars := []rune{'%', '$', '/', '\\', '[', '(', '{', '"', '\'', '`', '-'}
for _, char := range acceptableChars {
if firstRune == char {
return true
}
}
// URLs/paths are OK
if strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://") || strings.HasPrefix(s, "file://") {
return true
}
// Command line flags are OK (--flag, -flag)
if strings.HasPrefix(s, "--") || (strings.HasPrefix(s, "-") && len(s) > 1 && unicode.IsLetter([]rune(s)[1])) {
return true
}
// Configuration keys or technical terms in lowercase are sometimes OK
if strings.Contains(s, "=") || strings.Contains(s, ":") {
// Looks like a key=value or key: value format
return true
}
// Technical keywords that are acceptable in lowercase
technicalKeywords := []string{"gRPC"}
// Check if the string starts with any technical keyword
lowerS := strings.ToLower(s)
for _, keyword := range technicalKeywords {
if strings.HasPrefix(lowerS, strings.ToLower(keyword)) {
return true
}
}
return false
}
// getFirstWord extracts the first few characters for error reporting
func getFirstWord(s string) string {
trimmed := strings.TrimLeft(s, " \t\n\r")
if len(trimmed) == 0 {
return s
}
words := strings.Fields(trimmed)
if len(words) > 0 {
if len(words[0]) > 20 {
return words[0][:20] + "..."
}
return words[0]
}
// Fallback to first 20 characters
if len(trimmed) > 20 {
return trimmed[:20] + "..."
}
return trimmed
}

View File

@@ -0,0 +1,21 @@
package logcapitalization_test
import (
"testing"
"golang.org/x/tools/go/analysis/analysistest"
"github.com/OffchainLabs/prysm/v6/build/bazel"
"github.com/OffchainLabs/prysm/v6/tools/analyzers/logcapitalization"
)
func init() {
if bazel.BuiltWithBazel() {
bazel.SetGoEnv()
}
}
func TestAnalyzer(t *testing.T) {
testdata := analysistest.TestData()
analysistest.RunWithSuggestedFixes(t, testdata, logcapitalization.Analyzer, "a")
}

View File

@@ -0,0 +1,65 @@
package testdata
import (
logrus "log" // Use standard log package as alias to simulate logrus
)
func BadCapitalization() {
// These should trigger the analyzer
logrus.Print("hello world") // want "log message should start with a capital letter"
logrus.Printf("starting the process") // want "format string should start with a capital letter"
// Simulating logrus-style calls
Info("connection failed") // want "log message should start with a capital letter"
Infof("failed to process %d blocks", 5) // want "format string should start with a capital letter"
Error("low disk space") // want "log message should start with a capital letter"
Debug("processing attestation") // want "log message should start with a capital letter"
// More examples
Warn("validator not found") // want "log message should start with a capital letter"
}
func GoodCapitalization() {
// These should NOT trigger the analyzer
logrus.Print("Hello world")
logrus.Printf("Starting the beacon chain process")
// Simulating logrus-style calls with proper capitalization
Info("Connection established successfully")
Infof("Processing %d blocks in epoch %d", 5, 100)
Error("Connection failed with timeout")
Errorf("Failed to process %d blocks", 5)
Warn("Low disk space detected")
Debug("Processing attestation for validator")
// Fun blockchain-specific examples with proper capitalization
Info("Validator activated successfully")
Info("New block mined with hash 0x123abc")
Info("Checkpoint finalized at epoch 50000")
Info("Sync committee duties assigned")
Info("Fork choice updated to new head")
// Acceptable edge cases - these should NOT trigger
Info("404 validator not found") // Numbers are OK
Info("/eth/v1/beacon/blocks endpoint") // Paths are OK
Info("config=mainnet") // Config format is OK
Info("https://beacon-node.example.com") // URLs are OK
Infof("%s network started", "mainnet") // Format specifiers are OK
Debug("--weak-subjectivity-checkpoint not provided") // CLI flags are OK
Debug("-v flag enabled") // Single dash flags are OK
Info("--datadir=/tmp/beacon") // Flags with values are OK
// Empty or whitespace
Info("") // Empty is OK
Info(" ") // Just whitespace is OK
}
// Mock logrus-style functions for testing
func Info(msg string) { logrus.Print(msg) }
func Infof(format string, args ...any) { logrus.Printf(format, args...) }
func Error(msg string) { logrus.Print(msg) }
func Errorf(format string, args ...any) { logrus.Printf(format, args...) }
func Warn(msg string) { logrus.Print(msg) }
func Warnf(format string, args ...any) { logrus.Printf(format, args...) }
func Debug(msg string) { logrus.Print(msg) }
func Debugf(format string, args ...any) { logrus.Printf(format, args...) }

View File

@@ -87,7 +87,7 @@ func main() {
// check if the database file is present.
dbNameWithPath := filepath.Join(*datadir, *dbName)
if _, err := os.Stat(dbNameWithPath); os.IsNotExist(err) {
log.WithError(err).WithField("path", dbNameWithPath).Fatal("could not locate database file")
log.WithError(err).WithField("path", dbNameWithPath).Fatal("Could not locate database file")
}
switch *command {
@@ -104,7 +104,7 @@ func main() {
case "migration-check":
destDbNameWithPath := filepath.Join(*destDatadir, *dbName)
if _, err := os.Stat(destDbNameWithPath); os.IsNotExist(err) {
log.WithError(err).WithField("path", destDbNameWithPath).Fatal("could not locate database file")
log.WithError(err).WithField("path", destDbNameWithPath).Fatal("Could not locate database file")
}
switch *migrationName {
case "validator-entries":
@@ -133,14 +133,14 @@ func printBucketContents(dbNameWithPath string, rowLimit uint64, bucketName stri
dbDirectory := filepath.Dir(dbNameWithPath)
db, openErr := kv.NewKVStore(context.Background(), dbDirectory)
if openErr != nil {
log.WithError(openErr).Fatal("could not open db")
log.WithError(openErr).Fatal("Could not open db")
}
// don't forget to close it when ejecting out of this function.
defer func() {
closeErr := db.Close()
if closeErr != nil {
log.WithError(closeErr).Fatal("could not close db")
log.WithError(closeErr).Fatal("Could not close db")
}
}()
@@ -166,14 +166,14 @@ func readBucketStat(dbNameWithPath string, statsC chan<- *bucketStat) {
// open the raw database file. If the file is busy, then exit.
db, openErr := bolt.Open(dbNameWithPath, 0600, &bolt.Options{Timeout: 1 * time.Second})
if openErr != nil {
log.WithError(openErr).Fatal("could not open db to show bucket stats")
log.WithError(openErr).Fatal("Could not open db to show bucket stats")
}
// make sure we close the database before ejecting out of this function.
defer func() {
closeErr := db.Close()
if closeErr != nil {
log.WithError(closeErr).Fatalf("could not close db after showing bucket stats")
log.WithError(closeErr).Fatalf("Could not close db after showing bucket stats")
}
}()
@@ -188,7 +188,7 @@ func readBucketStat(dbNameWithPath string, statsC chan<- *bucketStat) {
return nil
})
}); viewErr1 != nil {
log.WithError(viewErr1).Fatal("could not read buckets from db while getting list of buckets")
log.WithError(viewErr1).Fatal("Could not read buckets from db while getting list of buckets")
}
// for every bucket, calculate the stats and send it for printing.
@@ -258,7 +258,7 @@ func readStates(ctx context.Context, db *kv.Store, stateC chan<- *modifiedState,
for rowCount, key := range keys {
st, stateErr := db.State(ctx, bytesutil.ToBytes32(key))
if stateErr != nil {
log.WithError(stateErr).Errorf("could not get state for key : %s", hexutils.BytesToHex(key))
log.WithError(stateErr).Errorf("Could not get state for key : %s", hexutils.BytesToHex(key))
continue
}
mst := &modifiedState{
@@ -282,7 +282,7 @@ func readStateSummary(ctx context.Context, db *kv.Store, stateSummaryC chan<- *m
for rowCount, key := range keys {
ss, ssErr := db.StateSummary(ctx, bytesutil.ToBytes32(key))
if ssErr != nil {
log.WithError(ssErr).Errorf("could not get state summary for key : %s", hexutils.BytesToHex(key))
log.WithError(ssErr).Errorf("Could not get state summary for key : %s", hexutils.BytesToHex(key))
continue
}
mst := &modifiedStateSummary{
@@ -377,14 +377,14 @@ func checkValidatorMigration(dbNameWithPath, destDbNameWithPath string) {
destStateKeys, _ := keysOfBucket(destDbNameWithPath, []byte("state"), MaxUint64)
if len(destStateKeys) < len(sourceStateKeys) {
log.Fatalf("destination keys are lesser then source keys (%d/%d)", len(sourceStateKeys), len(destStateKeys))
log.Fatalf("Destination keys are lesser then source keys (%d/%d)", len(sourceStateKeys), len(destStateKeys))
}
// create the source and destination KV stores.
sourceDbDirectory := filepath.Dir(dbNameWithPath)
sourceDB, openErr := kv.NewKVStore(context.Background(), sourceDbDirectory)
if openErr != nil {
log.WithError(openErr).Fatal("could not open sourceDB")
log.WithError(openErr).Fatal("Could not open sourceDB")
}
destinationDbDirectory := filepath.Dir(destDbNameWithPath)
@@ -394,7 +394,7 @@ func checkValidatorMigration(dbNameWithPath, destDbNameWithPath string) {
// if you want to avoid this then we should pass the metric name when opening the DB which touches
// too many places.
if openErr.Error() != "duplicate metrics collector registration attempted" {
log.WithError(openErr).Fatalf("could not open sourceDB")
log.WithError(openErr).Fatalf("Could not open sourceDB")
}
}
@@ -402,13 +402,13 @@ func checkValidatorMigration(dbNameWithPath, destDbNameWithPath string) {
defer func() {
closeErr := sourceDB.Close()
if closeErr != nil {
log.WithError(closeErr).Fatal("could not close sourceDB")
log.WithError(closeErr).Fatal("Could not close sourceDB")
}
}()
defer func() {
closeErr := destDB.Close()
if closeErr != nil {
log.WithError(closeErr).Fatal("could not close sourceDB")
log.WithError(closeErr).Fatal("Could not close sourceDB")
}
}()
@@ -417,11 +417,11 @@ func checkValidatorMigration(dbNameWithPath, destDbNameWithPath string) {
for rowCount, key := range sourceStateKeys[910:] {
sourceState, stateErr := sourceDB.State(ctx, bytesutil.ToBytes32(key))
if stateErr != nil {
log.WithError(stateErr).WithField("key", hexutils.BytesToHex(key)).Fatalf("could not get from source db, the state for key")
log.WithError(stateErr).WithField("key", hexutils.BytesToHex(key)).Fatalf("Could not get from source db, the state for key")
}
destinationState, stateErr := destDB.State(ctx, bytesutil.ToBytes32(key))
if stateErr != nil {
log.WithError(stateErr).WithField("key", hexutils.BytesToHex(key)).Fatalf("could not get from destination db, the state for key")
log.WithError(stateErr).WithField("key", hexutils.BytesToHex(key)).Fatalf("Could not get from destination db, the state for key")
}
if destinationState == nil {
log.Infof("could not find state in migrated DB: index = %d, slot = %d, epoch = %d, numOfValidators = %d, key = %s",
@@ -435,11 +435,11 @@ func checkValidatorMigration(dbNameWithPath, destDbNameWithPath string) {
}
sourceStateHash, err := sourceState.HashTreeRoot(ctx)
if err != nil {
log.WithError(err).Fatal("could not find hash of source state")
log.WithError(err).Fatal("Could not find hash of source state")
}
destinationStateHash, err := destinationState.HashTreeRoot(ctx)
if err != nil {
log.WithError(err).Fatal("could not find hash of destination state")
log.WithError(err).Fatal("Could not find hash of destination state")
}
if !bytes.Equal(sourceStateHash[:], destinationStateHash[:]) {
log.Fatalf("state mismatch : key = %s", hexutils.BytesToHex(key))
@@ -452,14 +452,14 @@ func keysOfBucket(dbNameWithPath string, bucketName []byte, rowLimit uint64) ([]
// open the raw database file. If the file is busy, then exit.
db, openErr := bolt.Open(dbNameWithPath, 0600, &bolt.Options{Timeout: 1 * time.Second})
if openErr != nil {
log.WithError(openErr).Fatal("could not open db while getting keys of a bucket")
log.WithError(openErr).Fatal("Could not open db while getting keys of a bucket")
}
// make sure we close the database before ejecting out of this function.
defer func() {
closeErr := db.Close()
if closeErr != nil {
log.WithError(closeErr).Fatal("could not close db while getting keys of a bucket")
log.WithError(closeErr).Fatal("Could not close db while getting keys of a bucket")
}
}()
@@ -484,7 +484,7 @@ func keysOfBucket(dbNameWithPath string, bucketName []byte, rowLimit uint64) ([]
}
return nil
}); viewErr != nil {
log.WithError(viewErr).Fatal("could not read keys of bucket from db")
log.WithError(viewErr).Fatal("Could not read keys of bucket from db")
}
return keys, sizes
}

View File

@@ -49,7 +49,7 @@ func main() {
for _, endpt := range endpts {
conn, err := grpc.Dial(endpt, grpc.WithInsecure())
if err != nil {
log.WithError(err).Fatal("fail to dial")
log.WithError(err).Fatal("Fail to dial")
}
clients[endpt] = pb.NewBeaconChainClient(conn)
}

View File

@@ -18,7 +18,7 @@ import (
func mergeProfiles(p, merge *cover.Profile) {
if p.Mode != merge.Mode {
log.Fatalf("cannot merge profiles with different modes")
log.Fatalf("Cannot merge profiles with different modes")
}
// Since the blocks are sorted, we can keep track of where the last block
// was inserted and only look at the blocks after that as targets for merge
@@ -107,7 +107,7 @@ func main() {
for _, file := range flag.Args() {
profiles, err := cover.ParseProfiles(file)
if err != nil {
log.WithError(err).Fatal("failed to parse profiles")
log.WithError(err).Fatal("Failed to parse profiles")
}
for _, p := range profiles {
merged = addProfile(merged, p)

View File

@@ -393,7 +393,7 @@ func benchmarkHash(sszPath string, sszType string) {
runtime.ReadMemStats(stat)
root, err := stateTrieState.HashTreeRoot(context.Background())
if err != nil {
log.Fatal("couldn't hash")
log.Fatal("Couldn't hash")
}
newStat := &runtime.MemStats{}
runtime.ReadMemStats(newStat)