feat: controller lease acquisition metrics (JEP-0013 Phase 2) - #932
feat: controller lease acquisition metrics (JEP-0013 Phase 2)#932RoddieKieley wants to merge 1 commit into
Conversation
- Add `jumpstarter_lease_acquisitions_total{result}` on the Jumpstarter controller `/metrics` endpoint with Prometheus exemplars (default keys `client`, `lease_id`).
- Keep operator-managed Controller `-metrics-bind-address=:8080`; no Router metrics flags in this PR.
- Tests cover series registration, acquire success/failure increments, exemplars, and Controller metrics bind assertions (JEP-0013 Phase 2, controller slice).
📝 WalkthroughWalkthroughThe controller adds Prometheus lease-acquisition metrics, records success and failure outcomes during lease reconciliation, registers default metrics at startup, and tests both metric behavior and the controller deployment’s metrics binding. ChangesLease acquisition metrics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LeaseReconciler
participant LeaseMetrics
participant Prometheus
LeaseReconciler->>LeaseMetrics: Record lease acquisition success or failure
LeaseMetrics->>Prometheus: Increment labeled counter
LeaseMetrics->>Prometheus: Attach allowlisted exemplar labels when available
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go`:
- Around line 27-77: The stdlib-only TestControllerDeploymentMetricsBind
violates the operator test harness requirement. Move or adapt this assertion
into the existing envtest-based test setup for the Jumpstarter controller,
preserving validation of the metrics argument and port, then verify with the
requested controller tests, type checks, lint, and full test commands.
In `@controller/internal/metrics/lease_test.go`:
- Around line 136-151: The TestOpenMetricsTextContainsSeries serialization flow
currently uses expfmt.MetricFamilyToText, which produces Prometheus text.
Replace it with the OpenMetrics formatter or encoder, then assert
OpenMetrics-specific output including exemplar labels and the required
OpenMetrics end-of-stream terminator while preserving the existing metric and
result-label assertions.
In `@controller/internal/metrics/lease.go`:
- Around line 81-83: Update the exemplar path in the metric handling code around
AddWithExemplar to validate or constrain the client and lease_id labels before
calling it, dropping the exemplar when either value is invalid rather than
allowing a panic. Preserve the existing metric increment behavior and only omit
the exemplar for invalid resource-derived values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e89286d-35c8-4332-b83e-c0e79c97a350
📒 Files selected for processing (5)
controller/cmd/main.gocontroller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.gocontroller/internal/controller/lease_controller.gocontroller/internal/metrics/lease.gocontroller/internal/metrics/lease_test.go
| // Stdlib unit test (no envtest): asserts Controller Deployment metrics bind for JEP-0013 Phase 2. | ||
| func TestControllerDeploymentMetricsBind(t *testing.T) { | ||
| r := &JumpstarterReconciler{} | ||
| js := &operatorv1alpha1.Jumpstarter{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "jumpstarter", | ||
| Namespace: "jumpstarter-lab", | ||
| }, | ||
| Spec: operatorv1alpha1.JumpstarterSpec{ | ||
| Controller: operatorv1alpha1.ControllerConfig{ | ||
| Image: "example.com/controller:test", | ||
| ImagePullPolicy: corev1.PullIfNotPresent, | ||
| Replicas: 1, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| dep := r.createControllerDeployment(js, "testhash") | ||
| if dep == nil { | ||
| t.Fatal("expected non-nil deployment") | ||
| } | ||
| if len(dep.Spec.Template.Spec.Containers) == 0 { | ||
| t.Fatal("expected at least one container") | ||
| } | ||
|
|
||
| c := dep.Spec.Template.Spec.Containers[0] | ||
| foundArg := false | ||
| for _, arg := range c.Args { | ||
| if arg == "-metrics-bind-address=:8080" { | ||
| foundArg = true | ||
| break | ||
| } | ||
| } | ||
| if !foundArg { | ||
| t.Fatalf("expected -metrics-bind-address=:8080 in args, got %#v", c.Args) | ||
| } | ||
|
|
||
| foundPort := false | ||
| for _, p := range c.Ports { | ||
| if p.Name == "metrics" { | ||
| foundPort = true | ||
| if p.ContainerPort != 8080 { | ||
| t.Fatalf("metrics port = %d, want 8080", p.ContainerPort) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| if !foundPort { | ||
| t.Fatal("expected container port named metrics") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'controller/deploy/operator/internal/controller/jumpstarter/.*_test\.go|controller/deploy/operator/internal/controller/jumpstarter/'
echo "== target file =="
if [ -f controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go ]; then
cat -n controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go
fi
echo "== operator test files environment imports/usages =="
rg -n "envtest|SetupEnv|StartEnv|controller.RuntimeConfig|Kubebuilder|s\.Start|env\.Start|CreateNamespace|DeleteNamespace" controller/deploy/operator/internal/controller -*ext --glob '*_test.go' | head -200 || true
echo "== make targets =="
if [ -f Makefile ]; then
rg -n 'pkg-test-controller|pkg-ty-controller|lint-fix|test|make' Makefile | head -200
elif [ -f makefile ]; then
rg -n 'pkg-test-controller|pkg-ty-controller|lint-fix|test|make' makefile | head -200
fi
echo "== go.mod k8s test env packages =="
rg -n "sigs.k8s.io/controller-runtime/pkg/envtest|testing/memfs|k8s.io/client-go/kubernetes/fake|k8s.io/api" controller/deploy/operator -g '*.go' | head -200Repository: jumpstarter-dev/jumpstarter
Length of output: 19607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'controller/deploy/operator/internal/controller/jumpstarter/.*_test\.go|controller/deploy/operator/internal/controller/jumpstarter/'
echo "== target file =="
if [ -f controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go ]; then
cat -n controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go
fi
echo "== operator test files envtest/harness usages =="
rg -n "envtest|SetupEnv|StartEnv|controller.RuntimeConfig|Kubebuilder|s\.Start|env\.Start|CreateNamespace|DeleteNamespace|NewTestClient|ctrl\.Run|ctrl\.SetupWithManager" controller/deploy/operator/internal/controller controller/deploy/operator -g '*_test.go' | head -200 || true
echo "== make targets =="
if [ -f Makefile ]; then
rg -n 'pkg-test-controller|pkg-ty-controller|lint-fix|test|make' Makefile | head -200
elif [ -f makefile ]; then
rg -n 'pkg-test-controller|pkg-ty-controller|lint-fix|test|make' makefile | head -200
fi
echo "== test client/env package imports in operator tests =="
rg -n "sigs.k8s.io/controller-runtime/pkg/envtest|testing/memfs|k8s.io/client-go/kubernetes/fake|k8s.io/apimachinery/pkg/runtime/import|k8s.io/api" controller/deploy/operator -g '*_test.go' | head -200 || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 15538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'controller/deploy/operator/internal/controller/jumpstarter/.*_test\.go|controller/deploy/operator/internal/controller/jumpstarter/'
echo "== target file =="
if [ -f controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go ]; then
cat -n controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go
fi
echo "== operator test harness/envtest usage =="
rg -n "envtest|SetupEnv|StartEnv|controller\\.RuntimeConfig|Kubebuilder|s\\.Start|env\\.Start|CreateNamespace|DeleteNamespace|NewTestClient|ctrl\\.Run|ctrl\\.SetupWithManager" controller/deploy/operator/internal/controller controller/deploy/operator -g '*_test.go' | head -200 || true
echo "== make targets =="
if [ -f Makefile ]; then
rg -n 'pkg-test-controller|pkg-ty-controller|lint-fix|test|make' Makefile | head -200
elif [ -f makefile ]; then
rg -n 'pkg-test-controller|pkg-ty-controller|lint-fix|test|make' makefile | head -200
fi
echo "== test imports indicating stdlib/fake/envtest harness in operator tests =="
rg -n "sigs.k8s.io/controller-runtime/pkg/envtest|testing/memfs|k8s.io/client-go/kubernetes/fake|k8s.io/apimachinery/pkg/runtime/import|k8s.io/api" controller/deploy/operator -g '*_test.go' | head -200 || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 15551
Use the envtest harness for this operator test.
This file is under controller/deploy/operator/internal/controller/jumpstarter/*_test.go, so follow the operator test requirement to use envtest instead of the stdlib-only setup. Move or adapt this assertion into the existing envtest path and run make pkg-test-controller, make pkg-ty-controller, make lint-fix, and make test afterward.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@controller/deploy/operator/internal/controller/jumpstarter/controller_metrics_bind_test.go`
around lines 27 - 77, The stdlib-only TestControllerDeploymentMetricsBind
violates the operator test harness requirement. Move or adapt this assertion
into the existing envtest-based test setup for the Jumpstarter controller,
preserving validation of the metrics argument and port, then verify with the
requested controller tests, type checks, lint, and full test commands.
Source: Coding guidelines
| var b strings.Builder | ||
| families, err := reg.Gather() | ||
| if err != nil { | ||
| t.Fatalf("gather: %v", err) | ||
| } | ||
| for _, f := range families { | ||
| if _, err := expfmt.MetricFamilyToText(&b, f); err != nil { | ||
| t.Fatalf("encode: %v", err) | ||
| } | ||
| } | ||
| out := b.String() | ||
| if !strings.Contains(out, LeaseAcquisitionsTotal) { | ||
| t.Fatalf("OpenMetrics text missing %s:\n%s", LeaseAcquisitionsTotal, out) | ||
| } | ||
| if !strings.Contains(out, `result="success"`) { | ||
| t.Fatalf("OpenMetrics text missing result label:\n%s", out) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'lease_test\.go|metrics.*go|prometheus|go.mod' . | sed 's#^\./##' | head -80
echo "== target file excerpt =="
sed -n '1,220p' controller/internal/metrics/lease_test.go
echo "== module prometheus deps =="
if [ -f go.mod ]; then cat go.mod; fi
echo "== imports/usages for expfmt/OpenMetrics in metrics =="
rg -n "MetricFamilyToText|MetricFamilyToTextToGo|MetricFamilyToTextToOpenMetrics|OpenMetrics|model\.Exemplar|exemplar|prometheus_client|client_golang" -S .Repository: jumpstarter-dev/jumpstarter
Length of output: 21512
🌐 Web query:
prometheus/client_golang v1.22.0 MetricFamilyToText OpenMetrics
💡 Result:
In prometheus/client_golang v1.22.0, the function MetricFamilyToText is located in the prometheus/common/expfmt package [1][2]. It is specifically designed for the Prometheus text format and does not generate OpenMetrics output [3][4]. To handle OpenMetrics encoding, the library uses the expfmt.NewEncoder interface [4]. When using expfmt.NewEncoder with the TypeOpenMetrics format, the library calls MetricFamilyToOpenMetrics instead of MetricFamilyToText [4]. Additionally, while v1.22.0 supports a subset of OpenMetrics (such as exemplars) via opt-in configurations in promhttp.HandlerOpts, these features are considered experimental [5][6]. There is also a specific option, promhttp.HandlerOpts.EnableOpenMetricsTextCreatedSamples, which allows the creation of OpenMetrics _created lines, though this is not recommended due to significant cardinality increases [5][7].
Citations:
- 1: https://pkg.go.dev/github.com/prometheus/client_golang@v1.22.0
- 2: https://github.com/prometheus/client_golang/tree/v1.22.0
- 3: https://github.com/prometheus/common/blob/master/expfmt/text_create.go
- 4: https://github.com/prometheus/common/blob/main/expfmt/encode.go
- 5: https://github.com/prometheus/client_golang/blob/d72eeeffefb864253bb748447701a3b381f4080f/CHANGELOG.md
- 6: https://github.com/prometheus/client_golang/blob/886616e561a35e73eb39eb7241ca34ebe4ec956c/CHANGELOG.md
- 7: https://github.com/prometheus/client_golang/blob/9351edd797213e38dacc6a1dbd4514dab7abaafc/CHANGELOG.md
🌐 Web query:
prometheus/client_golang MetricFamilyToTextToOpenMetrics v1.22.0
💡 Result:
In the prometheus/client_golang library, functions related to metric parsing and encoding, such as MetricFamilyToText, are not located within the client_golang module itself [1][2]. Instead, these utilities are contained within the prometheus/common/expfmt package [1][3]. While prometheus/client_golang version 1.22.0 was released on April 7, 2025 [4][5], it does not introduce a specific function named MetricFamilyToTextToOpenMetrics. The primary functions for handling text-based exchange formats in the Prometheus ecosystem are typically found in the expfmt package, which includes: 1. MetricFamilyToText: Converts a MetricFamily protobuf message into the standard Prometheus text format [3]. 2. TextToMetricFamilies: Parses the Prometheus text format into MetricFamily protobuf messages [3][6]. Users seeking to interact with the OpenMetrics format or manipulate metric families generally utilize the tools provided by prometheus/common/expfmt [1][3]. Note that the client_golang library itself focuses on instrumentation and API client functionality, while common serialization and formatting logic is abstracted into the prometheus/common module [1][2]. If you are encountering a reference to a function named MetricFamilyToTextToOpenMetrics, it is likely a custom implementation or a misidentification of existing encoder/decoder functions within the expfmt package.
Citations:
- 1: https://pkg.go.dev/github.com/prometheus/client_golang@v1.22.0
- 2: https://github.com/prometheus/client_golang/tree/v1.22.0
- 3: https://pkg.go.dev/github.com/lidezhu/tidb/_vendor/src/github.com/prometheus/common/expfmt
- 4: https://github.com/prometheus/client_golang/releases/tag/v1.22.0
- 5: https://groups.google.com/g/prometheus-announce/c/ZJ1UWy4HE5M
- 6: https://qiita.com/fits/items/6e2dd785c7495cb37022
Use an actual OpenMetrics encoder.
MetricFamilyToText emits the Prometheus text format, not OpenMetrics, so TestOpenMetricsTextContainsSeries cannot catch OpenMetrics serialization bugs. Encode the gathered families with the OpenMetrics formatter/encoder and assert the OpenMetrics-specific parts, including exemplar labels and the OpenMetrics terminator.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/metrics/lease_test.go` around lines 136 - 151, The
TestOpenMetricsTextContainsSeries serialization flow currently uses
expfmt.MetricFamilyToText, which produces Prometheus text. Replace it with the
OpenMetrics formatter or encoder, then assert OpenMetrics-specific output
including exemplar labels and the required OpenMetrics end-of-stream terminator
while preserving the existing metric and result-label assertions.
| if adder, ok := metric.(prometheus.ExemplarAdder); ok { | ||
| adder.AddWithExemplar(1, labels) | ||
| return |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)controller/internal/metrics/lease\.go$' || true
echo "== file excerpt =="
if [ -f controller/internal/metrics/lease.go ]; then
nl -ba controller/internal/metrics/lease.go | sed -n '1,170p'
fi
echo "== prometheus imports/usages in metrics package =="
rg -n "ExemplarAdder|AddWithExemplar|prometheus|CounterVec|RecordAcquisition|client|lease_id" controller/internal/metrics -S || true
echo "== go.mod prometheus version =="
rg -n "github.com/prometheus/client" go.mod go.sum 2>/dev/null || true
echo "== read relevant imports around controller-runtime metrics =="
rg -n "controller-runtime|metrics|Register" controller -S --glob '*.go' | head -200 || trueRepository: jumpstarter-dev/jumpstarter
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,170p' controller/internal/metrics/lease.go
echo "== prometheus imports/usages and metric source values =="
rg -n "ExemplarAdder|AddWithExemplar|prometheus|CounterVec|RecordAcquisition|client|lease_id|RecordLose|recordLease" controller/internal/metrics controller -S | head -200
echo "== go module versions =="
if [ -f go.mod ]; then awk '/github.com\/prometheus\/client|github.com\/prometheus\/common|github.com\/prometheus\/procfs|github.com\/prometheus\/model/ {print}' go.mod go.sum 2>/dev/null | head -80 || true; fi
echo "== related metrics tests/files =="
find controller/internal/metrics -maxdepth 2 -type f \( -name '*.go' -o -name '*_test*' \) -print | sortRepository: jumpstarter-dev/jumpstarter
Length of output: 24353
🌐 Web query:
Prometheus Go client AddWithExemplar returns error invalid exemplar label too long
💡 Result:
The error "invalid exemplar label too long" in the Prometheus Go client occurs because the total length of all label names and values in an exemplar must not exceed 128 UTF-8 runes (characters) [1][2]. This limit is enforced by the OpenMetrics specification [2]. Key details regarding this error: - Limit Definition: The combined length of all exemplar label keys and their corresponding values is capped at 128 runes [1][2]. Note that this is a rune (character) count, not a byte count [1][2]. - Consequence: When this limit is exceeded, the exemplar is typically dropped to maintain compliance with the specification [3]. - Common Causes: This limit is often reached when excessive or high-cardinality metadata is attached to exemplars, such as full URLs or unnecessary filtered attributes, in addition to standard tracing identifiers like trace_id and span_id [4][3]. - Evolution: The limit was previously 64 runes and was raised to 128 runes in client_golang v1.13.1 to better align with the OpenMetrics specification [5][2][6]. There are ongoing community discussions regarding the restrictiveness of this limit and potential plans to evolve it in future versions, such as OpenMetrics 2.0 [4][7]. To resolve or avoid this error, you should minimize the number and length of labels added to your exemplars, prioritizing essential tracing information like trace_id and span_id [4][3]. If you are using instrumentation libraries (such as OpenTelemetry), check their configuration to see if they are automatically attaching additional labels that are pushing your exemplars over the 128-rune threshold [7][3].
Citations:
- 1: https://eb3fe27dx646f6373x636c6f7564x676f6f676c65x636f6d.gateway.web.tr/https/stackdriver/docs/managed-prometheus/exemplars
- 2: Raise exemplar labels limit from 64 to 128 prometheus/client_golang#1091
- 3: Prometheus Exporter adds filtered labels to exemplars, causing them to exceed exemplar size limit open-telemetry/opentelemetry-go#6718
- 4: Exemplar rune limit is quite restrictive, and overly punitive. prometheus/client_golang#1803
- 5: https://gitlab.com/gitlab-org/release-tools/-/merge_requests/2029
- 6: https://github.com/prometheus/client_golang/blob/master/CHANGELOG.md
- 7: Plan for how to relax exemplar max runes for OM 2.0 prometheus/client_golang#1953
🌐 Web query:
github.com/prometheus/client_golang/prometheus ExemplarAdder AddWithExemplar error validation
💡 Result:
In the Prometheus Go client (prometheus/client_golang), the ExemplarAdder interface defines the AddWithExemplar method, which is implemented by Counter types to record an exemplar alongside an observation [1][2]. Validation and Error Handling The AddWithExemplar method performs internal validation on the provided exemplar data. If validation fails, the method panics [1][2]. Specifically, AddWithExemplar will panic if any of the following conditions are met [1][2]: 1. The provided value is less than 0 [1][2]. 2. Any of the provided label names are invalid (e.g., they do not match the required Prometheus label format) [1][3]. 3. Any of the provided label values are not valid UTF-8 [3]. 4. The total number of runes in all label names and values combined exceeds the limit of 128 [1][2][3]. If the provided Labels argument is nil, the method leaves the current exemplar in place rather than replacing it [1][2]. Empty Labels (an empty map) are permitted by the validator and will result in a valid, label-less exemplar being stored [1][2]. Note that while the client allows empty label sets, there have been reports of issues where providing an empty label map could lead to invalid metric exposition formats depending on the scrape configuration [4]. Implementation Details The AddWithExemplar method updates the counter's value and subsequently calls updateExemplar [1][2]. Internally, updateExemplar uses a helper function, newExemplar, which performs the aforementioned validation and returns an error [1][3]. Because the ExemplarAdder interface requires AddWithExemplar to be signature-compliant with standard Go patterns that do not return errors, the client implementation panics when newExemplar returns an error [1][2]. As of early 2026, discussions are ongoing regarding how to relax these constraints (such as the 128-rune limit) in anticipation of future OpenMetrics protocols [5].
Citations:
- 1: https://github.com/prometheus/client_golang/blob/master/prometheus/counter.go
- 2: https://github.com/prometheus/client_golang/blob/main/prometheus/counter.go
- 3: https://github.com/prometheus/client_golang/blob/master/prometheus/value.go
- 4: Bug: empty exemplar labels results in invalid metrics format prometheus/client_golang#1333
- 5: Plan for how to relax exemplar max runes for OM 2.0 prometheus/client_golang#1953
Handle invalid exemplar values before adding.
AddWithExemplar rejects invalid exemplar labels/values (for example, over 128 runes), causing panic rather than returning an error. Since client and lease_id can be resource-derived names, trim/validate them or drop the exemplar on invalid input instead of letting AddWithExemplar crash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controller/internal/metrics/lease.go` around lines 81 - 83, Update the
exemplar path in the metric handling code around AddWithExemplar to validate or
constrain the client and lease_id labels before calling it, dropping the
exemplar when either value is invalid rather than allowing a panic. Preserve the
existing metric increment behavior and only omit the exemplar for invalid
resource-derived values.
| func NewLeaseMetrics() *LeaseMetrics { | ||
| return &LeaseMetrics{ | ||
| acquisitions: prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Name: LeaseAcquisitionsTotal, | ||
| Help: "Lease acquire attempts on the Jumpstarter controller.", | ||
| }, | ||
| []string{"result"}, | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Counter time series not pre-initialized with zero values. So, before any lease acquisition, scraping /metrics produces TYPE/HELP metadata but no data lines.
There was a problem hiding this comment.
Do you need to pre-initialize to 0 ? I assumed that was the default in go.
There was a problem hiding this comment.
Zero values in go - " Variables declared without an explicit initial value are given their zero value. "
There was a problem hiding this comment.
https://prometheus.io/docs/practices/instrumentation/#avoid-missing-metrics - its lazily initialized hence if you do do not prepopulate no graph will show.
| func setUnsatisfiableAndRecord(lease *jumpstarterdevv1alpha1.Lease, reason, messageFormat string, a ...any) { | ||
| already := meta.IsStatusConditionTrue( | ||
| lease.Status.Conditions, | ||
| string(jumpstarterdevv1alpha1.LeaseConditionTypeUnsatisfiable), | ||
| ) | ||
| lease.SetStatusUnsatisfiable(reason, messageFormat, a...) | ||
| if !already { | ||
| recordLeaseAcquisition(lease, jmpmetrics.ResultFailure) | ||
| } |
There was a problem hiding this comment.
Some test for the check that prevents double-counting on re-reconciliation would be nice.
| } | ||
|
|
||
| // Default is the process-wide lease metrics instance used by the controller. | ||
| var Default = NewLeaseMetrics() |
There was a problem hiding this comment.
All controller tests share the global counter, breaking test isolation.
There was a problem hiding this comment.
Don't those tests run in serial?, is it worth complicating the code in favor of testability? could be, just sharing the question here, I am not familiar with the prometheus metrics framework to be able to figure out if it'd get too complex, or if it really won't.
| lease.Status.ExporterRef = &corev1.LocalObjectReference{ | ||
| Name: selected.Exporter.Name, | ||
| } | ||
| recordLeaseAcquisition(lease, jmpmetrics.ResultSuccess) |
There was a problem hiding this comment.
The success metric is recorded here before status is persisted to the API server.
| func recordLeaseAcquisition(lease *jumpstarterdevv1alpha1.Lease, result string) { | ||
| exemplars := map[string]string{} | ||
| if lease != nil { | ||
| exemplars["lease_id"] = lease.Name |
There was a problem hiding this comment.
from the pov of exemplars and useful data, each lease_id is different, so it would not provide much statistical value.
What do you think about recording the assigned exporter name instead?
| func NewLeaseMetrics() *LeaseMetrics { | ||
| return &LeaseMetrics{ | ||
| acquisitions: prometheus.NewCounterVec( | ||
| prometheus.CounterOpts{ | ||
| Name: LeaseAcquisitionsTotal, | ||
| Help: "Lease acquire attempts on the Jumpstarter controller.", | ||
| }, | ||
| []string{"result"}, | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you need to pre-initialize to 0 ? I assumed that was the default in go.
| } | ||
|
|
||
| // Default is the process-wide lease metrics instance used by the controller. | ||
| var Default = NewLeaseMetrics() |
There was a problem hiding this comment.
Don't those tests run in serial?, is it worth complicating the code in favor of testability? could be, just sharing the question here, I am not familiar with the prometheus metrics framework to be able to figure out if it'd get too complex, or if it really won't.
3 participants
jumpstarter_lease_acquisitions_total{result}on the Jumpstarter controller/metricsendpoint with Prometheus exemplars (default keysclient,lease_id).-metrics-bind-address=:8080; no Router metrics flags in this PR.