changefeedccl: add scoping support for max_behind_nanos #137534

aerfrei · 2024-12-16T17:02:39Z

Currently, changefeed.max_behind_nanos is a measurement of the
lag of the furthest behind changefeed. We'd like to be able to
support scoping/grouping changefeeds. This applies scoping to
that metric similar to the scoping on changefeed.aggregator_progress.

Epic: none
Fixes: #132281

Release note (ops change): the changefeed.max_behind_nanos metric
now supports scoping with metric labels.

blathers-crl · 2024-12-16T17:02:43Z

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 · 2024-12-16T17:02:51Z

This change is

pkg/ccl/changefeedccl/changefeed_processors.go

andyyang890

Reviewed 1 of 3 files at r1, 2 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aa-joshi, @aerfrei, @arjunmahishi, @asg0451, @rharding6373, and @xinhaoz)

-- commits line 9 at r2:
FYI it's not necessary to put an Epic if you've already linked an issue (linking an issue is preferred per https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/2009039063/Issue+and+Epic+Refs+in+Commits+and+PRs)

pkg/ccl/changefeedccl/metrics.go line 1003 at r2 (raw file):

		var max int64
		for _, val := range childValues {
			if val != 0 && val > max {

Do you need the val != 0 clause? Another thing you could do is leverage go's new max function: return max(0, childValues...)

andyyang890

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aa-joshi, @aerfrei, @arjunmahishi, @asg0451, @rharding6373, and @xinhaoz)

pkg/ccl/changefeedccl/metrics.go line 1208 at r2 (raw file):

				return 0
			}
			return timeutil.Now().UnixNano() - minTs

Is there a reason you chose to do a raw subtraction instead of using the Sub method? From skimming the code, it seems like we usually convert the hlc.Timestamp to a go time and then use Sub.

aerfrei

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aa-joshi, @andyyang890, @arjunmahishi, @asg0451, @rharding6373, and @xinhaoz)

-- commits line 9 at r2:

Previously, andyyang890 (Andy Yang) wrote…

FYI it's not necessary to put an Epic if you've already linked an issue (linking an issue is preferred per https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/2009039063/Issue+and+Epic+Refs+in+Commits+and+PRs)

I'll change this, thanks for the tip!

pkg/ccl/changefeedccl/metrics.go line 1003 at r2 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Do you need the val != 0 clause? Another thing you could do is leverage go's new max function: return max(0, childValues...)

Good point on the val check, I'll remove that. I get a compilation error on using max that way, seems like max doesn't support it. https://stackoverflow.com/questions/78778577/a-compilation-error-while-using-builtin-func-max-with-go-1-22-1-invalid-ope

pkg/ccl/changefeedccl/metrics.go line 1208 at r2 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Is there a reason you chose to do a raw subtraction instead of using the Sub method? From skimming the code, it seems like we usually convert the hlc.Timestamp to a go time and then use Sub.

The reason I did it this way is because I chose to reuse the minTimestampGetter, which returns an int64. If I still wanted to use it, I considered converting back into a timestamp to do the subtraction and then back into nanoseconds like this: "timeutil.Now().Sub(time.Unix(0, minTs)).Nanoseconds()" which seems less readable and I didn't see a strong argument the other way. Thoughts?

andyyang890

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aa-joshi, @aerfrei, @arjunmahishi, @asg0451, @rharding6373, and @xinhaoz)

pkg/ccl/changefeedccl/metrics.go line 1003 at r2 (raw file):

Previously, aerfrei (Aerin Freilich) wrote…

Good point on the val check, I'll remove that. I get a compilation error on using max that way, seems like max doesn't support it. https://stackoverflow.com/questions/78778577/a-compilation-error-while-using-builtin-func-max-with-go-1-22-1-invalid-ope

Ah ok. In that case, maybe you could use slices.Max like the StackOverflow answer says instead?

pkg/ccl/changefeedccl/metrics.go line 1208 at r2 (raw file):

Previously, aerfrei (Aerin Freilich) wrote…

The reason I did it this way is because I chose to reuse the minTimestampGetter, which returns an int64. If I still wanted to use it, I considered converting back into a timestamp to do the subtraction and then back into nanoseconds like this: "timeutil.Now().Sub(time.Unix(0, minTs)).Nanoseconds()" which seems less readable and I didn't see a strong argument the other way. Thoughts?

Ah ok, that makes sense to me.

aerfrei

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aa-joshi, @andyyang890, @arjunmahishi, @asg0451, @rharding6373, and @xinhaoz)

pkg/ccl/changefeedccl/metrics.go line 1003 at r2 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Ah ok. In that case, maybe you could use slices.Max like the StackOverflow answer says instead?

Great, done!

asg0451 · 2024-12-18T20:00:04Z

pkg/ccl/changefeedccl/changefeed_processors.go

@@ -1233,14 +1233,14 @@ func newChangeFrontierProcessor(
 		return nil, err
 	}

-	sliMertics, err := flowCtx.Cfg.JobRegistry.MetricsStruct().Changefeed.(*Metrics).getSLIMetrics(cf.spec.Feed.Opts[changefeedbase.OptMetricsScope])
+	sliMetrics, err := flowCtx.Cfg.JobRegistry.MetricsStruct().Changefeed.(*Metrics).getSLIMetrics(cf.spec.Feed.Opts[changefeedbase.OptMetricsScope])


asg0451 · 2024-12-18T20:01:53Z

pkg/ccl/changefeedccl/metrics.go

+	// for now.
+	metaChangefeedMaxBehindNanos := metric.Metadata{
+		Name:        "changefeed.max_behind_nanos",
+		Help:        "(Deprecated in favor of checkpoint_progress) The most any changefeed's persisted checkpoint is behind the present",


maybe remove the deprecation notice, now that we've improved it

aerfrei

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @aa-joshi, @andyyang890, @arjunmahishi, @asg0451, @rharding6373, and @xinhaoz)

aerfrei · 2024-12-19T15:46:06Z

pkg/ccl/changefeedccl/metrics.go

+	// for now.
+	metaChangefeedMaxBehindNanos := metric.Metadata{
+		Name:        "changefeed.max_behind_nanos",
+		Help:        "(Deprecated in favor of checkpoint_progress) The most any changefeed's persisted checkpoint is behind the present",


Currently, changefeed.max_behind_nanos is a measurement of the lag of the furthest behind changefeed. We'd like to be able to support scoping/grouping changefeeds. This applies scoping to that metric similar to the scoping on changefeed.aggregator_progress. Fixes: cockroachdb#132281 Release note (ops change): the changefeed.max_behind_nanos metric now supports scoping with metric labels.

aerfrei · 2024-12-19T20:37:38Z

bors r=andyyang890,asg0451

craig · 2024-12-19T21:17:47Z

Build succeeded:

aerfrei · 2025-01-16T18:01:33Z

blathers backport 23.2 24.1 24.2 24.3

blathers-crl · 2025-01-16T18:01:37Z

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 #132281: 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 · 2025-01-16T18:01:58Z

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

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

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

error creating merge commit from 0a3420e to blathers/backport-release-23.2-137534: 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 failed. See errors above.

error creating merge commit from 0a3420e to blathers/backport-release-24.1-137534: 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 24.1 failed. See errors above.

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

aerfrei · 2025-01-16T18:02:17Z

blathers backport 23.2 24.1 24.2 24.3

blathers-crl · 2025-01-16T18:02:26Z

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

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

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

error creating merge commit from 0a3420e to blathers/backport-release-23.2-137534: 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 failed. See errors above.

error creating merge commit from 0a3420e to blathers/backport-release-24.1-137534: 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 24.1 failed. See errors above.

error creating backport branch refs/heads/blathers/backport-release-24.2-137534: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 24.2 failed. See errors above.

error creating backport branch refs/heads/blathers/backport-release-24.3-137534: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 24.3 failed. See errors above.

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

aerfrei force-pushed the aerin/metrics branch 7 times, most recently from 2842954 to 611ab0c Compare December 17, 2024 19:06

aerfrei changed the title ~~Aerin/metrics~~ changefeedccl: add scoping support for max_behind_nanos Dec 17, 2024

aerfrei force-pushed the aerin/metrics branch from 611ab0c to 41c8b83 Compare December 17, 2024 20:34

aerfrei commented Dec 17, 2024

View reviewed changes

pkg/ccl/changefeedccl/changefeed_processors.go Show resolved Hide resolved

aerfrei force-pushed the aerin/metrics branch 2 times, most recently from 1b644b9 to 6e1a766 Compare December 17, 2024 21:51

aerfrei marked this pull request as ready for review December 17, 2024 21:52

aerfrei requested review from a team as code owners December 17, 2024 21:52

aerfrei requested review from xinhaoz, arjunmahishi, aa-joshi, rharding6373, asg0451 and andyyang890 and removed request for a team December 17, 2024 21:52

andyyang890 approved these changes Dec 18, 2024

View reviewed changes

andyyang890 reviewed Dec 18, 2024

View reviewed changes

aerfrei force-pushed the aerin/metrics branch from 6e1a766 to 578ed5a Compare December 18, 2024 19:01

aerfrei commented Dec 18, 2024

View reviewed changes

andyyang890 reviewed Dec 18, 2024

View reviewed changes

aerfrei force-pushed the aerin/metrics branch from 578ed5a to 72ae4ab Compare December 18, 2024 20:57

aerfrei commented Dec 18, 2024

View reviewed changes

andyyang890 approved these changes Dec 18, 2024

View reviewed changes

asg0451 approved these changes Dec 19, 2024

View reviewed changes

aerfrei force-pushed the aerin/metrics branch from 72ae4ab to 8a40ff1 Compare December 19, 2024 15:45

aerfrei commented Dec 19, 2024

View reviewed changes

aerfrei force-pushed the aerin/metrics branch from 8a40ff1 to 0a3420e Compare December 19, 2024 16:49

craig bot merged commit 9a32312 into cockroachdb:master Dec 19, 2024
22 checks passed

This was referenced Jan 16, 2025

cdc: add checkpoint progress metrics per changefeed #132281

Closed

release-24.2: changefeedccl: add scoping support for max_behind_nanos #139233

Open

release-24.3: changefeedccl: add scoping support for max_behind_nanos #139234

Open

This was referenced Jan 16, 2025

release-24.1: changefeedccl: add scoping support for max_behind_nanos #139239

Open

release-23.2: changefeedccl: add scoping support for max_behind_nanos #139241

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changefeedccl: add scoping support for max_behind_nanos #137534

changefeedccl: add scoping support for max_behind_nanos #137534

aerfrei commented Dec 16, 2024 •

edited

Loading

blathers-crl bot commented Dec 16, 2024

cockroach-teamcity commented Dec 16, 2024

andyyang890 left a comment

andyyang890 left a comment

aerfrei left a comment

andyyang890 left a comment

aerfrei left a comment

asg0451 Dec 18, 2024

asg0451 Dec 18, 2024

aerfrei Dec 19, 2024

aerfrei left a comment

aerfrei Dec 19, 2024

aerfrei commented Dec 19, 2024

craig bot commented Dec 19, 2024

aerfrei commented Jan 16, 2025

blathers-crl bot commented Jan 16, 2025

blathers-crl bot commented Jan 16, 2025

aerfrei commented Jan 16, 2025

blathers-crl bot commented Jan 16, 2025

changefeedccl: add scoping support for max_behind_nanos #137534

changefeedccl: add scoping support for max_behind_nanos #137534

Conversation

aerfrei commented Dec 16, 2024 • edited Loading

blathers-crl bot commented Dec 16, 2024

cockroach-teamcity commented Dec 16, 2024

andyyang890 left a comment

Choose a reason for hiding this comment

andyyang890 left a comment

Choose a reason for hiding this comment

aerfrei left a comment

Choose a reason for hiding this comment

andyyang890 left a comment

Choose a reason for hiding this comment

aerfrei left a comment

Choose a reason for hiding this comment

asg0451 Dec 18, 2024

Choose a reason for hiding this comment

asg0451 Dec 18, 2024

Choose a reason for hiding this comment

aerfrei Dec 19, 2024

Choose a reason for hiding this comment

aerfrei left a comment

Choose a reason for hiding this comment

aerfrei Dec 19, 2024

Choose a reason for hiding this comment

aerfrei commented Dec 19, 2024

craig bot commented Dec 19, 2024

aerfrei commented Jan 16, 2025

blathers-crl bot commented Jan 16, 2025

blathers-crl bot commented Jan 16, 2025

aerfrei commented Jan 16, 2025

blathers-crl bot commented Jan 16, 2025

aerfrei commented Dec 16, 2024 •

edited

Loading