Merge cockroachdb#96504 cockroachdb#96516

96504: Revert "sql: improve stack trace for get-user-timeout timeouts" r=ecwall a=rafiss

This reverts commit 662e19c.

gRPC does not expect the context to be wrapped, so it can get confused.

informs cockroachdb#95794
Release note: None

96516: pkg/util/metric: fix HistogramMode for streamingingest histograms r=dhartunian a=abarganier

Ref: cockroachdb#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:

a82aa82#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 cockroachdb#96514.

Release note: none

Part of cockroachdb#95833

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Alex Barganier <[email protected]>

pkg/ccl/streamingccl/streamingest/metrics.go

@@ -155,23 +155,20 @@ 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,
 			Duration: histogramWindow,
 			Buckets:  metric.BatchProcessLatencyBuckets,
 			MaxVal:   streamingCommitLatencyMaxValue.Nanoseconds(),
 			SigFigs:  1,
-			Mode:     metric.HistogramModePreferHdrLatency,
 		}),
 		AdmitLatency: metric.NewHistogram(metric.HistogramOptions{
 			Metadata: metaReplicationAdmitLatency,
 			Duration: histogramWindow,
 			Buckets:  metric.BatchProcessLatencyBuckets,
 			MaxVal:   streamingAdmitLatencyMaxValue.Nanoseconds(),
 			SigFigs:  1,
-			Mode:     metric.HistogramModePreferHdrLatency,
 		}),
 		RunningCount:                metric.NewGauge(metaStreamsRunning),
 		EarliestDataCheckpointSpan:  metric.NewGauge(metaEarliestDataCheckpointSpan),

pkg/util/contextutil/context.go

@@ -79,26 +79,13 @@ 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.
 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)

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

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()