Skip to content

changefeedccl: add static labels to changefeed metrics #156901

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

changefeedccl: add static labels to changefeed metrics #156901

wants to merge 1 commit into from
+109 −42

Conversation

@KeithCh
Copy link
Contributor

@KeithCh KeithCh commented Nov 4, 2025 •
edited
Loading

This commit adds static labels to changefeed stage and emitted metrics
metrics.

Resolves: #156290

Release note: None

@blathers-crl
Copy link

blathers-crl bot commented Nov 4, 2025

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

cockroach-teamcity commented Nov 4, 2025

This change is Reviewable

@KeithCh KeithCh force-pushed the add-static-labels-to-metrics branch 2 times, most recently from ccc8ce5 to 8713506 Compare November 4, 2025 23:57
@KeithCh KeithCh requested a review from log-head November 4, 2025 23:58
@KeithCh KeithCh marked this pull request as ready for review November 4, 2025 23:58
@KeithCh KeithCh requested a review from a team as a code owner November 4, 2025 23:59
log-head
log-head approved these changes Nov 6, 2025
View reviewed changes
Copy link
Contributor

@log-head log-head left a comment

Choose a reason for hiding this comment

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

Looks good! Wondering if you think we should add a unit test for using these through the /metrics endpoint? I did cursory manual testing on this branch and the static labeled metrics show up.

@blathers-crl
Copy link

blathers-crl bot commented Nov 6, 2025

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

@KeithCh KeithCh force-pushed the add-static-labels-to-metrics branch from 8713506 to 25158e2 Compare November 6, 2025 22:28
@KeithCh KeithCh requested a review from asg0451 November 6, 2025 22:29
asg0451
asg0451 requested changes Nov 6, 2025
View reviewed changes
pkg/ccl/changefeedccl/timers/timers.go Outdated
PTSManage: b.Histogram(histogramOptsFor("changefeed.stage.pts.manage.latency", "Latency of the changefeed stage: Time spent successfully managing protected timestamp records on highwater advance, including time spent creating new protected timestamps when needed")),
PTSManageError: b.Histogram(histogramOptsFor("changefeed.stage.pts.manage_error.latency", "Latency of the changefeed stage: Time spent managing protected timestamp when we eventually error")),
PTSCreate: b.Histogram(histogramOptsFor("changefeed.stage.pts.create.latency", "Latency of the changefeed stage: Time spent creating protected timestamp records on changefeed creation")),
CheckpointJobProgress: b.Histogram(stageOpts("checkpoint_job_progress", "Latency of the changefeed stage: checkpointing job progress")),
Copy link
Contributor

@asg0451 asg0451 Nov 6, 2025

Choose a reason for hiding this comment

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

i get the appeal but i actually dont think we should do it like this. the ability to grep the codebase for the literal metric name should not be underestimated. can you alter your approach to maintain that feature pls

@KeithCh KeithCh force-pushed the add-static-labels-to-metrics branch from 25158e2 to 5df1d6e Compare November 7, 2025 02:45
@KeithCh KeithCh requested a review from a team as a code owner November 7, 2025 02:45
@KeithCh KeithCh requested review from golgeek and nameisbhaskar and removed request for a team November 7, 2025 02:45
@KeithCh KeithCh changed the title changefeedccl: add static labels to changefeed stage metrics changefeedccl: add static labels to changefeed metrics Nov 7, 2025
@KeithCh KeithCh force-pushed the add-static-labels-to-metrics branch from 5df1d6e to 817d91c Compare November 7, 2025 02:47
@KeithCh KeithCh requested a review from asg0451 November 7, 2025 02:49
@github-actions
Copy link

github-actions bot commented Nov 7, 2025

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

  • If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
  • If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

@github-actions github-actions bot added the o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. label Nov 7, 2025
@KeithCh KeithCh force-pushed the add-static-labels-to-metrics branch from 817d91c to ce1f3d2 Compare November 7, 2025 03:07
@KeithCh KeithCh marked this pull request as draft November 7, 2025 05:11
@KeithCh
changefeedccl: add static labels to changefeed metrics
3bd384c 
This commit adds static labels to changefeed stage and emitted metrics
metrics.

Resolves: cockroachdb#156290

Release note: None
@KeithCh KeithCh force-pushed the add-static-labels-to-metrics branch from ce1f3d2 to 3bd384c Compare November 7, 2025 15:28
@KeithCh KeithCh marked this pull request as ready for review November 7, 2025 15:29
@KeithCh KeithCh added the O-AI-Review-Real-Issue-Found AI reviewer found real issue label Nov 7, 2025
@KeithCh
Copy link
Contributor Author

KeithCh commented Nov 7, 2025 •
edited
Loading

AI did find a real issue but it was also caught by a failed test in CI.

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

Reviewers

@log-head log-head log-head approved these changes

@golgeek golgeek Awaiting requested review from golgeek golgeek is a code owner automatically assigned from cockroachdb/test-eng

@nameisbhaskar nameisbhaskar Awaiting requested review from nameisbhaskar nameisbhaskar is a code owner automatically assigned from cockroachdb/test-eng

@asg0451 asg0451 Awaiting requested review from asg0451

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

o-AI-Review-Potential-Issue-Detected AI reviewer found potential issue. Never assign manually—auto-applied by GH action only. O-AI-Review-Real-Issue-Found AI reviewer found real issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cdc: consolidate metrics using static labels

4 participants

@KeithCh @cockroach-teamcity @asg0451 @log-head