rpc, server: add DRPC metrics server interceptor #153651

Nukitt · 2025-09-17T09:18:16Z

Previously, there was no support for metrics interceptor in
DRPC, and capturing drpc metrics wouldn't be possible.

In this PR, we add metrics server interceptor support in DRPC and
reuse the rpc_server_request_duration_nanos metric
for both gRPC and DRPC since the metric reflects server-side
duration agnostic of transport and interceptor chains are not expected
to differ materially in practice. When differentiation is needed, it
can be correlated with other metadata. This way we can observe both
gRPC and DRPC latency through interceptors.

Since this metric is behind a cluster setting, we also had to
change the key of the metric to server.rpc.request_metrics.enabled
so that it can be related to both the rpc framework and set an alias
as its retired name server.grpc.request_metrics.enabled.

Epic: CRDB-49359
Fixes: #144373
Release note: None

cockroach-teamcity · 2025-09-17T09:18:28Z

This change is

aa-joshi

@aa-joshi reviewed 3 of 10 files at r1, 1 of 4 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cthumuluru-crdb, @dhartunian, and @shubhamdhama)

-- commits line 4 at r3:
Let's add the brief about the changes.

-- commits line 10 at r3:
Let's include the release notes as this change is going to affect customer as well

pkg/server/serve_mode.go line 60 at r3 (raw file):

// metrics so the metrics are only exported to external sources such as
// /_status/vars and DataDog.
var serverRPCRequestMetricsEnabled = settings.RegisterBoolSetting(

I see that we are deprecating the existing cluster settings and introducing the new one. I am not really sure about this change as existing customer might get affected due to this. Let's confirm the expectation about renaming/changing the cluster settings.

pkg/rpc/metrics.go line 483 at r3 (raw file):

			RpcMethodLabel:     info.FullMethod,
			RpcStatusCodeLabel: code.String(),
			RpcServerLabel:     "grpc",

I would suggest to add it as constant

pkg/rpc/metrics.go line 500 at r3 (raw file):

	return func(
		ctx context.Context,
		req interface{},

let's use any.

pkg/rpc/metrics.go line 503 at r3 (raw file):

		rpc string,
		handler drpcmux.UnaryHandler,
	) (interface{}, error) {

let's use any.

pkg/rpc/metrics.go line 521 at r3 (raw file):

			RpcMethodLabel:     rpc,
			RpcStatusCodeLabel: code.String(),
			RpcServerLabel:     "drpc",

Let's add it as constant

Nukitt

TFTR!!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @cthumuluru-crdb, @dhartunian, and @shubhamdhama)

-- commits line 4 at r3:

Previously, aa-joshi (Akshay Joshi) wrote…

Let's add the brief about the changes.

Done.

-- commits line 10 at r3:

Previously, aa-joshi (Akshay Joshi) wrote…

Let's include the release notes as this change is going to affect customer as well

Done.

pkg/rpc/metrics.go line 483 at r3 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

I would suggest to add it as constant

Done.

pkg/rpc/metrics.go line 500 at r3 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

let's use any.

Done.

pkg/rpc/metrics.go line 503 at r3 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

let's use any.

Done.

pkg/rpc/metrics.go line 521 at r3 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

Let's add it as constant

Done.

pkg/server/serve_mode.go line 60 at r3 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

I see that we are deprecating the existing cluster settings and introducing the new one. I am not really sure about this change as existing customer might get affected due to this. Let's confirm the expectation about renaming/changing the cluster settings.

Not going ahead with the setting deprecation, instead added an alias for it. Customer shouldn't face any issues now.

aa-joshi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cthumuluru-crdb, @dhartunian, @Nukitt, and @shubhamdhama)

-- commits line 10 at r3:

Previously, Nukitt (Nukit) wrote…

Done.

Let's be explicit about the alias that we are introducing in the cluster settings.

pkg/server/serve_mode.go line 65 at r4 (raw file):

	"enables the collection of rpc metrics",
	false,
	settings.WithName("server.grpc.request_metrics.enabled"),

Should it be WithName or WithRetiredName?

cthumuluru-crdb

@aa-joshi, @dhartunian Existing customer dashboards may not account for this new dimension. Adding it could distort the signals customers see today. Should we instead introduce a new metric and keep it internal until DRPC is released. Do you have any recommendations on how we should proceed with this change?

pkg/rpc/drpc.go

pkg/rpc/metrics.go

pkg/rpc/settings.go

Nukitt

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aa-joshi, @cthumuluru-crdb, @dhartunian, and @shubhamdhama)

-- commits line 10 at r3:

Previously, aa-joshi (Akshay Joshi) wrote…

Let's be explicit about the alias that we are introducing in the cluster settings.

Done.

pkg/server/serve_mode.go line 65 at r4 (raw file):

Previously, aa-joshi (Akshay Joshi) wrote…

Should it be WithName or WithRetiredName?

WithRetiredName looks more appropriate, will go with that.

Nukitt · 2025-10-30T11:10:54Z

Based on the conversation on this slack thread, we evaluated how to expose DRPC server request metrics alongside the existing gRPC server interceptor that records durations in rpc.server.request.duration.nanos (a non-essential metric implemented with Prometheus vector metrics that are being deprecated in favor of AggMetrics) and is disabled by default behind server.grpc.request_metrics.enabled, collecting for methods prefixed with /cockroach.server and /cockroach.ts when enabled.
Adding a DRPC request-metrics interceptor prompted three options: (1) reuse the existing metric, (2) add a distinguishing label (e.g., rpc_server) to the metric, or (3) create a new DRPC-specific metric.
We are now moving ahead with the first option to reuse the existing metric for both gRPC and DRPC since the metric reflects server-side duration agnostic of transport and interceptor chains are not expected to differ materially in practice.
When differentiation is needed, we can correlate with other metadata; since the metric is for internal use, renaming or meaning adjustments are not to be worried about.

cthumuluru-crdb

LGTM! I've a few minor comments. Can you please take care of those before pushing the PR? Regarding adding guidance feel free to create a ticket to track that separately.

pkg/rpc/drpc.go

pkg/rpc/metrics.go

pkg/rpc/drpc.go

pkg/rpc/settings.go

cthumuluru-crdb · 2025-11-02T06:34:02Z

pkg/server/serve_mode.go

+// collection of gRPC and DRPC request duration metrics. This uses export only
+// metrics so the metrics are only exported to external sources such as
+// /_status/vars and DataDog.
+var serverRPCRequestMetricsEnabled = settings.RegisterBoolSetting(


server.rpc.request_metrics.enabled is not a public setting. This change looks good to me. There is some discussion around cluster setting deprecation here. Can you followup, add the guidance for deprecating and renaming cluster settings and publish it please?

Ok will update this doc accordingly.

blathers-crl · 2025-11-02T06:38:00Z

Metrics change detected

This PR adds or updates one or more CRDB metrics. If you want these metrics to be exported by CRDB Cloud clusters to Internal CRL Datadog and/or included in the customer metric export integration (Essential metrics for standard deployment, and Essential metrics for advanced deployment), refer to this Installation and Usage guide of a CLI tool that syncs the metric mappings in managed-service. Run this CLI tool after your CRDB PR is merged.

The CLI opens a PR in managed-service with the required config changes.
Please track that PR and ensure it merges so your metrics become available to CRDB Cloud clusters.

Note: Your metric will appear in Internal CRL Datadog only after the managed-service PR merges and the new OTel configuration rolls out to at least one cluster running a CRDB build that includes this metric.

Docs: cockroach-metric-sync

Questions: reach out to @obs-india-prs

Previously, there was no support for metrics interceptor in DRPC, and capturing drpc metrics wouldn't be possible. In this PR, we add metrics server interceptor support in DRPC and reuse the `rpc_server_request_duration_nanos` metric for both gRPC and DRPC since the metric reflects server-side duration agnostic of transport and interceptor chains are not expected to differ materially in practice. When differentiation is needed, it can be correlated with other metadata. This way we can observe both gRPC and DRPC latency through interceptors. Since this metric is behind a cluster setting, we also had to change the key of the metric to `server.rpc.request_metrics.enabled` so that it can be related to both the rpc framework and set an alias as its retired name `server.grpc.request_metrics.enabled`. Epic: CRDB-49359 Fixes: cockroachdb#144373 Release note: None

cthumuluru-crdb · 2025-11-10T05:37:29Z

@Nukitt, re: comment here, I don't think we are adding any new metrics right? We’re planning to reuse the existing ones, so these metrics should already be exported to Cloud’s internal Datadog if they’re intended to be.

Nukitt · 2025-11-10T05:41:34Z

@Nukitt, re: comment here, I don't think we are adding any new metrics right? We’re planning to reuse the existing ones, so these metrics should already be exported to Cloud’s internal Datadog if they’re intended to be.

@cthumuluru-crdb correct, I have the same understanding hence ignoring the bot's message.

Nukitt · 2025-11-10T08:00:11Z

TFTR!

bors r=cthumuluru-crdb

craig · 2025-11-10T08:46:27Z

Build succeeded:

Nukitt force-pushed the metricsServerInterceptor branch from aefbab8 to c02a61f Compare September 17, 2025 11:40

Nukitt marked this pull request as ready for review September 17, 2025 13:58

Nukitt requested review from a team as code owners September 17, 2025 13:58

Nukitt requested review from cthumuluru-crdb, dhartunian and shubhamdhama and removed request for a team September 17, 2025 13:58

Nukitt force-pushed the metricsServerInterceptor branch from c02a61f to 59818af Compare September 18, 2025 15:19

Nukitt requested a review from aa-joshi September 18, 2025 16:04

aa-joshi reviewed Sep 19, 2025

View reviewed changes

Nukitt commented Sep 19, 2025

View reviewed changes

Nukitt force-pushed the metricsServerInterceptor branch 2 times, most recently from 71447ae to 4116db7 Compare September 22, 2025 05:53

aa-joshi reviewed Sep 22, 2025

View reviewed changes

shubhamdhama mentioned this pull request Sep 23, 2025

server: add metrics server interceptor to DRPC #149761

Closed

cthumuluru-crdb reviewed Sep 24, 2025

View reviewed changes

pkg/rpc/drpc.go Outdated Show resolved Hide resolved

pkg/rpc/metrics.go Outdated Show resolved Hide resolved

pkg/rpc/metrics.go Outdated Show resolved Hide resolved

pkg/rpc/settings.go Show resolved Hide resolved

shubhamdhama mentioned this pull request Sep 25, 2025

kv/kvserver: TestProcessSplitAfterRightHandSideHasBeenRemoved failed #153948

Closed

Nukitt commented Sep 26, 2025

View reviewed changes

Nukitt force-pushed the metricsServerInterceptor branch from 4116db7 to 541bb05 Compare September 26, 2025 10:43

shubhamdhama requested review from aa-joshi and cthumuluru-crdb October 8, 2025 08:43

Nukitt force-pushed the metricsServerInterceptor branch from 541bb05 to fb02340 Compare October 31, 2025 03:46

cthumuluru-crdb approved these changes Nov 2, 2025

View reviewed changes

Nukitt force-pushed the metricsServerInterceptor branch from fb02340 to c561d21 Compare November 10, 2025 05:17

craig bot merged commit 7281dd9 into cockroachdb:master Nov 10, 2025
24 checks passed

celeste-cockroachdb bot added the target-release-26.1.0 label Nov 10, 2025

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 20, 2025

Add simulation README for PR cockroachdb#153651

9e588a8

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 21, 2025

Add simulation README for PR cockroachdb#153651

abf7dfb

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Nov 21, 2025

Add simulation README for PR cockroachdb#153651

f17caaf

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Dec 1, 2025

Add simulation README for PR cockroachdb#153651

493326b

celeste-cockroachdb bot added v26.1.0-prerelease and removed target-release-26.1.0 labels Dec 4, 2025

rishabh7m added a commit to crltest-issue-syncing/cockroach-fork that referenced this pull request Dec 10, 2025

Add simulation README for PR cockroachdb#153651

bf4fd6f

rpc, server: add DRPC metrics server interceptor #153651

rpc, server: add DRPC metrics server interceptor #153651

Uh oh!

Conversation

Nukitt commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Sep 17, 2025

Uh oh!

aa-joshi left a comment

Choose a reason for hiding this comment

Uh oh!

Nukitt left a comment

Choose a reason for hiding this comment

Uh oh!

aa-joshi left a comment

Choose a reason for hiding this comment

Uh oh!

cthumuluru-crdb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nukitt left a comment

Choose a reason for hiding this comment

Uh oh!

Nukitt commented Oct 30, 2025

Uh oh!

cthumuluru-crdb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cthumuluru-crdb Nov 2, 2025

Choose a reason for hiding this comment

Uh oh!

Nukitt Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

blathers-crl bot commented Nov 2, 2025

Metrics change detected

Uh oh!

cthumuluru-crdb commented Nov 10, 2025

Uh oh!

Nukitt commented Nov 10, 2025

Uh oh!

Nukitt commented Nov 10, 2025

Uh oh!

craig bot commented Nov 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Nukitt commented Sep 17, 2025 •

edited

Loading