WIP: feat(releaseinfo): add Prometheus metrics for mirror lookup instrumentation#9035
WIP: feat(releaseinfo): add Prometheus metrics for mirror lookup instrumentation#9035ironcladlou wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThe releaseinfo package defines and registers Prometheus histograms for mirror lookup and mutex wait durations, plus a counter vector for lookup outcomes. The lookup path records timing, mirror hits, unavailable mirrors, errors, and fallback behavior. The operator enables Go mutex profiling with a fixed sampling fraction at the start of Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ironcladlou The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/main.go (1)
248-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the mutex profile sampling rate constant.
runtime.SetMutexProfileFraction(5)should use a package-level constant, e.g.mutexProfileSamplingRate, so the profiling policy is explicit and easier to review.🤖 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 `@hypershift-operator/main.go` at line 248, Define a package-level constant for the mutex profiling sampling rate and update the runtime.SetMutexProfileFraction call to use it instead of the literal 5. Name the constant mutexProfileSamplingRate or an equivalent descriptive symbol.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@hypershift-operator/main.go`:
- Line 248: Define a package-level constant for the mutex profiling sampling
rate and update the runtime.SetMutexProfileFraction call to use it instead of
the literal 5. Name the constant mutexProfileSamplingRate or an equivalent
descriptive symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 47511dd8-d90b-4695-9cf8-5e2e995f5dd9
📒 Files selected for processing (3)
hypershift-operator/main.gosupport/releaseinfo/metrics.gosupport/releaseinfo/registry_image_content_policies.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9035 +/- ##
==========================================
+ Coverage 43.28% 44.12% +0.83%
==========================================
Files 771 780 +9
Lines 95503 98091 +2588
==========================================
+ Hits 41335 43279 +1944
- Misses 51284 51777 +493
- Partials 2884 3035 +151
... and 82 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| p.lock.Lock() | ||
| defer p.lock.Unlock() | ||
| lockWaitDuration.Observe(time.Since(start).Seconds()) | ||
| defer func() { lookupDuration.Observe(time.Since(start).Seconds()) }() |
There was a problem hiding this comment.
Both lockWaitDuration and lookupDuration use the same start timestamp, so lookupDuration includes the time spent waiting for the mutex. That makes it hard to tell whether high latency comes from lock contention or from slow registry I/O — which is exactly what having two separate metrics should help with.
Would it make sense to capture a second timestamp after the lock is acquired? If the intent is to measure total wall-clock time (wait + work), that's fine too — but then the metric name/help should say "total duration including lock wait" so it's unambiguous.
There was a problem hiding this comment.
I meant for it to be wall clock time, you could still get the granular time by subtracting lock duration from lookup duration. Could add another granular timing. In either case the docs can be updated
| // Verify mirror image availability. | ||
| if _, _, err = repoSetup(ctx, replacedImage, pullSecret); err == nil { | ||
| p.mirroredReleaseImage = replacedImage | ||
| mirrorLookupTotal.WithLabelValues("mirror_hit").Inc() |
There was a problem hiding this comment.
nit: these label values are free-form strings with no compile-time protection — a typo like "mirror_Hit" would silently create a new series. Might be worth pulling them into constants.
There was a problem hiding this comment.
Constants added
|
|
||
| func run(ctx context.Context, opts *StartOptions, log logr.Logger) error { | ||
| log.Info("Starting hypershift-operator-manager", "version", supportedversion.String()) | ||
| runtime.SetMutexProfileFraction(5) |
There was a problem hiding this comment.
This enables mutex profiling for the entire HO process (not just the releaseinfo path), always on, with no opt-out. Totally reasonable for passive pprof visibility, but without a comment it reads like a debugging leftover that someone might remove later. A one-liner explaining the intent would help.
Also — minor thing — the PR description says "20% sampling" but it might be worth noting that 5 means "1 in 5 contention events sampled," just so future readers don't have to look it up.
8fb20a7 to
c4bd5a3
Compare
…tation Replace ad-hoc debug logging with Prometheus histograms and counters for release image mirror lookup duration, mutex contention, and per-mirror outcomes. Enable Go runtime mutex profiling at 20% sampling for passive contention visibility via pprof. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c4bd5a3 to
d68500c
Compare
ironcladlou
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@ironcladlou: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Replace ad-hoc debug logging with Prometheus histograms and counters for release image mirror lookup duration, mutex contention, and per-mirror outcomes. Enable Go runtime mutex profiling at 20% sampling for passive contention visibility via pprof.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Performance / Diagnostics
Bug Fixes