Skip to content

Comments

changefeedccl: change cdc latency hist bucket sizes#136265

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
rharding6373:20241126_cdc_hist_buckets
Dec 3, 2024
Merged

changefeedccl: change cdc latency hist bucket sizes#136265
craig[bot] merged 1 commit intocockroachdb:masterfrom
rharding6373:20241126_cdc_hist_buckets

Conversation

@rharding6373
Copy link
Collaborator

@rharding6373 rharding6373 commented Nov 27, 2024

Increases the changefeed.admit_latency and changefeed.commit_latency max
from 5m to 60m and decreases their min to 5ms.

The metrics changefeed.parallel_io_queue_nanos,
changefeed.parallel_io_result_queue_nanos,
changefeed.sink_batch_hist_nanos,
changefeed.flush_hist_nanos, and
changefeed.kafka_throttling_hist_nanos have the new limits 5ms to 10m
(previously 500ms to 5m).

Epic: none
Fixes: #134593

Release note (general change): In order to improve the granularity of
changefeed pipeline metrics, the changefeed metrics
changefeed.admit_latency and changefeed.commit_latency have histogram
buckets from 5ms to 60m (previously 500ms to 5m). The changefeed
metrics changefeed.parallel_io_queue_nanos,
changefeed.parallel_io_result_queue_nanos,
changefeed.sink_batch_hist_nanos,
changefeed.flush_hist_nanos, and
changefeed.kafka_throttling_hist_nanos have histogram buckets from 5ms
to 10m (previously 500ms to 5m).

@rharding6373 rharding6373 requested a review from asg0451 November 27, 2024 00:24
@rharding6373 rharding6373 requested a review from a team as a code owner November 27, 2024 00:24
@blathers-crl
Copy link

blathers-crl bot commented Nov 27, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@andyyang890 andyyang890 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 @asg0451 and @rharding6373)


-- commits line 9 at r1:
nit: release notes should have a category


pkg/ccl/changefeedccl/metrics.go line 1062 at r1 (raw file):

			MaxVal:       commitLatencyMaxValue.Nanoseconds(),
			SigFigs:      1,
			BucketConfig: metric.LongRunning60mLatencyBuckets,

I recall we were previously discussing introducing a new bucket config with a smaller min bucket than 500ms. Should we address that in the same PR?

@rharding6373 rharding6373 force-pushed the 20241126_cdc_hist_buckets branch from fb97477 to 604985c Compare November 27, 2024 19:14
Copy link
Collaborator Author

@rharding6373 rharding6373 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 @andyyang890 and @asg0451)


pkg/ccl/changefeedccl/metrics.go line 1062 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

I recall we were previously discussing introducing a new bucket config with a smaller min bucket than 500ms. Should we address that in the same PR?

You're referring to the metrics in this issue? #134593

Those are different metrics. I can try to follow up with that one next week.

MaxVal: commitLatencyMaxValue.Nanoseconds(),
SigFigs: 1,
BucketConfig: metric.BatchProcessLatencyBuckets,
BucketConfig: metric.LongRunning60mLatencyBuckets,
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're making changes to the histogram buckets, can we change them to ones with a min < 500ms?

also may want to call out which metrics are changing by name in the release notes, in case this affects metric users.

Increases the changefeed.admit_latency and changefeed.commit_latency max
from 5m to 60m and decreases their min to 5ms.

The metrics changefeed.parallel_io_queue_nanos,
changefeed.parallel_io_result_queue_nanos,
changefeed.sink_batch_hist_nanos,
changefeed.flush_hist_nanos, and
changefeed.kafka_throttling_hist_nanos have the new limits 5ms to 10m
(previously 500ms to 5m).

Epic: none
Fixes: cockroachdb#134593

Release note (general change): In order to improve the granularity of
changefeed pipeline metrics, the changefeed metrics
changefeed.admit_latency and changefeed.commit_latency have histogram
buckets from 5ms to 60m (previously  500ms to 5m). The changefeed
metrics changefeed.parallel_io_queue_nanos,
changefeed.parallel_io_result_queue_nanos,
changefeed.sink_batch_hist_nanos,
changefeed.flush_hist_nanos, and
changefeed.kafka_throttling_hist_nanos have histogram buckets from 5ms
to 10m (previously 500ms to 5m).
@rharding6373 rharding6373 force-pushed the 20241126_cdc_hist_buckets branch from 604985c to 83e8c2a Compare December 3, 2024 00:42
@rharding6373 rharding6373 requested review from a team as code owners December 3, 2024 00:42
@rharding6373 rharding6373 requested review from aa-joshi, arjunmahishi and kyle-a-wong and removed request for a team December 3, 2024 00:42
@rharding6373 rharding6373 changed the title changefeedccl: increase admit and commit latency hist bucket max changefeedccl: change cdc latency hist bucket sizes Dec 3, 2024
@rharding6373 rharding6373 requested a review from asg0451 December 3, 2024 00:45
@rharding6373 rharding6373 added backport-23.2.x PAST MAINTENANCE SUPPORT: 23.2 patch releases via ER request only backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x backport-24.3.x Flags PRs that need to be backported to 24.3 labels Dec 3, 2024
@rharding6373
Copy link
Collaborator Author

TFTRs!

bors r=asg0451

@craig craig bot merged commit 2e1a81d into cockroachdb:master Dec 3, 2024
@blathers-crl
Copy link

blathers-crl bot commented Dec 3, 2024

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.


Issue #134593: branch-release-23.2, branch-release-24.1, branch-release-24.2, branch-release-24.3.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl
Copy link

blathers-crl bot commented Dec 3, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 83e8c2a to blathers/backport-release-23.2-136265: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-23.2.x PAST MAINTENANCE SUPPORT: 23.2 patch releases via ER request only backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.3.x Flags PRs that need to be backported to 24.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

changefeedccl: lower minimum bucket for some metrics

4 participants