Skip to content

Commit

Permalink
server: report the SQL instance ID as log tag sqli
Browse files Browse the repository at this point in the history
Prior to this patch, the SQL instance ID was not reported in log
messages, which made it hard to distinguish log messages coming from
different in-memory SQL instance servers (e.g. in tests).

This patch fixes it.

The patch also changes the log tag for SQL instances from just `sql`
to `sqli`, which is easier to search for and recognize.

Release note: None
  • Loading branch information
knz committed Nov 10, 2021
1 parent 2de17e7 commit 75257ac
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 3 deletions.
1 change: 1 addition & 0 deletions docs/generated/redact_safe.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ The following types are considered always safe for reporting:
File | Type
--|--
pkg/base/node_id.go | `*NodeIDContainer`
pkg/base/node_id.go | `*SQLIDContainer`
pkg/base/node_id.go | `*StoreIDContainer`
pkg/cli/exit/exit.go | `Code`
pkg/jobs/jobspb/wrap.go | `JobID`
Expand Down
47 changes: 46 additions & 1 deletion pkg/base/node_id.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,21 @@ func (s *StoreIDContainer) Set(ctx context.Context, val int32) {
type SQLInstanceID int32

func (s SQLInstanceID) String() string {
return strconv.FormatInt(int64(s), 10)
if s == 0 {
return "?"
}
return strconv.Itoa(int(s))
}

// SQLIDContainer wraps a SQLInstanceID and optionally a NodeID.
type SQLIDContainer struct {
w errorutil.TenantSQLDeprecatedWrapper // NodeID
sqlInstanceID SQLInstanceID

// If the value has been set, str represents the instance ID
// converted to string. We precompute this value to speed up
// String() and keep it from allocating memory dynamically.
str atomic.Value
}

// NewSQLIDContainer sets up an SQLIDContainer. It is handed either a positive SQLInstanceID
Expand Down Expand Up @@ -196,6 +204,7 @@ func (c *SQLIDContainer) SetSQLInstanceID(sqlInstanceID SQLInstanceID) error {
return errors.New("attempting to initialize instance ID when node ID is set")
}
c.sqlInstanceID = sqlInstanceID
c.str.Store(strconv.Itoa(int(sqlInstanceID)))
return nil
}

Expand Down Expand Up @@ -227,6 +236,42 @@ func (c *SQLIDContainer) SQLInstanceID() SQLInstanceID {
return c.sqlInstanceID
}

// SafeValue implements the redact.SafeValue interface.
func (s *SQLIDContainer) SafeValue() {}

func (s *SQLIDContainer) String() string {
st := s.str.Load()
if st == nil {
// This can mean either that:
// - neither the instance ID nor the node ID has been set.
// - only the node ID has been set.
//
// If the lattercase, we don't want to return "?" here, as in the
// NodeIDContainer case above: we want to return the node ID
// representation instead. Alas, there is no way to know
// but to open the node ID container box.
//
// TODO(knz): This could be greatly simplified if we accepted to
// use the same data type for both SQL instance ID and node ID
// containers.
v, ok := s.w.Optional()
if !ok {
// No node ID either (yet).
return "?"
}
nc := v.(*NodeIDContainer)
st = nc.str.Load()
if st == nil {
// Node ID not set yet.
return "?"
}
// Node ID was set. Keep its representation for the instance ID as
// well.
s.str.Store(st)
}
return st.(string)
}

// TestingIDContainer is an SQLIDContainer with hard-coded SQLInstanceID of 10 and
// NodeID of 1.
var TestingIDContainer = func() *SQLIDContainer {
Expand Down
12 changes: 10 additions & 2 deletions pkg/server/tenant.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,15 @@ func makeTenantSQLServerArgs(
stopper *stop.Stopper, kvClusterName string, baseCfg BaseConfig, sqlCfg SQLConfig,
) (sqlServerArgs, error) {
st := baseCfg.Settings
baseCfg.AmbientCtx.AddLogTag("sql", nil)

// We want all log messages issued on behalf of this SQL instance to report
// the instance ID (once known) as a tag.
instanceIDContainer := base.NewSQLIDContainer(0, nil)
// We use the tag "sqli" instead of just "sql" because the latter is
// too generic and would be hard to search if someone was looking at
// a log message and wondering what it stands for.
baseCfg.AmbientCtx.AddLogTag("sqli", instanceIDContainer)

// TODO(tbg): this is needed so that the RPC heartbeats between the testcluster
// and this tenant work.
//
Expand Down Expand Up @@ -511,7 +519,7 @@ func makeTenantSQLServerArgs(
externalStorageFromURI: externalStorageFromURI,
// Set instance ID to 0 and node ID to nil to indicate
// that the instance ID will be bound later during preStart.
nodeIDContainer: base.NewSQLIDContainer(0, nil),
nodeIDContainer: instanceIDContainer,
},
sqlServerOptionalTenantArgs: sqlServerOptionalTenantArgs{
tenantConnect: tenantConnect,
Expand Down

0 comments on commit 75257ac

Please sign in to comment.