ts: declare expected type of each allowlisted child metric#169750
Closed
jasonlmfong wants to merge 2 commits intocockroachdb:masterfrom
Closed
ts: declare expected type of each allowlisted child metric#169750jasonlmfong wants to merge 2 commits intocockroachdb:masterfrom
jasonlmfong wants to merge 2 commits intocockroachdb:masterfrom
Conversation
Contributor
|
Merging to
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 |
Member
|
This change is |
jasonlmfong
force-pushed
the
jf/child-ts-metrics-types
branch
from
a809966 to
eeb615f
Compare
jasonlmfong
commented
May 5, 2026
Comment on lines
+1165
to
1175
| // hashLabels computes a stable hash of a label set. The zero-byte separator | ||
| // after each field prevents collisions where boundaries could otherwise shift | ||
| // (e.g. without it, {a="bc"} and {ab="c"} would both hash "abc"). | ||
| func hashLabels(labels []*prometheusgo.LabelPair) uint64 { | ||
| h := fnv.New64a() | ||
| for _, label := range labels { | ||
| h.Write([]byte(label.GetName())) | ||
| h.Write(hashSep) | ||
| h.Write([]byte{0}) | ||
| h.Write([]byte(label.GetValue())) | ||
| h.Write(hashSep) | ||
| h.Write([]byte{0}) | ||
| } |
Member
Author
There was a problem hiding this comment.
This does not incur allocation when inlined, so i cleaned it up
dhartunian
reviewed
May 5, 2026
Collaborator
dhartunian
left a comment
dhartunian
left a comment
There was a problem hiding this comment.
Some general comments:
- this diff contains changes that are unrelated to the intent, you're refactoring code while changing its behavior which makes it tough to review
- you can automate some pretty fine-grained commit management so, can you make this PR contain two commits: one that's a mechanical refactor and one that's a behavioral change? I think the behavior change is much smaller than the refactor in terms of the diff.
- separately: if you're going to expand the lines of code and methods that
recordChangefeedChildMetricsis going to "occupy" inrecorder.go, I'd prefer for it to get its own file. This isn't really something I actually want to do, as it creates even more churn in the codebase for a feature that we really should delete in a year or two and makes backports challenging, but just consider for next time regarding how features expand within a file and create pollution.
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
Three unrelated cleanups:
- Drop the hashSep package-level variable in hashLabels and use a
[]byte{0} literal directly. The compiler does not allocate for that
literal, so the hoist was unnecessary.
- Inline single-use value and metricName temporaries in the histogram
emit path; they were computed only to be plugged into the next
struct literal.
- Remove the metricName := name shadow in IsAllowedChildMetric. The
function parameter is already a local string and can be reassigned
in place.
No behavior change.
Epic: none
Release note: None
jasonlmfong
force-pushed
the
jf/child-ts-metrics-types
branch
from
eeb615f to
596efde
Compare
Previously, AllowedChildMetrics was a set of metric names with no information about what kind of Prometheus metric each one represents. recordChangefeedChildMetrics decided how to record each entry by attempting type assertions at runtime. This made the TSDB shape of an entry an emergent property of whatever Go type the metric happened to have at registration time -- a future implementation change (e.g. swapping a counter for a histogram, or replacing a gauge with a different aggmetric variant) would silently rewrite what gets written to TSDB. This change declares the expected kind alongside the name. The recorder now dispatches on the declared class and verifies the runtime type matches before recording; a mismatch skips the metric rather than emitting it under the wrong shape. The map is unexported and accessed via LookupChildMetricClass so the declared kind cannot be bypassed by callers iterating the map directly. Resolves: cockroachdb#169179 Epic: none Release note: None
jasonlmfong
force-pushed
the
jf/child-ts-metrics-types
branch
from
596efde to
85bde5d
Compare
Member
Author
|
I extracted the cleanups into a separate PR, also ok with not proceeding with these changes that increase the surface area for no real gain |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, AllowedChildMetrics was a set of metric names with no information about what kind of Prometheus metric each one represents. recordChangefeedChildMetrics decided how to record each entry by attempting type assertions at runtime. This made the TSDB shape of an entry an emergent property of whatever Go type the metric happened to have at registration time -- a future implementation change (e.g. swapping a counter for a histogram, or replacing a gauge with a different aggmetric variant) would silently rewrite what gets written to TSDB.
This change declares the expected kind alongside the name. The recorder now dispatches on the declared class and verifies the runtime type matches before recording; a mismatch skips the metric rather than emitting it under the wrong shape. Split the two recording paths into recordHistogramChildren and recordScalarChildren to keep the dispatch readable.
The map is unexported and accessed via LookupChildMetricClass so the declared kind cannot be bypassed by callers iterating the map directly.
Resolves: #169179
Epic: none
Release note: None
this is stacked behind #169817