Skip to content

metric: embed source file in Metadata for CODEOWNERS resolution#167494

Open
angles-n-daemons wants to merge 1 commit intocockroachdb:masterfrom
angles-n-daemons:bdillmann/metric-source-file
Open

metric: embed source file in Metadata for CODEOWNERS resolution#167494
angles-n-daemons wants to merge 1 commit intocockroachdb:masterfrom
angles-n-daemons:bdillmann/metric-source-file

Conversation

@angles-n-daemons
Copy link
Copy Markdown
Contributor

@angles-n-daemons angles-n-daemons commented Apr 3, 2026

Summary

  • Adds a source_file field to metric.Metadata proto, populated
    automatically by a new metric.NewMetadata() constructor via
    runtime.Caller.
  • At generation time, cockroach gen metric-list resolves metric
    ownership by calling codeowners.Match() on each metric's source
    file — eliminating the separate gen-metric-owners AST scanning
    tool, metric_owners.yaml, and bespoke CI/dev steps.
  • Adds a metadatanew nogo analyzer to enforce constructor usage
    across the codebase.

Epic: none

Release note: None

@angles-n-daemons angles-n-daemons requested review from a team as code owners April 3, 2026 18:15
@angles-n-daemons angles-n-daemons requested review from alyshanjahani-crl and removed request for a team April 3, 2026 18:15
@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented Apr 3, 2026

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@angles-n-daemons angles-n-daemons requested review from mw5h and visheshbardia and removed request for a team April 3, 2026 18:15
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 3, 2026

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@angles-n-daemons angles-n-daemons requested review from aerfrei and dt and removed request for a team April 3, 2026 18:15
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Apr 3, 2026

This change is Reviewable

@angles-n-daemons angles-n-daemons removed request for a team April 3, 2026 18:16
@angles-n-daemons angles-n-daemons marked this pull request as draft April 3, 2026 18:16
@angles-n-daemons angles-n-daemons force-pushed the bdillmann/metric-source-file branch from 588f079 to 52318d2 Compare April 3, 2026 18:20
@angles-n-daemons angles-n-daemons marked this pull request as ready for review April 10, 2026 17:50
jasonlmfong
jasonlmfong reviewed Apr 13, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@jasonlmfong jasonlmfong left a comment

Choose a reason for hiding this comment

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

overall makes sense, some comments

Comment thread pkg/backup/backup_processor_planning.go Outdated
recv := sql.MakeDistSQLReceiver(
ctx,
sql.NewMetadataCallbackWriter(rowResultWriter, metaFn),
sql.InitMetadataCallbackWriter(rowResultWriter, metaFn),
Copy link
Copy Markdown
Member

@jasonlmfong jasonlmfong Apr 13, 2026

Choose a reason for hiding this comment

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

i don't think these are supposed to change?

Comment thread pkg/obs/clustermetrics/cmmetrics/metadata.go
meta, ok := metadata.mu.registeredMetadata[name]
if !ok {
return metric.Metadata{}, nil, false
return metric.InitMetadata(metric.Metadata{}), nil, false
Copy link
Copy Markdown
Member

@jasonlmfong jasonlmfong Apr 13, 2026

Choose a reason for hiding this comment

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

what happens when the metadata is empty?

Comment thread pkg/util/metric/metadata.go Outdated
Comment on lines +56 to +59
pkg := frame.Function
if idx := strings.LastIndex(pkg, "."); idx >= 0 {
pkg = pkg[:idx]
}
Copy link
Copy Markdown
Member

@jasonlmfong jasonlmfong Apr 13, 2026

Choose a reason for hiding this comment

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

claude suggests that this can break for some paths that have more than one .

Copy link
Copy Markdown
Member

@jasonlmfong jasonlmfong Apr 13, 2026

Choose a reason for hiding this comment

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

maybe add some tests for this

angles-n-daemons
angles-n-daemons commented Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@angles-n-daemons angles-n-daemons left a comment

Choose a reason for hiding this comment

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

Thanks for the review Jason, really appreciate you catching these!

  • MetadataCallbackWriter renames: Good eye — that was unintended collateral from the rename. Reverted all of them back to New.
  • Empty metadata in cmmetrics: This one we actually need to keep since the linter will flag it otherwise.
  • LastIndex(".") bug: You were right to flag this. It broke for method receivers (e.g. pkg.(*Type).Method) and closures (pkg.init.func1) where there are multiple dots. Fixed it to find the last / and then take the first . after that, which correctly isolates the package in all cases.
  • Tests: Added tests covering plain functions, method receivers, and closures. All green.

@angles-n-daemons angles-n-daemons force-pushed the bdillmann/metric-source-file branch from a89a6e8 to a61b6b9 Compare April 15, 2026 15:25
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

1 similar comment
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

1 similar comment
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

1 similar comment
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 15, 2026

Detected infrastructure failure (matched: ). Automatically rerunning failed jobs. (run link)

@angles-n-daemons angles-n-daemons force-pushed the bdillmann/metric-source-file branch from 545d136 to cb97091 Compare April 16, 2026 12:53
dt
dt reviewed Apr 16, 2026
View reviewed changes
Comment thread pkg/util/metric/metadata.go Outdated
// "path/to/pkg.(*Type).Method". We find the last '/' to isolate
// the final segment, then take the first '.' after it — that
// always separates the package name from the symbol.
pkg := frame.Function
Copy link
Copy Markdown
Contributor

@dt dt Apr 16, 2026

Choose a reason for hiding this comment

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

why frame.Function and not frame.File?

Copy link
Copy Markdown
Contributor Author

@angles-n-daemons angles-n-daemons Apr 16, 2026

Choose a reason for hiding this comment

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

Good call — switched to using frame.File directly. Go records module-relative paths in frame.File for code within a module, so strings.TrimPrefix(frame.File, modulePrefix) is all we need. Much simpler and avoids the dot-parsing complexity that Jason flagged.

@angles-n-daemons @roachdev-claude
metric: embed source file in Metadata for CODEOWNERS resolution
c533a65 
Adds a `source_file` field to `metric.Metadata` proto, populated
automatically by new `metric.InitMetadata()` and `metric.NewMetadata()`
constructors via `runtime.Callers`.

At generation time, `cockroach gen metric-list` resolves metric
ownership by calling `codeowners.Match()` on each metric's source
file — eliminating the separate `gen-metric-owners` AST scanning
tool, `metric_owners.yaml`, and bespoke CI/dev steps.

Adds a `metricmetadatainit` nogo analyzer to enforce constructor usage
across the codebase.

Epic: none
Release note: None

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
@angles-n-daemons angles-n-daemons force-pushed the bdillmann/metric-source-file branch from cb97091 to c533a65 Compare April 16, 2026 14:12
jasonlmfong
jasonlmfong approved these changes Apr 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@jasonlmfong jasonlmfong 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! thanks for addressing the comments

@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 27, 2026

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

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

Reviewers

@dt dt dt left review comments

@jasonlmfong jasonlmfong jasonlmfong approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@angles-n-daemons @cockroach-teamcity @dt @jasonlmfong