Merge #52718 #53147

52718: *: ensure that log and error messages start with lowercase r=spaskob,irfansharif a=knz

This commit adjusts a few log and error calls to ensure the texts
start with a lowercase letter. (Messages that start with a code
identifier keep their capital if any - these are treated as proper
nouns.)

Good: `log.Infof(ctx, "this is my message: %s", err)`
Less good: `log.Info(ctx, "This is my message: %s", err)`

Good: `errors.Wrap(err, "some context")`
Less good: `errors.Wrap(err, "Some context")`

This aligns the code with the prevalent convention valid throughout
the project. The reason for adjusting this now is that log entries
will soon receive additional scrutiny from users in the shape of
documentation. We want the UX to be consistent throughout all logging
and error outputs.

A linter may be introduced later to enforce this convention.

For context, why we use lowercase:

Both log and error messages are always reported to users in a text
format after a colon, for example, `file.go:123: this is the log
message`. 
In English, random capital letters are not allowed in the
middle of sentences (unlike in German) and distract from the flow of
reading.

This is particularly important for error objects, which can receive an
arbitrary amount of text prefixes using `errors.Wrap()` in
callers. Additionally, error objects can be included inside a larger
sentence, for example `while handling "this is my error"`. Here, as
well, capitals are not welcome after an opening quote in the middle of
a sentence.

Release note: None

53147: opt: teach buildScan how to use inverted histograms r=mjibson a=mjibson

Teach stats builder how to detect statistics on inverted
index virtual columns that are already correctly being used by
buildScan. invertedConstrainScan can then filter the histograms by the
inverted constraint.

Fixes #48220

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Matt Jibson <[email protected]>

pkg/ccl/cmdccl/enc_utils/main.go

@@ -90,7 +90,7 @@ func loadFileRegistry() {
 
 func loadStoreKey() {
 	if len(*storeKeyPath) == 0 || *storeKeyPath == "plain" {
-		log.Infof(context.Background(), "No store key specified")
+		log.Infof(context.Background(), "no store key specified")
 		return
 	}
 

pkg/cli/debug.go

@@ -998,7 +998,7 @@ func removeDeadReplicas(
 		removeDeadReplicasOpts.deadStoreIDs)
 
 	if _, ok := deadStoreIDs[storeIdent.StoreID]; ok {
-		return nil, errors.Errorf("This store's ID (%s) marked as dead, aborting", storeIdent.StoreID)
+		return nil, errors.Errorf("this store's ID (%s) marked as dead, aborting", storeIdent.StoreID)
 	}
 
 	var newDescs []roachpb.RangeDescriptor

pkg/cli/demo_cluster.go

@@ -699,7 +699,7 @@ func (c *transientCluster) runWorkload(
 						// Only log an error and return when the workload function throws
 						// an error, because errors these errors should be ignored, and
 						// should not interrupt the rest of the demo.
-						log.Warningf(ctx, "Error running workload query: %+v\n", err)
+						log.Warningf(ctx, "error running workload query: %+v", err)
 						return
 					}
 				}

pkg/cli/node.go

@@ -321,7 +321,7 @@ func runDecommissionNode(cmd *cobra.Command, args []string) error {
 
 	conn, _, finish, err := getClientGRPCConn(ctx, serverCfg)
 	if err != nil {
-		return errors.Wrap(err, "Failed to connect to the node")
+		return errors.Wrap(err, "failed to connect to the node")
 	}
 	defer finish()
 
@@ -544,7 +544,7 @@ func runRecommissionNode(cmd *cobra.Command, args []string) error {
 
 	conn, _, finish, err := getClientGRPCConn(ctx, serverCfg)
 	if err != nil {
-		return errors.Wrap(err, "Failed to connect to the node")
+		return errors.Wrap(err, "failed to connect to the node")
 	}
 	defer finish()
 

pkg/cli/quit.go

@@ -248,7 +248,7 @@ func doShutdown(ctx context.Context, c serverpb.AdminClient) (hardError bool, er
 func getAdminClient(ctx context.Context, cfg server.Config) (serverpb.AdminClient, func(), error) {
 	conn, _, finish, err := getClientGRPCConn(ctx, cfg)
 	if err != nil {
-		return nil, nil, errors.Wrap(err, "Failed to connect to the node")
+		return nil, nil, errors.Wrap(err, "failed to connect to the node")
 	}
 	return serverpb.NewAdminClient(conn), finish, nil
 }

pkg/cli/systembench/disk_bench.go

@@ -69,7 +69,7 @@ func newWorkerSeqWrite(
 	data := make([]byte, diskOptions.WriteSize)
 	_, err := rand.Read(data)
 	if err != nil {
-		log.Fatalf(ctx, "Failed to fill byte array with random bytes %s", err)
+		log.Fatalf(ctx, "failed to fill byte array with random bytes %s", err)
 	}
 
 	w := &workerSeqWrite{data: data,

pkg/geo/geohash.go

@@ -19,7 +19,7 @@ import (
 // using a Lng/Lat Point representation of the GeoHash.
 func NewGeometryPointFromGeoHash(g string, precision int) (*Geometry, error) {
 	if len(g) == 0 {
-		return nil, errors.Newf("Length of geohash must be greater than 0")
+		return nil, errors.Newf("length of geohash must be greater than 0")
 	}
 
 	// If precision is more than the length of the geohash

pkg/gossip/gossip.go

@@ -388,7 +388,7 @@ func NewTestWithLocality(
 // AssertNotStarted fatals if the Gossip instance was already started.
 func (g *Gossip) AssertNotStarted(ctx context.Context) {
 	if g.started {
-		log.Fatalf(ctx, "Gossip instance was already started")
+		log.Fatalf(ctx, "gossip instance was already started")
 	}
 }
 

pkg/jobs/job_scheduler.go

@@ -277,7 +277,7 @@ func (s *jobScheduler) executeSchedules(
 func (s *jobScheduler) runDaemon(ctx context.Context, stopper *stop.Stopper) {
 	stopper.RunWorker(ctx, func(ctx context.Context) {
 		initialDelay := getInitialScanDelay(s.TestingKnobs)
-		log.Infof(ctx, "Waiting %s before scheduled jobs daemon start", initialDelay.String())
+		log.Infof(ctx, "waiting %s before scheduled jobs daemon start", initialDelay.String())
 
 		for timer := time.NewTimer(initialDelay); ; timer.Reset(
 			getWaitPeriod(&s.Settings.SV, s.TestingKnobs)) {
@@ -286,7 +286,7 @@ func (s *jobScheduler) runDaemon(ctx context.Context, stopper *stop.Stopper) {
 				return
 			case <-timer.C:
 				if !schedulerEnabledSetting.Get(&s.Settings.SV) {
-					log.Info(ctx, "Scheduled job daemon disabled")
+					log.Info(ctx, "scheduled job daemon disabled")
 					continue
 				}
 

pkg/jobs/jobs.go

@@ -327,8 +327,8 @@ func (j *Job) FractionProgressed(ctx context.Context, progressedFn FractionProgr
 		}
 		if fractionCompleted < 0.0 || fractionCompleted > 1.0 {
 			return errors.Errorf(
-				"Job: fractionCompleted %f is outside allowable range [0.0, 1.0] (job %d)",
-				fractionCompleted, *j.ID(),
+				"job %d: fractionCompleted %f is outside allowable range [0.0, 1.0]",
+				*j.ID(), fractionCompleted,
 			)
 		}
 		md.Progress.Progress = &jobspb.Progress_FractionCompleted{
@@ -353,8 +353,8 @@ func (j *Job) HighWaterProgressed(ctx context.Context, progressedFn HighWaterPro
 		}
 		if highWater.Less(hlc.Timestamp{}) {
 			return errors.Errorf(
-				"Job: high-water %s is outside allowable range > 0.0 (job %d)",
-				highWater, *j.ID(),
+				"job %d: high-water %s is outside allowable range > 0.0",
+				*j.ID(), highWater,
 			)
 		}
 		md.Progress.Progress = &jobspb.Progress_HighWater{
@@ -591,7 +591,7 @@ func (j *Job) succeeded(ctx context.Context, fn func(context.Context, *kv.Txn) e
 			return nil
 		}
 		if md.Status != StatusRunning && md.Status != StatusPending {
-			return errors.Errorf("Job with status %s cannot be marked as succeeded", md.Status)
+			return errors.Errorf("job with status %s cannot be marked as succeeded", md.Status)
 		}
 		if fn != nil {
 			if err := fn(ctx, txn); err != nil {
@@ -807,7 +807,7 @@ func UnmarshalPayload(datum tree.Datum) (*jobspb.Payload, error) {
 	bytes, ok := datum.(*tree.DBytes)
 	if !ok {
 		return nil, errors.Errorf(
-			"Job: failed to unmarshal payload as DBytes (was %T)", datum)
+			"job: failed to unmarshal payload as DBytes (was %T)", datum)
 	}
 	if err := protoutil.Unmarshal([]byte(*bytes), payload); err != nil {
 		return nil, err
@@ -822,7 +822,7 @@ func UnmarshalProgress(datum tree.Datum) (*jobspb.Progress, error) {
 	bytes, ok := datum.(*tree.DBytes)
 	if !ok {
 		return nil, errors.Errorf(
-			"Job: failed to unmarshal Progress as DBytes (was %T)", datum)
+			"job: failed to unmarshal Progress as DBytes (was %T)", datum)
 	}
 	if err := protoutil.Unmarshal([]byte(*bytes), progress); err != nil {
 		return nil, err
@@ -841,10 +841,10 @@ func unmarshalCreatedBy(createdByType, createdByID tree.Datum) (*CreatedByInfo,
 			return &CreatedByInfo{Name: string(*ds), ID: int64(*id)}, nil
 		}
 		return nil, errors.Errorf(
-			"Job: failed to unmarshal created_by_type as DInt (was %T)", createdByID)
+			"job: failed to unmarshal created_by_type as DInt (was %T)", createdByID)
 	}
 	return nil, errors.Errorf(
-		"Job: failed to unmarshal created_by_type as DString (was %T)", createdByType)
+		"job: failed to unmarshal created_by_type as DString (was %T)", createdByType)
 }
 
 // CurrentStatus returns the current job status from the jobs table or error.

pkg/jobs/registry.go

@@ -603,7 +603,7 @@ func (r *Registry) Start(
 			log.Errorf(ctx, "error getting live session: %s", err)
 			return
 		}
-		log.VEventf(ctx, 1, "Registry live claim (instance_id: %s, sid: %s)", r.ID(), s.ID())
+		log.VEventf(ctx, 1, "registry live claim (instance_id: %s, sid: %s)", r.ID(), s.ID())
 
 		if _, err := r.ex.QueryRowEx(
 			ctx, "expire-sessions", nil,

pkg/jobs/update.go

@@ -99,7 +99,7 @@ func (ju *JobUpdater) hasUpdates() bool {
 // defined in jobs.go.
 func (j *Job) Update(ctx context.Context, updateFn UpdateFn) error {
 	if j.id == nil {
-		return errors.New("Job: cannot update: job not created")
+		return errors.New("job: cannot update: job not created")
 	}
 
 	var payload *jobspb.Payload
@@ -118,7 +118,7 @@ func (j *Job) Update(ctx context.Context, updateFn UpdateFn) error {
 
 		statusString, ok := row[0].(*tree.DString)
 		if !ok {
-			return errors.Errorf("Job: expected string status on job %d, but got %T", *j.id, statusString)
+			return errors.AssertionFailedf("job %d: expected string status, but got %T", *j.id, statusString)
 		}
 		status := Status(*statusString)
 		if payload, err = UnmarshalPayload(row[1]); err != nil {
@@ -194,7 +194,7 @@ func (j *Job) Update(ctx context.Context, updateFn UpdateFn) error {
 		}
 		if n != 1 {
 			return errors.Errorf(
-				"Job: expected exactly one row affected, but %d rows affected by job update", n,
+				"job %d: expected exactly one row affected, but %d rows affected by job update", *j.id, n,
 			)
 		}
 		return nil

pkg/kv/bulk/buffering_adder.go

@@ -128,7 +128,9 @@ func MakeBulkAdder(
 	// it will store in-memory before sending it to RocksDB.
 	b.memAcc = bulkMon.MakeBoundAccount()
 	if err := b.memAcc.Grow(ctx, b.curBufferSize); err != nil {
-		return nil, errors.Wrap(err, "Not enough memory available to create a BulkAdder. Try setting a higher --max-sql-memory.")
+		return nil, errors.WithHint(
+			errors.Wrap(err, "not enough memory available to create a BulkAdder"),
+			"Try setting a higher --max-sql-memory.")
 	}
 
 	return b, nil

pkg/kv/kvserver/tenantrate/limiter.go

@@ -195,7 +195,7 @@ func (rb *tokenBuckets) Merge(val interface{}) (shouldNotify bool) {
 		// further in the future.
 		return false
 	default:
-		panic(errors.AssertionFailedf("Merge not implemented for %T", val))
+		panic(errors.AssertionFailedf("merge not implemented for %T", val))
 	}
 }
 

pkg/rpc/context.go

@@ -952,7 +952,7 @@ func (ctx *Context) grpcDialRaw(
 	dialerFunc := dialer.dial
 	if ctx.Knobs.ArtificialLatencyMap != nil {
 		latency := ctx.Knobs.ArtificialLatencyMap[target]
-		log.VEventf(ctx.masterCtx, 1, "Connecting to node %s (%d) with simulated latency %dms", target, remoteNodeID,
+		log.VEventf(ctx.masterCtx, 1, "connecting to node %s (%d) with simulated latency %dms", target, remoteNodeID,
 			latency)
 		dialer := artificialLatencyDialer{
 			dialerFunc: dialerFunc,

pkg/security/certs.go

@@ -181,7 +181,7 @@ func createCACertAndKey(
 			return errors.Errorf("could not write CA key to file %s: %v", caKeyPath, err)
 		}
 
-		log.Infof(context.Background(), "Generated CA key %s", caKeyPath)
+		log.Infof(context.Background(), "generated CA key %s", caKeyPath)
 	} else {
 		if !allowKeyReuse {
 			return errors.Errorf("CA key %s exists, but key reuse is disabled", caKeyPath)
@@ -197,7 +197,7 @@ func createCACertAndKey(
 			return errors.Errorf("could not parse CA key file %s: %v", caKeyPath, err)
 		}
 
-		log.Infof(context.Background(), "Using CA key from file %s", caKeyPath)
+		log.Infof(context.Background(), "using CA key from file %s", caKeyPath)
 	}
 
 	// Generate certificate.
@@ -233,7 +233,7 @@ func createCACertAndKey(
 		if err != nil {
 			return errors.Errorf("could not parse existing CA cert file %s: %v", certPath, err)
 		}
-		log.Infof(context.Background(), "Found %d certificates in %s",
+		log.Infof(context.Background(), "found %d certificates in %s",
 			len(existingCertificates), certPath)
 	} else if !os.IsNotExist(err) {
 		return errors.Errorf("could not stat CA cert file %s: %v", certPath, err)
@@ -247,7 +247,7 @@ func createCACertAndKey(
 		return errors.Errorf("could not write CA certificate file %s: %v", certPath, err)
 	}
 
-	log.Infof(context.Background(), "Wrote %d certificates to %s", len(certificates), certPath)
+	log.Infof(context.Background(), "wrote %d certificates to %s", len(certificates), certPath)
 
 	return nil
 }
@@ -300,13 +300,13 @@ func CreateNodePair(
 	if err := writeCertificateToFile(certPath, nodeCert, overwrite); err != nil {
 		return errors.Errorf("error writing node server certificate to %s: %v", certPath, err)
 	}
-	log.Infof(context.Background(), "Generated node certificate: %s", certPath)
+	log.Infof(context.Background(), "generated node certificate: %s", certPath)
 
 	keyPath := cm.NodeKeyPath()
 	if err := writeKeyToFile(keyPath, nodeKey, overwrite); err != nil {
 		return errors.Errorf("error writing node server key to %s: %v", keyPath, err)
 	}
-	log.Infof(context.Background(), "Generated node key: %s", keyPath)
+	log.Infof(context.Background(), "generated node key: %s", keyPath)
 
 	return nil
 }
@@ -355,13 +355,13 @@ func CreateUIPair(
 	if err := writeCertificateToFile(certPath, uiCert, overwrite); err != nil {
 		return errors.Errorf("error writing UI server certificate to %s: %v", certPath, err)
 	}
-	log.Infof(context.Background(), "Generated UI certificate: %s", certPath)
+	log.Infof(context.Background(), "generated UI certificate: %s", certPath)
 
 	keyPath := cm.UIKeyPath()
 	if err := writeKeyToFile(keyPath, uiKey, overwrite); err != nil {
 		return errors.Errorf("error writing UI server key to %s: %v", keyPath, err)
 	}
-	log.Infof(context.Background(), "Generated UI key: %s", keyPath)
+	log.Infof(context.Background(), "generated UI key: %s", keyPath)
 
 	return nil
 }
@@ -426,20 +426,20 @@ func CreateClientPair(
 	if err := writeCertificateToFile(certPath, clientCert, overwrite); err != nil {
 		return errors.Errorf("error writing client certificate to %s: %v", certPath, err)
 	}
-	log.Infof(context.Background(), "Generated client certificate: %s", certPath)
+	log.Infof(context.Background(), "generated client certificate: %s", certPath)
 
 	keyPath := cm.ClientKeyPath(user)
 	if err := writeKeyToFile(keyPath, clientKey, overwrite); err != nil {
 		return errors.Errorf("error writing client key to %s: %v", keyPath, err)
 	}
-	log.Infof(context.Background(), "Generated client key: %s", keyPath)
+	log.Infof(context.Background(), "generated client key: %s", keyPath)
 
 	if wantPKCS8Key {
 		pkcs8KeyPath := keyPath + ".pk8"
 		if err := writePKCS8KeyToFile(pkcs8KeyPath, clientKey, overwrite); err != nil {
 			return errors.Errorf("error writing client PKCS8 key to %s: %v", pkcs8KeyPath, err)
 		}
-		log.Infof(context.Background(), "Generated PKCS8 client key: %s", pkcs8KeyPath)
+		log.Infof(context.Background(), "generated PKCS8 client key: %s", pkcs8KeyPath)
 	}
 
 	return nil
@@ -523,13 +523,13 @@ func WriteTenantClientPair(certsDir string, cp *TenantClientPair, overwrite bool
 	if err := writeCertificateToFile(certPath, cp.Cert, overwrite); err != nil {
 		return errors.Errorf("error writing tenant certificate to %s: %v", certPath, err)
 	}
-	log.Infof(context.Background(), "Wrote SQL tenant client certificate: %s", certPath)
+	log.Infof(context.Background(), "wrote SQL tenant client certificate: %s", certPath)
 
 	keyPath := cm.TenantClientKeyPath(tenantIdentifier)
 	if err := writeKeyToFile(keyPath, cp.PrivateKey, overwrite); err != nil {
 		return errors.Errorf("error writing tenant key to %s: %v", keyPath, err)
 	}
-	log.Infof(context.Background(), "Generated tenant key: %s", keyPath)
+	log.Infof(context.Background(), "generated tenant key: %s", keyPath)
 	return nil
 }
 

pkg/server/authentication.go

@@ -394,7 +394,7 @@ func (am *authenticationMux) ServeHTTP(w http.ResponseWriter, req *http.Request)
 		ctx = context.WithValue(ctx, webSessionIDKey{}, cookie.ID)
 		req = req.WithContext(ctx)
 	} else if !am.allowAnonymous {
-		log.Infof(req.Context(), "Web session error: %s", err)
+		log.Infof(req.Context(), "web session error: %s", err)
 		http.Error(w, "a valid authentication cookie is required", http.StatusUnauthorized)
 		return
 	}

pkg/server/node.go

@@ -356,7 +356,7 @@ func (n *Node) start(
 		select {
 		case <-n.storeCfg.Gossip.Connected:
 		default:
-			log.Fatalf(ctx, "Gossip is not connected yet")
+			log.Fatalf(ctx, "gossip is not connected yet")
 		}
 		ctxWithSpan, span := n.AnnotateCtxWithSpan(ctx, "alloc-node-id")
 		newID, err := allocateNodeID(ctxWithSpan, n.storeCfg.DB)

pkg/server/status/recorder.go

@@ -464,7 +464,7 @@ func (mr *MetricsRecorder) GenerateNodeStatus(ctx context.Context) *statuspb.Nod
 		// Gather descriptor from store.
 		descriptor, err := mr.mu.stores[storeID].Descriptor(ctx, false /* useCached */)
 		if err != nil {
-			log.Errorf(ctx, "Could not record status summaries: Store %d could not return descriptor, error: %s", storeID, err)
+			log.Errorf(ctx, "could not record status summaries: Store %d could not return descriptor, error: %s", storeID, err)
 			continue
 		}
 

pkg/server/status/runtime.go

@@ -307,7 +307,7 @@ func NewRuntimeStatSampler(ctx context.Context, clock *hlc.Clock) *RuntimeStatSa
 	timestamp, err := info.Timestamp()
 	if err != nil {
 		// We can't panic here, tests don't have a build timestamp.
-		log.Warningf(ctx, "Could not parse build timestamp: %v", err)
+		log.Warningf(ctx, "could not parse build timestamp: %v", err)
 	}
 
 	// Build information.

pkg/server/updates.go

@@ -227,7 +227,7 @@ func (s *Server) checkForUpdates(ctx context.Context) bool {
 
 	err = decoder.Decode(&r)
 	if err != nil && err != io.EOF {
-		log.Warningf(ctx, "Error decoding updates info: %v", err)
+		log.Warningf(ctx, "error decoding updates info: %v", err)
 		return false
 	}
 
@@ -238,7 +238,7 @@ func (s *Server) checkForUpdates(ctx context.Context) bool {
 		r.Details = r.Details[len(r.Details)-updateMaxVersionsToReport:]
 	}
 	for _, v := range r.Details {
-		log.Infof(ctx, "A new version is available: %s, details: %s", v.Version, v.Details)
+		log.Infof(ctx, "a new version is available: %s, details: %s", v.Version, v.Details)
 	}
 	return true
 }

pkg/sql/app_stats.go

@@ -550,7 +550,7 @@ func dumpStmtStats(ctx context.Context, appName string, stats map[stmtKey]*stmtS
 		}
 		fmt.Fprintf(&buf, "%q: %s\n", key.String(), json)
 	}
-	log.Infof(ctx, "Statistics for %q:\n%s", appName, buf.String())
+	log.Infof(ctx, "statistics for %q:\n%s", appName, buf.String())
 }
 
 func constructStatementIDFromStmtKey(key stmtKey) roachpb.StmtID {