Conversation
zinic
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a thread-safe Prometheus metrics registry in Go that caches and reuses Counter, CounterVec, and Gauge instances keyed by name, namespace, and deterministic labels; registers Go/process collectors on init; exposes functions to obtain metrics and to register/unregister collectors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Reg as MetricsRegistry
participant PR as PrometheusRegistry
Client->>Reg: Request metric (name, namespace, constLabels, [varLabels])
alt metric cached
Reg-->>Client: Return cached metric
else not cached
Reg->>Reg: Build stable key (sort const labels / var label names)
Reg->>PR: Create metric via promauto.With(Registerer()).NewXxx
PR-->>Reg: New metric/collector created
Reg->>Reg: Cache metric under key
Reg-->>Client: Return new metric
end
Note over Reg,PR: On init, Reg registers Go and process collectors with PR
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/go/metrics/registry.go (2)
86-86: Use a richer boolean name in theCounterVeccache check.
hasCounteris ambiguous in this scope;hasCounterVecis clearer.As per coding guidelines `**/*.go`: Prefer rich variable names, for example: `databaseInterface` instead of `di` or `dbi`.Proposed refactor
- if counterVec, hasCounter := s.counterVecs[key]; hasCounter { + if counterVec, hasCounterVec := s.counterVecs[key]; hasCounterVec { return counterVec } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` at line 86, The boolean variable name in the CounterVec cache lookup is ambiguous: change the inline bool name from hasCounter to hasCounterVec where the code checks s.counterVecs[key] (the expression that binds counterVec, hasCounter) so it reads something like "if counterVec, hasCounterVec := s.counterVecs[key]; hasCounterVec { ... }"; update any subsequent references in that conditional block to use hasCounterVec to match the new name.
36-43: Group and hoist local initializations inmetricKey.Use a
var (...)block at the top of the function for local initializations.As per coding guidelines `**/*.go`: Group variable initializations in a `var ( ... )` block and hoist them to the top of the function when possible.Proposed refactor
func metricKey(name, namespace string, labels map[string]string) string { - builder := strings.Builder{} - - builder.WriteString(namespace) - builder.WriteString(name) - - sortedLabelKeys := make([]string, 0, len(labels)) + var ( + builder strings.Builder + sortedLabelKeys = make([]string, 0, len(labels)) + ) + + builder.WriteString(namespace) + builder.WriteString(name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 36 - 43, The function metricKey has local variable initializations (builder, sortedLabelKeys) scattered; hoist and group them in a var (...) block at the top of metricKey to follow the Go style guideline: declare builder (strings.Builder), sortedLabelKeys (make []string with capacity len(labels)), and any other locals in a var block before any logic, then leave the subsequent WriteString and label-sorting logic unchanged, referencing metricKey, builder and sortedLabelKeys by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/metrics/registry.go`:
- Around line 18-26: The metricKey function currently concatenates parts without
a boundary causing ambiguous keys and metric aliasing; update metricKey (and any
similar key-building logic around the metricKey usages) to produce unambiguous
keys by encoding or delimiting parts—e.g., join parts with a reserved separator
(such as '\x00' or a character sequence that cannot appear in parts) or use a
length-prefixed encoding (like fmt.Sprintf("%d:%s", len(part), part)) so
different inputs cannot collapse to the same key; modify the metricKey
implementation and any callers that rely on its output (the metricKey function
and related caching code in registry.go) accordingly.
- Around line 80-97: The cache key for registry.CounterVec currently uses
metricKey(name, namespace, constLabels) but omits the variableLabelNames,
causing different label schemas to collide; update the CounterVec implementation
(function registry.CounterVec) to include a deterministic representation of
variableLabelNames in the key (e.g., sort the slice then join into a string or
otherwise serialize it) and use that augmented key when looking up and storing
in s.counterVecs so vectors with different variable labels are cached
separately.
---
Nitpick comments:
In `@packages/go/metrics/registry.go`:
- Line 86: The boolean variable name in the CounterVec cache lookup is
ambiguous: change the inline bool name from hasCounter to hasCounterVec where
the code checks s.counterVecs[key] (the expression that binds counterVec,
hasCounter) so it reads something like "if counterVec, hasCounterVec :=
s.counterVecs[key]; hasCounterVec { ... }"; update any subsequent references in
that conditional block to use hasCounterVec to match the new name.
- Around line 36-43: The function metricKey has local variable initializations
(builder, sortedLabelKeys) scattered; hoist and group them in a var (...) block
at the top of metricKey to follow the Go style guideline: declare builder
(strings.Builder), sortedLabelKeys (make []string with capacity len(labels)),
and any other locals in a var block before any logic, then leave the subsequent
WriteString and label-sorting logic unchanged, referencing metricKey, builder
and sortedLabelKeys by name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/go/metrics/registry.go (1)
42-56:⚠️ Potential issue | 🟠 MajorMake cache keys unambiguous to prevent metric aliasing.
metricKeystill concatenates parts without boundaries, so different(namespace, name, labels)combinations can collide and return the wrong cached metric.Proposed fix
import ( + "strconv" "slices" "strings" "sync" @@ func metricKey(name, namespace string, labels map[string]string) string { - builder := strings.Builder{} + var builder strings.Builder - builder.WriteString(namespace) - builder.WriteString(name) + writeMetricKeyPart(&builder, namespace) + writeMetricKeyPart(&builder, name) @@ - for _, key := range sortedLabelKeys { - builder.WriteString(key) - builder.WriteString(labels[key]) + for _, labelKey := range sortedLabelKeys { + writeMetricKeyPart(&builder, labelKey) + writeMetricKeyPart(&builder, labels[labelKey]) } return builder.String() } + +func writeMetricKeyPart(builder *strings.Builder, value string) { + builder.WriteString(strconv.Itoa(len(value))) + builder.WriteByte(':') + builder.WriteString(value) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 42 - 56, The cache key construction in registry.go (the metricKey builder) currently concatenates namespace, name and label key/values without boundaries causing ambiguous collisions; update the builder to insert explicit separators and key/value delimiters (for example add a separator between namespace and name, use "=" between each label key and value and a distinct separator between label pairs, or alternatively prefix each component with its length) while keeping the sortedLabelKeys ordering—modify the block that writes namespace, name and the loop over sortedLabelKeys (the builder.WriteString calls) to emit unambiguous boundaries so different (namespace, name, labels) combinations cannot collide.
🧹 Nitpick comments (1)
packages/go/metrics/registry_test.go (1)
22-127: Add regression cases for collision boundaries and input immutability.Current tests validate sorting/determinism, but they don’t catch two high-risk regressions: (1) distinct metric inputs collapsing to the same concatenated key, and (2)
variableLabelsbeing mutated by key generation. Add explicit tests for both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry_test.go` around lines 22 - 127, Add two regression tests in registry_test.go: one that constructs distinct inputs which would collide if metricKey/metricVecKey simply concatenated without separators (use pairs that produce same flat string) and assert the produced keys differ (reference metricKey and metricVecKey), and another that verifies metricVecKey does not mutate its variableLabels input by saving a copy of the original slice (and labels map if needed) before calling metricVecKey and asserting the original slice (and map) equals the saved copy after the call; if mutation is detected, fail the test. Ensure these tests reference metricKey and metricVecKey to locate the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/metrics/registry.go`:
- Around line 67-71: In metricVecKey, avoid mutating the caller's variableLabels
and stop including constant label values in the key: clone variableLabels into a
new slice before sorting (so callers' slices remain unchanged), sort the cloned
slice, and when building the key only append metric name/namespace and the
variable label NAMES (do not append labels[variableLabel] or any constant
values); also initialize the builder using a var block (e.g., var builder
strings.Builder) per style guidelines and rename the loop variable to
variableLabel for clarity (these changes touch metricVecKey, variableLabels
handling, the builder initialization, and the loop that constructs the key used
by prometheus.NewCounterVec).
---
Duplicate comments:
In `@packages/go/metrics/registry.go`:
- Around line 42-56: The cache key construction in registry.go (the metricKey
builder) currently concatenates namespace, name and label key/values without
boundaries causing ambiguous collisions; update the builder to insert explicit
separators and key/value delimiters (for example add a separator between
namespace and name, use "=" between each label key and value and a distinct
separator between label pairs, or alternatively prefix each component with its
length) while keeping the sortedLabelKeys ordering—modify the block that writes
namespace, name and the loop over sortedLabelKeys (the builder.WriteString
calls) to emit unambiguous boundaries so different (namespace, name, labels)
combinations cannot collide.
---
Nitpick comments:
In `@packages/go/metrics/registry_test.go`:
- Around line 22-127: Add two regression tests in registry_test.go: one that
constructs distinct inputs which would collide if metricKey/metricVecKey simply
concatenated without separators (use pairs that produce same flat string) and
assert the produced keys differ (reference metricKey and metricVecKey), and
another that verifies metricVecKey does not mutate its variableLabels input by
saving a copy of the original slice (and labels map if needed) before calling
metricVecKey and asserting the original slice (and map) equals the saved copy
after the call; if mutation is detected, fail the test. Ensure these tests
reference metricKey and metricVecKey to locate the implementation.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/metrics/registry.gopackages/go/metrics/registry_test.go
zinic
force-pushed
the
pipetest
branch
2 times, most recently
from
fa89f98 to
45e52a4
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/go/metrics/registry.go (1)
39-60:⚠️ Potential issue | 🟠 MajorStrengthen key encoding—collisions are still possible in cache keys.
metricKeystill concatenates variable-length parts without unambiguous boundaries (e.g.,namespace+name,labelKey+labelValue). Different inputs can still collapse to the same key and return the wrong cached metric instance.Proposed fix
import ( + "strconv" "slices" "strings" "sync" @@ func metricKey(name, namespace string, labels map[string]string) string { - builder := strings.Builder{} - - builder.WriteString(namespace) - builder.WriteString(name) + var builder strings.Builder + writeMetricKeyPart(&builder, namespace) + writeMetricKeyPart(&builder, name) @@ - for idx, key := range sortedLabelKeys { - if idx > 0 { - builder.WriteRune('|') - } - - builder.WriteString(key) - builder.WriteString(labels[key]) + for _, labelKey := range sortedLabelKeys { + writeMetricKeyPart(&builder, labelKey) + writeMetricKeyPart(&builder, labels[labelKey]) } @@ func metricVecKey(name, namespace string, labels map[string]string, variableLabels []string) string { - builder := strings.Builder{} + var builder strings.Builder builder.WriteString(metricKey(name, namespace, labels)) - builder.WriteRune('|') @@ - for idx, label := range sortedVariableLabels { - if idx > 0 { - builder.WriteRune('|') - } - - builder.WriteString(label) + for _, variableLabel := range sortedVariableLabels { + writeMetricKeyPart(&builder, variableLabel) } - - builder.WriteString("vec") + writeMetricKeyPart(&builder, "vec") return builder.String() } + +func writeMetricKeyPart(builder *strings.Builder, value string) { + builder.WriteString(strconv.Itoa(len(value))) + builder.WriteByte(':') + builder.WriteString(value) +}Also applies to: 67-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 39 - 60, The metricKey function builds ambiguous keys by concatenating namespace, name, and labels without clear boundaries; update metricKey (and the similar code block around the 67-85 range) to produce unambiguous encodings by inserting explicit separators and/or length prefixes or by escaping components: e.g., prefix namespace and name with their lengths or a fixed separator, and for each sortedLabelKeys entry append a delimiter between key and value (or encode key/value using a reversible escaping or percent-encoding) so keys cannot collide when different parts concatenate; ensure you modify the logic that iterates sortedLabelKeys to include the chosen separator/encoding consistently.
🧹 Nitpick comments (1)
packages/go/metrics/registry_test.go (1)
22-67: Add regression tests for key-collision boundaries.Current cases prove ordering, but they don’t assert uniqueness for boundary-collision inputs. Add explicit non-equality cases (e.g.,
namespace/nameboundary andlabelKey/labelValueboundary) so aliasing regressions are caught immediately.Suggested test additions
func TestMetricKey(t *testing.T) { + t.Run("namespace and name boundaries do not collide", func(t *testing.T) { + left := metricKey("c", "ab", nil) + right := metricKey("bc", "a", nil) + if left == right { + t.Fatalf("unexpected collision: %q == %q", left, right) + } + }) + + t.Run("label key/value boundaries do not collide", func(t *testing.T) { + left := metricKey("metric", "ns", map[string]string{"a": "bc"}) + right := metricKey("metric", "ns", map[string]string{"ab": "c"}) + if left == right { + t.Fatalf("unexpected collision: %q == %q", left, right) + } + }) + tests := []struct {Also applies to: 69-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry_test.go` around lines 22 - 67, Add regression tests inside TestMetricKey to assert non-equality for boundary-collision inputs so aliasing regressions are caught: add cases that exercise the namespace/name boundary and label-key/label-value boundary (e.g., inputs where namespace="ns", name="name" vs namespace="nsname", name="" and label map entries like {"a":"bc"} vs {"ab":"c"}) and in the test loop call metricKey(name, namespace, labels) and assert keys are not equal for those pairs; update TestMetricKey to include these explicit non-equality assertions referencing the existing TestMetricKey function and metricKey helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/go/metrics/registry.go`:
- Around line 39-60: The metricKey function builds ambiguous keys by
concatenating namespace, name, and labels without clear boundaries; update
metricKey (and the similar code block around the 67-85 range) to produce
unambiguous encodings by inserting explicit separators and/or length prefixes or
by escaping components: e.g., prefix namespace and name with their lengths or a
fixed separator, and for each sortedLabelKeys entry append a delimiter between
key and value (or encode key/value using a reversible escaping or
percent-encoding) so keys cannot collide when different parts concatenate;
ensure you modify the logic that iterates sortedLabelKeys to include the chosen
separator/encoding consistently.
---
Nitpick comments:
In `@packages/go/metrics/registry_test.go`:
- Around line 22-67: Add regression tests inside TestMetricKey to assert
non-equality for boundary-collision inputs so aliasing regressions are caught:
add cases that exercise the namespace/name boundary and label-key/label-value
boundary (e.g., inputs where namespace="ns", name="name" vs namespace="nsname",
name="" and label map entries like {"a":"bc"} vs {"ab":"c"}) and in the test
loop call metricKey(name, namespace, labels) and assert keys are not equal for
those pairs; update TestMetricKey to include these explicit non-equality
assertions referencing the existing TestMetricKey function and metricKey helper.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/metrics/registry.gopackages/go/metrics/registry_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/go/metrics/registry.go (1)
55-62:⚠️ Potential issue | 🟠 MajorMetric cache keys can still collide for different const-label maps.
At Line 60-Line 61,
labelKeyandlabels[key]are concatenated without an internal boundary, so distinct inputs can still collapse to the same key and return the wrong cached metric.Proposed fix
import ( + "strconv" "slices" "strings" "sync" @@ func metricKey(name, namespace string, labels map[string]string) string { builder := strings.Builder{} @@ - for idx, key := range sortedLabelKeys { + for idx, key := range sortedLabelKeys { if idx > 0 { builder.WriteRune('|') } - - builder.WriteString(key) - builder.WriteString(labels[key]) + labelValue := labels[key] + builder.WriteString(strconv.Itoa(len(key))) + builder.WriteRune(':') + builder.WriteString(key) + builder.WriteRune('|') + builder.WriteString(strconv.Itoa(len(labelValue))) + builder.WriteRune(':') + builder.WriteString(labelValue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 55 - 62, The cache key builder in the loop over sortedLabelKeys concatenates key and labels[key] with no separator which can cause collisions; update the loop in registry.go (the code using sortedLabelKeys, builder.WriteString(key) and builder.WriteString(labels[key])) to emit an unambiguous boundary between key and value (for example write a delimiter like ':' or '=' between them) and either escape that delimiter in keys/values or use a length-prefixed format (e.g., append length + ':' + value) so distinct label maps cannot collapse to the same cache key.
🧹 Nitpick comments (1)
packages/go/metrics/registry.go (1)
40-47: Use groupedvar (...)initialization blocks in key builders.At Line 40-Line 47 and Line 70-Line 75, local initializations are split across short declarations. This repo’s Go style prefers grouped
varblocks hoisted to the top where possible.As per coding guidelines, "Group variable initializations in a
var ( ... )block and hoist them to the top of the function when possible".Also applies to: 70-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 40 - 47, Replace the split short declarations with a grouped var block hoisted to the top of the enclosing function: declare builder and sortedLabelKeys (and the second set at 70-75) inside a single var (...) block instead of using := inline; keep the same types and initial capacities (e.g. strings.Builder for builder and make([]string, 0, len(labels)) for sortedLabelKeys) and then use those variables in the existing key-building code (refer to builder and sortedLabelKeys to locate the spots to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/go/metrics/registry.go`:
- Around line 208-209: Unregister currently calls
Registerer().Unregister(collector) but leaves entries in the local cache
(globalRegistry), which lets later Counter/Gauge/CounterVec lookups return stale
collectors; update Unregister to also remove any cache entries that reference
the unregistered collector from globalRegistry (e.g., iterate/remove matching
values or keys) while holding the registry mutex (globalRegistryMu) to avoid
races so subsequent NewCounter/NewGauge/NewCounterVec calls won't return
cached-but-unregistered collectors.
- Around line 119-129: The cache key and vector must use the same deterministic
label ordering: sort a copy of variableLabelNames before calling metricVecKey
and before passing to NewCounterVec (do not mutate the caller slice); update the
block that checks s.counterVecs and creates newCounterVec so it computes a
sortedLabels slice, uses metricVecKey(name, namespace, constLabels,
sortedLabels) and passes sortedLabels to
promauto.With(s.prometheusRegistry).NewCounterVec(...) to prevent mismatched
WithLabelValues ordering for the same cached vector.
---
Duplicate comments:
In `@packages/go/metrics/registry.go`:
- Around line 55-62: The cache key builder in the loop over sortedLabelKeys
concatenates key and labels[key] with no separator which can cause collisions;
update the loop in registry.go (the code using sortedLabelKeys,
builder.WriteString(key) and builder.WriteString(labels[key])) to emit an
unambiguous boundary between key and value (for example write a delimiter like
':' or '=' between them) and either escape that delimiter in keys/values or use
a length-prefixed format (e.g., append length + ':' + value) so distinct label
maps cannot collapse to the same cache key.
---
Nitpick comments:
In `@packages/go/metrics/registry.go`:
- Around line 40-47: Replace the split short declarations with a grouped var
block hoisted to the top of the enclosing function: declare builder and
sortedLabelKeys (and the second set at 70-75) inside a single var (...) block
instead of using := inline; keep the same types and initial capacities (e.g.
strings.Builder for builder and make([]string, 0, len(labels)) for
sortedLabelKeys) and then use those variables in the existing key-building code
(refer to builder and sortedLabelKeys to locate the spots to change).
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/metrics/registry.gopackages/go/metrics/registry_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/metrics/registry_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/go/metrics/registry.go (1)
55-62:⚠️ Potential issue | 🟠 MajorMetric cache keys can still alias across different label maps.
At Line 60 and Line 61, key and value are concatenated without an internal boundary. Distinct labels like
{"ab":"c"}and{"a":"bc"}both serialize to the same segment, which can return the wrong cached collector.Proposed fix
import ( + "strconv" "slices" "strings" "sync" @@ func metricKey(name, namespace string, labels map[string]string) string { builder := strings.Builder{} @@ for idx, key := range sortedLabelKeys { if idx > 0 { builder.WriteRune('|') } - - builder.WriteString(key) - builder.WriteString(labels[key]) + writeMetricKeyPart(&builder, key) + writeMetricKeyPart(&builder, labels[key]) } @@ } + +func writeMetricKeyPart(builder *strings.Builder, value string) { + builder.WriteString(strconv.Itoa(len(value))) + builder.WriteRune(':') + builder.WriteString(value) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 55 - 62, The current cache key builder concatenates label key and value directly (loop over sortedLabelKeys with builder.WriteString(key) then builder.WriteString(labels[key])), which allows collisions like {"ab":"c"} vs {"a":"bc"}; fix by adding a clear delimiter or escaping between key and value (e.g., write a separator rune such as '=' or ':' or use a safe encoder/quoting like strconv.Quote on the value) when building each segment and keep the existing '|' between pairs to ensure unambiguous, stable serialization for the cache key.
🧹 Nitpick comments (2)
packages/go/metrics/registry.go (1)
197-209: Clarify cache-bypass semantics directly in exported docs.Please make the
Registerer,Register, andUnregistercomments explicit that they bypass the internal metric caches and are intended as escape hatches.Based on learnings:
RegisterandUnregisterare intentional escape hatches that allow direct registry access without interacting with cached metric paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry.go` around lines 197 - 209, Update the exported comments for Registerer, Register, and Unregister to explicitly state they bypass the package's internal metric caches and are intended only as an escape hatch for direct access to the underlying Prometheus registry; mention that using Register/Unregister will not update or consult cached metric paths and callers must manage cache consistency and label/metric naming themselves when using these functions (reference Registerer(), Register(prometheus.Collector), Unregister(prometheus.Collector)).packages/go/metrics/registry_test.go (1)
22-127: Add lifecycle tests forRegister/Unregisterescape-hatch behavior.Please add focused tests that verify these APIs intentionally bypass cache semantics, so future refactors don’t accidentally couple them to cached
Counter/CounterVec/Gaugepaths.Based on learnings:
RegisterandUnregisterare intentional escape hatches that bypass the internal cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/go/metrics/registry_test.go` around lines 22 - 127, Add new unit tests in registry_test.go that assert Register and Unregister act as escape hatches that bypass the internal cache: for Counter, CounterVec and Gauge create a metric via the normal cached path (e.g., NewCounter/NewCounterVec/NewGauge or existing helpers), then call Register with a different instance under the same name and assert the registry returns the registered instance (not the cached one) using the metricKey/metricVecKey to identify the expected key; also add a test that Unregister removes a metric regardless of cache state and allows a subsequent Register to succeed. Use the Register and Unregister symbols directly and keep tests focused and isolated (create separate registry instances if available) so future refactors won’t reintroduce cache coupling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/go/metrics/registry.go`:
- Around line 55-62: The current cache key builder concatenates label key and
value directly (loop over sortedLabelKeys with builder.WriteString(key) then
builder.WriteString(labels[key])), which allows collisions like {"ab":"c"} vs
{"a":"bc"}; fix by adding a clear delimiter or escaping between key and value
(e.g., write a separator rune such as '=' or ':' or use a safe encoder/quoting
like strconv.Quote on the value) when building each segment and keep the
existing '|' between pairs to ensure unambiguous, stable serialization for the
cache key.
---
Nitpick comments:
In `@packages/go/metrics/registry_test.go`:
- Around line 22-127: Add new unit tests in registry_test.go that assert
Register and Unregister act as escape hatches that bypass the internal cache:
for Counter, CounterVec and Gauge create a metric via the normal cached path
(e.g., NewCounter/NewCounterVec/NewGauge or existing helpers), then call
Register with a different instance under the same name and assert the registry
returns the registered instance (not the cached one) using the
metricKey/metricVecKey to identify the expected key; also add a test that
Unregister removes a metric regardless of cache state and allows a subsequent
Register to succeed. Use the Register and Unregister symbols directly and keep
tests focused and isolated (create separate registry instances if available) so
future refactors won’t reintroduce cache coupling.
In `@packages/go/metrics/registry.go`:
- Around line 197-209: Update the exported comments for Registerer, Register,
and Unregister to explicitly state they bypass the package's internal metric
caches and are intended only as an escape hatch for direct access to the
underlying Prometheus registry; mention that using Register/Unregister will not
update or consult cached metric paths and callers must manage cache consistency
and label/metric naming themselves when using these functions (reference
Registerer(), Register(prometheus.Collector), Unregister(prometheus.Collector)).
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/go/metrics/registry.gopackages/go/metrics/registry_test.go
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
1 participant
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.
Description
Metrics work to better support future work.
Motivation and Context
Resolves: BED-7502
Types of changes
Checklist:
Summary by CodeRabbit