Merge cockroachdb#154409 cockroachdb#154451

154409: changefeedccl: remove fixed latency checks from pts benchmark test r=asg0451,KeithCh a=aerfrei

Removes the fixed threshold checks for the
`changefeed_stage_pts_manage_latency` and
`changefeed_stage_pts_manage_error_latency` metrics in the
cdc/multi-table-pts-benchmark roachtests. These thresholds were
too rigid and caused noisy failures.

Issue cockroachdb#154447 has been filed to track the goal of monitoring these metrics
via roachperf instead.

Fixes: cockroachdb#152900
Fixes: cockroachdb#152903
Fixes: cockroachdb#153017
Fixes: cockroachdb#154120
Fixes: cockroachdb#154122
Fixes: cockroachdb#154132
Fixes: cockroachdb#154302
Fixes: cockroachdb#154357
Epic: CRDB-1421
Release note: None

154451: server: remove unused argument, update doc comment r=yuzefovich a=tamird

- **server: remove unused argument, update doc comment**
- **server: remove ancient TODO**

Epic: None

Co-authored-by: Aerin Freilich <[email protected]>
Co-authored-by: Tamir Duberstein <[email protected]>

Author: 3 people

pkg/cmd/roachtest/tests/cdc.go

@@ -1880,14 +1880,8 @@ func runCDCMultiTablePTSBenchmark(
 
 	ct.waitForWorkload()
 
-	t.Status("workload finished, verifying metrics")
-
-	// These metrics are in nanoseconds, so we are asserting that both
-	// of these latency metrics are less than 25 milliseconds.
-	ct.verifyMetrics(ctx, ct.verifyMetricsUnderThreshold([]string{
-		"changefeed_stage_pts_manage_latency",
-		"changefeed_stage_pts_create_latency",
-	}, float64(30*time.Millisecond)))
+	// TODO(#154447): Send values of changefeed_stage_pts_manage_latency and
+	// changefeed_stage_pts_manage_error_latency metrics to RoachPerf.
 
 	t.Status("multi-table PTS benchmark finished")
 }
@@ -4921,40 +4915,3 @@ func verifyMetricsNonZero(names ...string) func(metrics map[string]*prompb.Metri
 		return false
 	}
 }
-
-func (ct *cdcTester) verifyMetricsUnderThreshold(
-	names []string, threshold float64,
-) func(metrics map[string]*prompb.MetricFamily) (ok bool) {
-	namesMap := make(map[string]struct{}, len(names))
-	for _, name := range names {
-		namesMap[name] = struct{}{}
-	}
-
-	return func(metrics map[string]*prompb.MetricFamily) (ok bool) {
-		found := map[string]struct{}{}
-
-		for name, fam := range metrics {
-			if _, ok := namesMap[name]; !ok {
-				continue
-			}
-
-			for _, m := range fam.Metric {
-				if m.Histogram.GetSampleCount() == 0 {
-					continue
-				}
-
-				observedValue := m.Histogram.GetSampleSum() / float64(m.Histogram.GetSampleCount())
-				if observedValue < threshold {
-					found[name] = struct{}{}
-				} else {
-					ct.t.Fatalf("observed value for metric %s over threshold. observedValue: %f, threshold: %f", name, observedValue, threshold)
-				}
-			}
-
-			if len(found) == len(names) {
-				return true
-			}
-		}
-		return false
-	}
-}

pkg/server/grpc_gateway.go

@@ -39,11 +39,7 @@ var _ grpcGatewayServer = authserver.Server(nil)
 var _ grpcGatewayServer = (*ts.Server)(nil)
 
 // configureGRPCGateway initializes services necessary for running the
-// GRPC Gateway services proxied against the server at `grpcSrv`.
-//
-// The connection between the reverse proxy provided by grpc-gateway
-// and our grpc server uses a loopback-based listener to create
-// connections between the two.
+// GRPC Gateway services proxied against the server at `grpcAddr`.
 //
 // The function returns 3 arguments that are necessary to call
 // `RegisterGateway` which generated for each of your gRPC services
@@ -53,8 +49,7 @@ func configureGRPCGateway(
 	ambientCtx log.AmbientContext,
 	rpcContext *rpc.Context,
 	stopper *stop.Stopper,
-	grpcSrv *grpcServer,
-	GRPCAddr string,
+	grpcAddr string,
 ) (*gwruntime.ServeMux, context.Context, *grpc.ClientConn, error) {
 	jsonpb := &protoutil.JSONPb{
 		EnumsAsInts:  true,
@@ -77,7 +72,7 @@ func configureGRPCGateway(
 
 	// Eschew `(*rpc.Context).GRPCDial` to avoid unnecessary moving parts on the
 	// uniquely in-process connection.
-	dialOpts, err := rpcContext.GRPCDialOptions(ctx, GRPCAddr, rpcbase.DefaultClass)
+	dialOpts, err := rpcContext.GRPCDialOptions(ctx, grpcAddr, rpcbase.DefaultClass)
 	if err != nil {
 		return nil, nil, nil, err
 	}
@@ -93,7 +88,7 @@ func configureGRPCGateway(
 		telemetry.Inc(getServerEndpointCounter(method))
 		return invoker(ctx, method, req, reply, cc, opts...)
 	}
-	conn, err := grpc.DialContext(ctx, GRPCAddr, append(
+	conn, err := grpc.DialContext(ctx, grpcAddr, append(
 		dialOpts,
 		grpc.WithUnaryInterceptor(callCountInterceptor),
 	)...)

pkg/server/server.go

@@ -1707,7 +1707,6 @@ func (s *topLevelServer) PreStart(ctx context.Context) error {
 		s.cfg.AmbientCtx,
 		s.rpcContext,
 		s.stopper,
-		s.grpc,
 		s.cfg.AdvertiseAddr,
 	)
 	if err != nil {

pkg/server/start_listen.go

@@ -158,7 +158,6 @@ func startListenRPCAndSQL(
 	waitForQuiesce := func(context.Context) {
 		<-stopper.ShouldQuiesce()
 		drpcCancel()
-		// TODO(bdarnell): Do we need to also close the other listeners?
 		netutil.FatalIfUnexpected(grpcL.Close())
 		netutil.FatalIfUnexpected(grpcLoopbackL.Close())
 		netutil.FatalIfUnexpected(drpcL.Close())         // Closing this listener is as good as closing drpcTLSL

pkg/server/tenant.go

@@ -636,7 +636,6 @@ func (s *SQLServerWrapper) PreStart(ctx context.Context) error {
 		s.sqlServer.cfg.AmbientCtx,
 		s.rpcContext,
 		s.stopper,
-		s.grpc,
 		s.sqlServer.cfg.AdvertiseAddr,
 	)
 	if err != nil {