Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvstreamer: add some metrics #136332

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Nov 28, 2024

This commit adds the following metrics:

  • kv.streamer.operators.active tracks how many active streamer operators are out there currently
  • kv.streamer.batches.count tracks total number of BatchRequests sent by all streamer instances
  • kv.streamer.batches.in_progress tracks how many BatchRequests are in flight currently, across all streamer instances
  • kv.streamer.batches.throttled tracks how many BatchRequests are being currently throttled (queued) due to reaching the streamer concurrency limit, across all streamer instances.

It also includes a couple of recently added dist sender metrics into roachprod opentelemetry registry.

Fixes: #131126.

Release note: None

@yuzefovich yuzefovich requested a review from michae2 November 28, 2024 01:42
@yuzefovich yuzefovich requested a review from a team as a code owner November 28, 2024 01:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @michae2)


pkg/kv/kvclient/kvstreamer/metrics.go line 18 at r1 (raw file):

	}
	metaStreamerRequestsInProgress = metric.Metadata{
		Name:        "kv.streamer.requests_in_progress",

I wonder whether we should use kv.streamers* prefix for the metrics. Unlike the dist sender (which is server-wide singleton), we create a separate streamer instance for every use, and e.g. the concurrency limit is applied to each instance separately, so perhaps plural in the metrics name would be more descriptive? Thoughts?

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Nice!

Should we also add these to pkg/roachprod/opentelemetry/cockroachdb_metrics.go?

Reviewed 10 of 12 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)


pkg/kv/kvclient/kvstreamer/metrics.go line 18 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I wonder whether we should use kv.streamers* prefix for the metrics. Unlike the dist sender (which is server-wide singleton), we create a separate streamer instance for every use, and e.g. the concurrency limit is applied to each instance separately, so perhaps plural in the metrics name would be more descriptive? Thoughts?

I think there's also a trend of naming stats after the "module", so from that perspective a singular kv.streamer prefix seems fine. I'm ok either way.


pkg/kv/kvclient/kvstreamer/metrics.go line 12 at r2 (raw file):

var (
	metaStreamerCount = metric.Metadata{
		Name:        "kv.streamer.operators.count",

bikeshedding: it's a little confusing to suffix a gauge metric with .count, maybe kv.streamer.operators.active would be clearer?

This commit adds the following metrics:
- `kv.streamer.operators.active` tracks how many active streamer operators
are out there currently
- `kv.streamer.batches.count` tracks total number of BatchRequests sent
by all streamer instances
- `kv.streamer.batches.in_progress` tracks how many BatchRequests are
in flight currently, across all streamer instances
- `kv.streamer.batches.throttled` tracks how many BatchRequests are
being currently throttled (queued) due to reaching the streamer concurrency
limit, across all streamer instances.

It also includes a couple of recently added dist sender metrics into
roachprod opentelemetry registry.

Release note: None
@yuzefovich yuzefovich requested a review from a team as a code owner December 3, 2024 17:54
@yuzefovich yuzefovich requested review from herkolategan and DarrylWong and removed request for a team December 3, 2024 17:54
Copy link
Member Author

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, TIL about the opentelemetry registry - added metrics from this PR as well as a couple we added earlier for the dist sender.

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @herkolategan, and @michae2)


pkg/kv/kvclient/kvstreamer/metrics.go line 18 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I think there's also a trend of naming stats after the "module", so from that perspective a singular kv.streamer prefix seems fine. I'm ok either way.

Ok, let's keep it singular then.


pkg/kv/kvclient/kvstreamer/metrics.go line 12 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

bikeshedding: it's a little confusing to suffix a gauge metric with .count, maybe kv.streamer.operators.active would be clearer?

Yeah, I think it's clearer, thanks.

@craig craig bot merged commit ef46c67 into cockroachdb:master Dec 3, 2024
22 of 23 checks passed
@yuzefovich yuzefovich deleted the streamer-metrics branch December 3, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gauge metric for concurrent kv.streamer operations
3 participants