From 7c42e3514b0943f80beed0996beb2ae5973b4e8b Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Fri, 3 Feb 2023 11:02:57 -0500 Subject: [PATCH 1/3] Revert "sql: improve stack trace for get-user-timeout timeouts" This reverts commit 662e19c291607cc7668de24bd20906d30c571713. gRPC does note expect the context to be wrapped, so it can get confused. Release note: None --- pkg/util/contextutil/context.go | 13 ------------- pkg/util/contextutil/context_test.go | 19 ------------------- 2 files changed, 32 deletions(-) diff --git a/pkg/util/contextutil/context.go b/pkg/util/contextutil/context.go index 87a139437153..db7dbb03597c 100644 --- a/pkg/util/contextutil/context.go +++ b/pkg/util/contextutil/context.go @@ -79,18 +79,6 @@ func wrap(ctx context.Context, cancel context.CancelFunc) (context.Context, cont } } -// ctxWithStacktrace overrides Err to annotate context.DeadlineExceeded and -// context.Canceled errors with a stacktrace. -// See: https://github.com/cockroachdb/cockroach/issues/95794 -type ctxWithStacktrace struct { - context.Context -} - -// Err implements the context.Context interface. -func (ctx *ctxWithStacktrace) Err() error { - return errors.WithStack(ctx.Context.Err()) -} - // RunWithTimeout runs a function with a timeout, the same way you'd do with // context.WithTimeout. It improves the opaque error messages returned by // WithTimeout by augmenting them with the op string that is passed in. @@ -98,7 +86,6 @@ func RunWithTimeout( ctx context.Context, op string, timeout time.Duration, fn func(ctx context.Context) error, ) error { ctx, cancel := context.WithTimeout(ctx, timeout) - ctx = &ctxWithStacktrace{Context: ctx} defer cancel() start := timeutil.Now() err := fn(ctx) diff --git a/pkg/util/contextutil/context_test.go b/pkg/util/contextutil/context_test.go index f84748f5a535..f313f8f540df 100644 --- a/pkg/util/contextutil/context_test.go +++ b/pkg/util/contextutil/context_test.go @@ -12,7 +12,6 @@ package contextutil import ( "context" - "fmt" "net" "testing" "time" @@ -71,24 +70,6 @@ func TestRunWithTimeout(t *testing.T) { } } -func testFuncA(ctx context.Context) error { - return testFuncB(ctx) -} - -func testFuncB(ctx context.Context) error { - <-ctx.Done() - return ctx.Err() -} - -func TestRunWithTimeoutCtxWithStacktrace(t *testing.T) { - ctx := context.Background() - err := RunWithTimeout(ctx, "foo", 1, testFuncA) - require.Error(t, err) - stacktrace := fmt.Sprintf("%+v", err) - require.Contains(t, stacktrace, "testFuncB") - require.Contains(t, stacktrace, "testFuncA") -} - // TestRunWithTimeoutWithoutDeadlineExceeded ensures that when a timeout on the // context occurs but the underlying error does not have // context.DeadlineExceeded as its Cause (perhaps due to serialization) the From 3168f399ac0328baf4a2940285d94cc9223571bb Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Fri, 3 Feb 2023 13:51:09 -0400 Subject: [PATCH 2/3] pkg/util/metric: fix HistogramMode for streamingingest histograms Ref: https://github.com/cockroachdb/cockroach/pull/96514 The above patch migrated all histogram constructors over to use a generic constructor, which has the ability to specify a HistogramMode. While preparing a backport of this patch, I noticed that the following streamingest histograms were incorrectly tagged with a HistogramMode of HistogramModePreferHdrLatency: 1. FlushHistNanos 2. CommitLatency 3. AdmitLatency If you look at the original migration, when HDR was still being used, these histograms were *not* using the HDR latency defaults: https://github.com/cockroachdb/cockroach/commit/a82aa8267b1b33a685a02a0f73ec2ece1f8ba295#diff-1bc5bdba63149e8efeadce17e7eb62bb5cd1dcee22974b37881a627e13c0501dL137-L143 This patch fixes these histograms to no longer incorrectly specify the HistogramModePreferHdrLatency mode in the histogram options. This was also fixed in the backport PR. Release note: none --- pkg/ccl/streamingccl/streamingest/metrics.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/ccl/streamingccl/streamingest/metrics.go b/pkg/ccl/streamingccl/streamingest/metrics.go index c2858cd5ad36..18cba44f8fb1 100644 --- a/pkg/ccl/streamingccl/streamingest/metrics.go +++ b/pkg/ccl/streamingccl/streamingest/metrics.go @@ -155,7 +155,6 @@ func MakeMetrics(histogramWindow time.Duration) metric.Struct { Buckets: metric.BatchProcessLatencyBuckets, MaxVal: streamingFlushHistMaxLatency.Nanoseconds(), SigFigs: 1, - Mode: metric.HistogramModePreferHdrLatency, }), CommitLatency: metric.NewHistogram(metric.HistogramOptions{ Metadata: metaReplicationCommitLatency, @@ -163,7 +162,6 @@ func MakeMetrics(histogramWindow time.Duration) metric.Struct { Buckets: metric.BatchProcessLatencyBuckets, MaxVal: streamingCommitLatencyMaxValue.Nanoseconds(), SigFigs: 1, - Mode: metric.HistogramModePreferHdrLatency, }), AdmitLatency: metric.NewHistogram(metric.HistogramOptions{ Metadata: metaReplicationAdmitLatency, @@ -171,7 +169,6 @@ func MakeMetrics(histogramWindow time.Duration) metric.Struct { Buckets: metric.BatchProcessLatencyBuckets, MaxVal: streamingAdmitLatencyMaxValue.Nanoseconds(), SigFigs: 1, - Mode: metric.HistogramModePreferHdrLatency, }), RunningCount: metric.NewGauge(metaStreamsRunning), EarliestDataCheckpointSpan: metric.NewGauge(metaEarliestDataCheckpointSpan), From 4dec0b7643d756c86b37707b4e70d1b627a74f9f Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Fri, 3 Feb 2023 14:07:55 -0400 Subject: [PATCH 3/3] pkg/util/metric: remove lingering debug print statements https://github.com/cockroachdb/cockroach/pull/96029 The above patch introduced some `fmt.Println` statements in a test accidentally. This patch removes them. Release note: none --- pkg/util/metric/aggmetric/agg_metric_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/util/metric/aggmetric/agg_metric_test.go b/pkg/util/metric/aggmetric/agg_metric_test.go index a139347d38db..605f7276b2f6 100644 --- a/pkg/util/metric/aggmetric/agg_metric_test.go +++ b/pkg/util/metric/aggmetric/agg_metric_test.go @@ -13,7 +13,6 @@ package aggmetric_test import ( "bufio" "bytes" - "fmt" "sort" "strings" "testing" @@ -93,7 +92,6 @@ func TestAggMetric(t *testing.T) { g3.Inc(3) g3.Dec(1) f2.Update(1.5) - fmt.Println(r) f3.Update(2.5) h2.RecordValue(10) h3.RecordValue(90) @@ -105,7 +103,6 @@ func TestAggMetric(t *testing.T) { }) t.Run("destroy", func(t *testing.T) { - fmt.Println(r) g3.Unlink() c2.Unlink() f3.Unlink()