Merge #81079 #82667 #83107 #83400 #83423 #83427 #83457 #83460

81079: tracing: aggregate OperationMetadata on span Finish() r=andreimatei a=adityamaru

This change adds a `ChildrenMetadata` map to `crdbspan` that is a
mapping from operation to the operations' aggregated metadata. This
map is updated whenever a child of the `crdbSpan` finishes, with metadata
from all spans in the finishing childs' Recording. The map is therefore
a bucketed view of all the operations being traced by a span.

The motivation for this change is to surface more metadata about the
suboperations being traced in a spans' Recording. This could in turn provide
more o11y into why a job is slow/stuck, or where the performance of a
distributed operation is bottlenecked.

As part of a span Finish()ing, the span fetches its Recording with the spans'
configured verbosity. Prior to this change the recording would then be
processed as follows:

*Verbose Recording*

In the case of Verbose recording the spans in the recording are added to the parents'
`finishedChildren` slice provided we have not exceeded the maximum number of
children a parent can track.

*Structured Recording*

In the case of a Structured recording, only the StructuredEvents from the spans in
the recording are copied into the parent.

With this change, in both the Verbose and Structured recording mode, a finishing span
is also responsible for rolling up the OperationMetadata of all the spans in its
recording. This involves updating the parents' `childrenMetadata` mapping with:

1) an entry for the finishing span.
2) an entry for each of the finishing spans' Finish()ed children.
3) an entry for each of the finishing spans' open children, and their children recursively

The logic for 2) and 3) is subsumed in the method responsible for getting
the finishing spans' recording. Notably, GetRecording(...) for both Structured and Verbose
recordings, populate the root of the recording with OperationMetadata of all
finished and open children in the recording.

As an example when we are done finishing `child`:

```
parent
  child (finished_C: 4s, finished_D: 3s)
    open_A (finished_B: 1s)
      finished_B
    finished_C (finished_D: 3s)
      finished_D
```

We'd expect `parent` to have:
`{child: 10s, finished_C: 4s, finished_D: 3s, open_A: 3s, finished_B: 1s}`

Given that Finish()ing a child, and importing a remote recording into a span
share the same code path, the above semantics also apply to a remote recording
being imported into a parent span.

Fixes: #80391

Release note: None

82667: storage: add `MVCCTimeInterval` block property for range keys r=jbowens a=erikgrinaker

This patch adds `MVCCTimeInternal` block property collection and
filtering for range keys, which allows using time-bound iterators with
range keys.

Range keys will only be written once the `MVCCRangeTombstones` version
gate is enabled.

Resolves #82596.

Release note: None

83107: ui: make Metrics and SQL timepicker align r=maryliag a=maryliag

Previously, the timepicker from Metrics page and
the timepicker on SQL Activity pages acted independently.
Now, if the value of one changes, the other value changes
to the same period selected.

This commit also fixes a bug where the period selected
would change to a custom value if the Metrics page was
refreshed.

Fixes #78187
Fixes #82152

Release note (ui change): The period selected on the Metrics
page and the SQL Activity pages are now aligned. If the user
changes in one page, the value will be the same for the other.

Release note (bug fix): The period selected on Metrics page 
continues the same when refreshing the page, no longer changing 
to a custom period.

83400: awsdms: further deflake roachtest r=rafiss a=otan

Once the connection is tested, we also have to ensure the status of
the DMS endpoint connection is successful before continuing.
Otherwise DMS may fail to startup.

Resolves #83369

Release note: None

83423: cluster-ui/ui: remove ability to search statements by plan r=xinhaoz a=xinhaoz

Closes  #83155

Previously, we allowed statements in the statements page to
searchable by text in the explain plan. This was before we
returned multiple plans for a statement fingerprint. This commit
removes the explain plan text as part of the searchable string, as
this feature could  now lead to confusing behaviour.

Release note (ui change): In the statements page, users can no
longer filter statements by searching for text in the EXPLAIN
plan.

83427: ui: update labels on Session Details page r=maryliag a=maryliag

Update labels so all of them use the same
format.

Fixes #80350

Release note: None

83457: ccl/sqlproxyccl: fix TestConnectionMigration test flake r=JeffSwenson,rafiss a=jaylim-crl

Fixes #83096.

It appears that database/sql does not provide any thread-safe guarantees for
methods on the Conn type except the Close method. This commit fixes a test-only
issue that causes a panic when there's a race between internal Conn methods
by ensuring that Conn methods are used in a single-threaded way.

Release note: None

Release justification: sqlproxy only test change.

83460: ui: explain eslint plugin prequisite r=laurenbarker a=sjbarag

A recent commit [1] introduced a custom eslint plugin that's hosted in
this repo, but didn't add documentation around building that plugin to
resolve errors reported in IDEs. Explain that eslint-plugin-crdb should
be built to silence errors from eslint that get reported in editors.

[1] ba68179 (ui: use esbuild-loader in webpack configs, 2022-05-26)

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Jay <[email protected]>
Co-authored-by: Sean Barag <[email protected]>

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -1280,10 +1280,8 @@ func TestConnectionMigration(t *testing.T) {
 		conn, err := db.Conn(tCtx)
 		require.NoError(t, err)
 
-		// Spin up a goroutine to trigger the initial connection.
-		go func() {
-			_ = conn.PingContext(tCtx)
-		}()
+		// Trigger the initial connection.
+		require.NoError(t, conn.PingContext(tCtx))
 
 		var f *forwarder
 		require.Eventually(t, func() bool {
@@ -1333,16 +1331,10 @@ func TestConnectionMigration(t *testing.T) {
 		// one test.
 		<-goCh
 		time.Sleep(2 * time.Second)
-		// This should be an error because the transfer timed out.
+		// This should be an error because the transfer timed out. Connection
+		// should automatically be closed.
 		require.Error(t, f.TransferConnection())
 
-		// Connection should be closed because this is a non-recoverable error,
-		// i.e. timeout after sending the request, but before fully receiving
-		// its response.
-		err = conn.PingContext(tCtx)
-		require.Error(t, err)
-		require.Regexp(t, "(closed|bad connection)", err.Error())
-
 		select {
 		case <-time.After(10 * time.Second):
 			t.Fatalf("require that pg_sleep query terminates")

pkg/cmd/roachtest/tests/awsdms.go

@@ -487,22 +487,55 @@ func setupDMSEndpointsAndTask(
 		}
 		*ep.arn = *epOut.Endpoint.EndpointArn
 
+		// Test the connections to see if they are "successful".
+		// If not, any subsequence DMS task will fail to startup.
 		t.L().Printf("testing replication endpoint %s", *ep.in.EndpointIdentifier)
+		if _, err := dmsCli.TestConnection(ctx, &dms.TestConnectionInput{
+			EndpointArn:            epOut.Endpoint.EndpointArn,
+			ReplicationInstanceArn: proto.String(replicationARN),
+		}); err != nil {
+			return errors.Wrapf(err, "error initiating a test connection")
+		}
 		r := retry.StartWithCtx(ctx, retry.Options{
 			InitialBackoff: 30 * time.Second,
 			MaxBackoff:     time.Minute,
 			MaxRetries:     10,
 		})
 		var lastErr error
 		for r.Next() {
-			_, lastErr = dmsCli.TestConnection(ctx, &dms.TestConnectionInput{
-				EndpointArn:            epOut.Endpoint.EndpointArn,
-				ReplicationInstanceArn: proto.String(replicationARN),
-			})
-			if lastErr == nil {
+			if lastErr = func() error {
+				result, err := dmsCli.DescribeConnections(
+					ctx,
+					&dms.DescribeConnectionsInput{
+						Filters: []dmstypes.Filter{
+							{
+								Name:   proto.String("endpoint-arn"),
+								Values: []string{*epOut.Endpoint.EndpointArn},
+							},
+						},
+					},
+				)
+				if err != nil {
+					return err
+				}
+				if len(result.Connections) != 1 {
+					return errors.AssertionFailedf("expected exactly one connection during DescribeConnections, found %d", len(result.Connections))
+				}
+				conn := result.Connections[0]
+				if *conn.Status == "successful" {
+					return nil
+				}
+				retErr := errors.Newf(
+					"replication test on %s not successful (%s)",
+					*ep.in.EndpointIdentifier,
+					*conn.Status,
+				)
+				return retErr
+			}(); lastErr == nil {
 				break
+			} else {
+				t.L().Printf("replication endpoint test failed, retrying: %s", lastErr)
 			}
-			t.L().Printf("replication endpoint test failed, retrying: %s", lastErr)
 		}
 		if lastErr != nil {
 			return lastErr

pkg/server/node_tenant_test.go

@@ -128,6 +128,7 @@ func TestRedactRecordingForTenant(t *testing.T) {
 			GoroutineID       uint64
 			Finished          bool
 			StructuredRecords []tracingpb.StructuredRecord
+			ChildrenMetadata  map[string]tracingpb.OperationMetadata
 		}
 		_ = (*calcifiedRecordedSpan)((*tracingpb.RecordedSpan)(nil))
 	})

pkg/storage/pebble.go

@@ -325,25 +325,66 @@ var MVCCMerger = &pebble.Merger{
 	},
 }
 
-// pebbleDataBlockMVCCTimeIntervalCollector provides an implementation of
+// pebbleDataBlockMVCCTimeIntervalPointCollector implements
+// pebble.DataBlockIntervalCollector for point keys.
+type pebbleDataBlockMVCCTimeIntervalPointCollector struct {
+	pebbleDataBlockMVCCTimeIntervalCollector
+}
+
+var (
+	_ sstable.DataBlockIntervalCollector      = (*pebbleDataBlockMVCCTimeIntervalPointCollector)(nil)
+	_ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalPointCollector)(nil)
+)
+
+func (tc *pebbleDataBlockMVCCTimeIntervalPointCollector) Add(
+	key pebble.InternalKey, _ []byte,
+) error {
+	return tc.add(key.UserKey)
+}
+
+// pebbleDataBlockMVCCTimeIntervalRangeCollector implements
+// pebble.DataBlockIntervalCollector for range keys.
+type pebbleDataBlockMVCCTimeIntervalRangeCollector struct {
+	pebbleDataBlockMVCCTimeIntervalCollector
+}
+
+var (
+	_ sstable.DataBlockIntervalCollector      = (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil)
+	_ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil)
+)
+
+func (tc *pebbleDataBlockMVCCTimeIntervalRangeCollector) Add(
+	key pebble.InternalKey, value []byte,
+) error {
+	// TODO(erikgrinaker): should reuse a buffer for keysDst, but keyspan.Key is
+	// not exported by Pebble.
+	span, err := sstable.DecodeRangeKey(key, value, nil)
+	if err != nil {
+		return errors.Wrapf(err, "decoding range key at %s", key)
+	}
+	for _, k := range span.Keys {
+		if err := tc.add(k.Suffix); err != nil {
+			return errors.Wrapf(err, "recording suffix %x for range key at %s", k.Suffix, key)
+		}
+	}
+	return nil
+}
+
+// pebbleDataBlockMVCCTimeIntervalCollector is a helper for a
 // pebble.DataBlockIntervalCollector that is used to construct a
 // pebble.BlockPropertyCollector. This provides per-block filtering, which
 // also gets aggregated to the sstable-level and filters out sstables. It must
 // only be used for MVCCKeyIterKind iterators, since it will ignore
 // blocks/sstables that contain intents (and any other key that is not a real
 // MVCC key).
+//
+// This is wrapped by structs for point or range key collection, which actually
+// implement pebble.DataBlockIntervalCollector.
 type pebbleDataBlockMVCCTimeIntervalCollector struct {
 	// min, max are the encoded timestamps.
 	min, max []byte
 }
 
-var _ sstable.DataBlockIntervalCollector = &pebbleDataBlockMVCCTimeIntervalCollector{}
-var _ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalCollector)(nil)
-
-func (tc *pebbleDataBlockMVCCTimeIntervalCollector) Add(key pebble.InternalKey, _ []byte) error {
-	return tc.add(key.UserKey)
-}
-
 // add collects the given slice in the collector. The slice may be an entire
 // encoded MVCC key, or the bare suffix of an encoded key.
 func (tc *pebbleDataBlockMVCCTimeIntervalCollector) add(b []byte) error {
@@ -431,8 +472,8 @@ var PebbleBlockPropertyCollectors = []func() pebble.BlockPropertyCollector{
 	func() pebble.BlockPropertyCollector {
 		return sstable.NewBlockIntervalCollector(
 			mvccWallTimeIntervalCollector,
-			&pebbleDataBlockMVCCTimeIntervalCollector{}, /* points */
-			nil, /* ranges */
+			&pebbleDataBlockMVCCTimeIntervalPointCollector{},
+			&pebbleDataBlockMVCCTimeIntervalRangeCollector{},
 		)
 	},
 }

pkg/storage/pebble_iterator.go

@@ -239,6 +239,11 @@ func (p *pebbleIterator) setOptions(opts IterOptions, durability DurabilityRequi
 				uint64(opts.MinTimestampHint.WallTime),
 				uint64(opts.MaxTimestampHint.WallTime)+1),
 		}
+		p.options.RangeKeyFilters = []pebble.BlockPropertyFilter{
+			sstable.NewBlockIntervalFilter(mvccWallTimeIntervalCollector,
+				uint64(opts.MinTimestampHint.WallTime),
+				uint64(opts.MaxTimestampHint.WallTime)+1),
+		}
 	}
 
 	// Set the new iterator options. We unconditionally do so, since Pebble will