Skip to content

Commit 9dfdf1b

Browse files
craig[bot]adityamaruerikgrinakermaryliagotan
committed
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 <adityamaru@gmail.com> Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Marylia Gutierrez <marylia@cockroachlabs.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com> Co-authored-by: Xin Hao Zhang <xzhang@cockroachlabs.com> Co-authored-by: Jay <jay@cockroachlabs.com> Co-authored-by: Sean Barag <barag@cockroachlabs.com>
9 parents 2808905 + 4ddc350 + 202b112 + 78b723d + 77f8930 + 7c57bce + 8e46e5c + 0b696bb + ec32cbb commit 9dfdf1b

33 files changed

Lines changed: 955 additions & 220 deletions

File tree

‎pkg/ccl/sqlproxyccl/proxy_handler_test.go‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1280,10 +1280,8 @@ func TestConnectionMigration(t *testing.T) {
12801280
conn, err := db.Conn(tCtx)
12811281
require.NoError(t, err)
12821282

1283-
// Spin up a goroutine to trigger the initial connection.
1284-
go func() {
1285-
_ = conn.PingContext(tCtx)
1286-
}()
1283+
// Trigger the initial connection.
1284+
require.NoError(t, conn.PingContext(tCtx))
12871285

12881286
var f *forwarder
12891287
require.Eventually(t, func() bool {
@@ -1333,16 +1331,10 @@ func TestConnectionMigration(t *testing.T) {
13331331
// one test.
13341332
<-goCh
13351333
time.Sleep(2 * time.Second)
1336-
// This should be an error because the transfer timed out.
1334+
// This should be an error because the transfer timed out. Connection
1335+
// should automatically be closed.
13371336
require.Error(t, f.TransferConnection())
13381337

1339-
// Connection should be closed because this is a non-recoverable error,
1340-
// i.e. timeout after sending the request, but before fully receiving
1341-
// its response.
1342-
err = conn.PingContext(tCtx)
1343-
require.Error(t, err)
1344-
require.Regexp(t, "(closed|bad connection)", err.Error())
1345-
13461338
select {
13471339
case <-time.After(10 * time.Second):
13481340
t.Fatalf("require that pg_sleep query terminates")

‎pkg/cmd/roachtest/tests/awsdms.go‎

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -487,22 +487,55 @@ func setupDMSEndpointsAndTask(
487487
}
488488
*ep.arn = *epOut.Endpoint.EndpointArn
489489

490+
// Test the connections to see if they are "successful".
491+
// If not, any subsequence DMS task will fail to startup.
490492
t.L().Printf("testing replication endpoint %s", *ep.in.EndpointIdentifier)
493+
if _, err := dmsCli.TestConnection(ctx, &dms.TestConnectionInput{
494+
EndpointArn: epOut.Endpoint.EndpointArn,
495+
ReplicationInstanceArn: proto.String(replicationARN),
496+
}); err != nil {
497+
return errors.Wrapf(err, "error initiating a test connection")
498+
}
491499
r := retry.StartWithCtx(ctx, retry.Options{
492500
InitialBackoff: 30 * time.Second,
493501
MaxBackoff: time.Minute,
494502
MaxRetries: 10,
495503
})
496504
var lastErr error
497505
for r.Next() {
498-
_, lastErr = dmsCli.TestConnection(ctx, &dms.TestConnectionInput{
499-
EndpointArn: epOut.Endpoint.EndpointArn,
500-
ReplicationInstanceArn: proto.String(replicationARN),
501-
})
502-
if lastErr == nil {
506+
if lastErr = func() error {
507+
result, err := dmsCli.DescribeConnections(
508+
ctx,
509+
&dms.DescribeConnectionsInput{
510+
Filters: []dmstypes.Filter{
511+
{
512+
Name: proto.String("endpoint-arn"),
513+
Values: []string{*epOut.Endpoint.EndpointArn},
514+
},
515+
},
516+
},
517+
)
518+
if err != nil {
519+
return err
520+
}
521+
if len(result.Connections) != 1 {
522+
return errors.AssertionFailedf("expected exactly one connection during DescribeConnections, found %d", len(result.Connections))
523+
}
524+
conn := result.Connections[0]
525+
if *conn.Status == "successful" {
526+
return nil
527+
}
528+
retErr := errors.Newf(
529+
"replication test on %s not successful (%s)",
530+
*ep.in.EndpointIdentifier,
531+
*conn.Status,
532+
)
533+
return retErr
534+
}(); lastErr == nil {
503535
break
536+
} else {
537+
t.L().Printf("replication endpoint test failed, retrying: %s", lastErr)
504538
}
505-
t.L().Printf("replication endpoint test failed, retrying: %s", lastErr)
506539
}
507540
if lastErr != nil {
508541
return lastErr

‎pkg/server/node_tenant_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,7 @@ func TestRedactRecordingForTenant(t *testing.T) {
128128
GoroutineID uint64
129129
Finished bool
130130
StructuredRecords []tracingpb.StructuredRecord
131+
ChildrenMetadata map[string]tracingpb.OperationMetadata
131132
}
132133
_ = (*calcifiedRecordedSpan)((*tracingpb.RecordedSpan)(nil))
133134
})

‎pkg/storage/pebble.go‎

Lines changed: 51 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -325,25 +325,66 @@ var MVCCMerger = &pebble.Merger{
325325
},
326326
}
327327

328-
// pebbleDataBlockMVCCTimeIntervalCollector provides an implementation of
328+
// pebbleDataBlockMVCCTimeIntervalPointCollector implements
329+
// pebble.DataBlockIntervalCollector for point keys.
330+
type pebbleDataBlockMVCCTimeIntervalPointCollector struct {
331+
pebbleDataBlockMVCCTimeIntervalCollector
332+
}
333+
334+
var (
335+
_ sstable.DataBlockIntervalCollector = (*pebbleDataBlockMVCCTimeIntervalPointCollector)(nil)
336+
_ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalPointCollector)(nil)
337+
)
338+
339+
func (tc *pebbleDataBlockMVCCTimeIntervalPointCollector) Add(
340+
key pebble.InternalKey, _ []byte,
341+
) error {
342+
return tc.add(key.UserKey)
343+
}
344+
345+
// pebbleDataBlockMVCCTimeIntervalRangeCollector implements
346+
// pebble.DataBlockIntervalCollector for range keys.
347+
type pebbleDataBlockMVCCTimeIntervalRangeCollector struct {
348+
pebbleDataBlockMVCCTimeIntervalCollector
349+
}
350+
351+
var (
352+
_ sstable.DataBlockIntervalCollector = (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil)
353+
_ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalRangeCollector)(nil)
354+
)
355+
356+
func (tc *pebbleDataBlockMVCCTimeIntervalRangeCollector) Add(
357+
key pebble.InternalKey, value []byte,
358+
) error {
359+
// TODO(erikgrinaker): should reuse a buffer for keysDst, but keyspan.Key is
360+
// not exported by Pebble.
361+
span, err := sstable.DecodeRangeKey(key, value, nil)
362+
if err != nil {
363+
return errors.Wrapf(err, "decoding range key at %s", key)
364+
}
365+
for _, k := range span.Keys {
366+
if err := tc.add(k.Suffix); err != nil {
367+
return errors.Wrapf(err, "recording suffix %x for range key at %s", k.Suffix, key)
368+
}
369+
}
370+
return nil
371+
}
372+
373+
// pebbleDataBlockMVCCTimeIntervalCollector is a helper for a
329374
// pebble.DataBlockIntervalCollector that is used to construct a
330375
// pebble.BlockPropertyCollector. This provides per-block filtering, which
331376
// also gets aggregated to the sstable-level and filters out sstables. It must
332377
// only be used for MVCCKeyIterKind iterators, since it will ignore
333378
// blocks/sstables that contain intents (and any other key that is not a real
334379
// MVCC key).
380+
//
381+
// This is wrapped by structs for point or range key collection, which actually
382+
// implement pebble.DataBlockIntervalCollector.
335383
type pebbleDataBlockMVCCTimeIntervalCollector struct {
336384
// min, max are the encoded timestamps.
337385
min, max []byte
338386
}
339387

340-
var _ sstable.DataBlockIntervalCollector = &pebbleDataBlockMVCCTimeIntervalCollector{}
341-
var _ sstable.SuffixReplaceableBlockCollector = (*pebbleDataBlockMVCCTimeIntervalCollector)(nil)
342-
343-
func (tc *pebbleDataBlockMVCCTimeIntervalCollector) Add(key pebble.InternalKey, _ []byte) error {
344-
return tc.add(key.UserKey)
345-
}
346-
347388
// add collects the given slice in the collector. The slice may be an entire
348389
// encoded MVCC key, or the bare suffix of an encoded key.
349390
func (tc *pebbleDataBlockMVCCTimeIntervalCollector) add(b []byte) error {
@@ -431,8 +472,8 @@ var PebbleBlockPropertyCollectors = []func() pebble.BlockPropertyCollector{
431472
func() pebble.BlockPropertyCollector {
432473
return sstable.NewBlockIntervalCollector(
433474
mvccWallTimeIntervalCollector,
434-
&pebbleDataBlockMVCCTimeIntervalCollector{}, /* points */
435-
nil, /* ranges */
475+
&pebbleDataBlockMVCCTimeIntervalPointCollector{},
476+
&pebbleDataBlockMVCCTimeIntervalRangeCollector{},
436477
)
437478
},
438479
}

‎pkg/storage/pebble_iterator.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,11 @@ func (p *pebbleIterator) setOptions(opts IterOptions, durability DurabilityRequi
239239
uint64(opts.MinTimestampHint.WallTime),
240240
uint64(opts.MaxTimestampHint.WallTime)+1),
241241
}
242+
p.options.RangeKeyFilters = []pebble.BlockPropertyFilter{
243+
sstable.NewBlockIntervalFilter(mvccWallTimeIntervalCollector,
244+
uint64(opts.MinTimestampHint.WallTime),
245+
uint64(opts.MaxTimestampHint.WallTime)+1),
246+
}
242247
}
243248

244249
// Set the new iterator options. We unconditionally do so, since Pebble will

0 commit comments

Comments
 (0)