-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
changefeedccl: add roachtesting for network metrics #130522
Conversation
b0a756a
to
59ac82c
Compare
59ac82c
to
793e2c7
Compare
2a6c3eb
to
a88b6e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice verifier! I have only one concern.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asg0451, @DarrylWong, and @renatolabs)
pkg/cmd/roachtest/tests/cdc.go
line 1659 at r1 (raw file):
steadyLatency: 5 * time.Minute, }) ct.runMetricsVerifier(verifyChangefeedBytesInOutMetricsPresent("changefeed_network_bytes_in", "changefeed_network_bytes_out"))
What do you think about moving this after ct.waitForWorkload()
. Although the changefeed has a job id at this point, it's not guaranteed that it has been scheduled, planned, and is processing events even with the 5s grace period as part of runMetricsVerifier
. It will likely be less flaky if called at the end of the test, but that depends on if we're reading the metrics live or if we can get historical metrics.
Alternatively, could use testutils.SucceedsSoon
to provide a longer grace period to runMetricsVerifier
pkg/cmd/roachtest/tests/cdc.go
line 3712 at r1 (raw file):
} func verifyChangefeedBytesInOutMetricsPresent(
nit: this function that checks metrics seems agnostic to bytes in/out. Might be better named something like verifyMetricNonZero.
a88b6e6
to
9187d74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DarrylWong, @renatolabs, and @rharding6373)
pkg/cmd/roachtest/tests/cdc.go
line 1659 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
What do you think about moving this after
ct.waitForWorkload()
. Although the changefeed has a job id at this point, it's not guaranteed that it has been scheduled, planned, and is processing events even with the 5s grace period as part ofrunMetricsVerifier
. It will likely be less flaky if called at the end of the test, but that depends on if we're reading the metrics live or if we can get historical metrics.Alternatively, could use
testutils.SucceedsSoon
to provide a longer grace period torunMetricsVerifier
Fixed. The way i had it before was more convenient for testing it but this should work too and as you say be less flaky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong and @renatolabs)
Add a roachtest helper that checks that the new cdc network metrics are being emitted. For now it simply checks that they are > 0. Part of: cockroachdb#130097 Release note: None
9187d74
to
33c96f7
Compare
bors r=rharding6373 |
Add a roachtest helper that checks that the new
cdc network metrics are being emitted. For now it
simply checks that they are > 0.
Part of: #130097
Release note: None