feat(catalogs): expose catalog controller metrics#1787
Open
abhijith-darshan wants to merge 1 commit intomainfrom
Open
feat(catalogs): expose catalog controller metrics#1787abhijith-darshan wants to merge 1 commit intomainfrom
abhijith-darshan wants to merge 1 commit intomainfrom
Conversation
abhijith-darshan
force-pushed
the
feat/catalog_metrics
branch
from
e00e0f8 to
8f3abe8
Compare
IvoGoman
reviewed
Feb 11, 2026
Contributor
IvoGoman
left a comment
IvoGoman
left a comment
There was a problem hiding this comment.
It would be cool if this also added a metric to the PluginDefinition regarding it's ready status.
e.g. greenhouse_plugindefintion_ready{"plugindefinition", "organization", "catalog", "reason"} & one for the clusterplugindefinition
Where catalog is filled when available. That way we could have a overall perses dashboard that shows PluginDefinitions managed by Catalogs as well.
abhijith-darshan
force-pushed
the
feat/catalog_metrics
branch
2 times, most recently
from
305d140 to
a1e62d7
Compare
abhijith-darshan
force-pushed
the
feat/catalog_metrics
branch
2 times, most recently
from
980455f to
7c3d60f
Compare
IvoGoman
reviewed
Feb 24, 2026
📝 WalkthroughWalkthroughAdds a Prometheus readiness metric for Catalogs, computes a readiness reason (SecretMissing, AuthError, Unknown/OK) from inventory and Ready condition, exposes update/delete helpers, and integrates metric updates into the catalog reconciler (deferred update on create, delete on removal). Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Reconciler
participant Metrics as "Metrics Package"
participant Prometheus as "Prometheus Registry"
rect rgba(200,230,255,0.5)
Controller->>Reconciler: EnsureCreated(obj)
Reconciler->>Reconciler: reconciliation logic (update resources)
Reconciler-->>Controller: reconciliation result
end
rect rgba(200,255,200,0.5)
Reconciler->>Metrics: updateCatalogReadyMetric(catalog) [deferred]
Metrics->>Metrics: determineReason(catalog)
Metrics->>Prometheus: readyGauge.With(labels).Set(1 or 0)
end
rect rgba(255,230,200,0.5)
Controller->>Reconciler: EnsureDeleted(obj)
Reconciler->>Metrics: deleteCatalogReadyMetric(catalog)
Metrics->>Prometheus: readyGauge.DeleteLabelValues(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🧹 Nitpick comments (1)
internal/controller/catalog/catalog_controller.go (1)
379-379: Remove unnecessaryunparamsuppression.Line 379 suppresses
unparam, butverifyStatusparameters are actively used. Keeping this suppression lowers lint signal quality.Suggested cleanup
-//nolint:unparam func (r *CatalogReconciler) verifyStatus(ctx context.Context, catalog *greenhousev1alpha1.Catalog, allErrors []catalogError) (ctrl.Result, lifecycle.ReconcileResult, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/catalog/catalog_controller.go` at line 379, Remove the unnecessary nolint:unparam directive on the verifyStatus declaration: the function's parameters are used so the suppression should be deleted. Locate the verifyStatus function in catalog_controller.go (the line with "//nolint:unparam") and delete that nolint comment so the linter can properly report real unparam issues elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/catalog/catalog_controller.go`:
- Around line 215-217: Ensure the catalog readiness metric is cleaned up when a
Catalog CR is deleted by adding metric removal logic to EnsureDeleted: use the
same identifying fields used in UpdateCatalogReadyMetric (e.g., catalog.Name and
catalog.Namespace from the Catalog object) to remove the
greenhouse_catalog_ready series. Either call a new helper
DeleteCatalogReadyMetric(catalog) or extend UpdateCatalogReadyMetric to accept a
deletion flag, and invoke that from EnsureDeleted in the CatalogReconciler to
guarantee the metric is removed when the CR is gone.
In `@internal/controller/catalog/metrics.go`:
- Around line 46-51: The code sets reason = "OK" for ready catalogs but can also
emit "OK" for NOT-ready catalogs when determineReason(catalog) returns "OK";
update both places that compute the reason (the blocks using isReady and
determineReason) so that when isReady is false you use determineReason(catalog)
and if that returns "OK" replace it with a distinct not-ready marker (e.g.,
"UNKNOWN_NOT_READY" or empty string) instead of "OK" so not-ready catalogs never
report "OK". Ensure the same change is applied to the duplicate logic later in
the file.
---
Nitpick comments:
In `@internal/controller/catalog/catalog_controller.go`:
- Line 379: Remove the unnecessary nolint:unparam directive on the verifyStatus
declaration: the function's parameters are used so the suppression should be
deleted. Locate the verifyStatus function in catalog_controller.go (the line
with "//nolint:unparam") and delete that nolint comment so the linter can
properly report real unparam issues elsewhere.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/catalog/catalog_controller.gointernal/controller/catalog/metrics.gointernal/controller/catalog/source.go
💤 Files with no reviewable changes (1)
- internal/controller/catalog/source.go
abhijith-darshan
force-pushed
the
feat/catalog_metrics
branch
from
591fb59 to
01a9da9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/controller/catalog/metrics.go (1)
102-110:⚠️ Potential issue | 🟠 MajorNot-ready catalogs can still be emitted as
reason="OK"On Line 109, fallback returns
"OK"even when the catalog is not ready. Since Line 50 uses this for non-ready states, you can emitvalue=0withreason="OK", which breaks reason-based alerts/dashboards.Suggested fix
- // Reason values: OK (ready), AuthError (auth issues), SecretMissing (missing secrets) + // Reason values: OK (ready), AuthError (auth issues), SecretMissing (missing secrets), OtherError (non-auth readiness failures) - for _, r := range []string{"OK", "AuthError", "SecretMissing"} { + for _, r := range []string{"OK", "AuthError", "SecretMissing", "OtherError"} { - return "OK" + return "OtherError"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/catalog/metrics.go` around lines 102 - 110, The current fallback returns "OK" even for non-ready catalogs (the block with returns for hasMissingSecret and hasAuthError falls through to return "OK"), which causes non-ready metrics to be emitted with reason="OK"; update the logic so that if the catalog is not ready you return a distinct reason (e.g. "NotReady" or "Initializing") instead of "OK" — change the final return that currently returns "OK" to return a non-ready reason when readiness is false (use the same readiness check used where value=0 is emitted), keeping the existing hasMissingSecret/hasAuthError branches intact (refer to the hasMissingSecret, hasAuthError variables and the return "OK" in this function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/controller/catalog/metrics.go`:
- Around line 102-110: The current fallback returns "OK" even for non-ready
catalogs (the block with returns for hasMissingSecret and hasAuthError falls
through to return "OK"), which causes non-ready metrics to be emitted with
reason="OK"; update the logic so that if the catalog is not ready you return a
distinct reason (e.g. "NotReady" or "Initializing") instead of "OK" — change the
final return that currently returns "OK" to return a non-ready reason when
readiness is false (use the same readiness check used where value=0 is emitted),
keeping the existing hasMissingSecret/hasAuthError branches intact (refer to the
hasMissingSecret, hasAuthError variables and the return "OK" in this function).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/catalog/catalog_controller.gointernal/controller/catalog/metrics.gointernal/controller/catalog/source.go
💤 Files with no reviewable changes (1)
- internal/controller/catalog/source.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/catalog/catalog_controller.go
IvoGoman
previously approved these changes
Mar 3, 2026
abhijith-darshan
force-pushed
the
feat/catalog_metrics
branch
from
01a9da9 to
9fad9d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/catalog/metrics.go`:
- Around line 44-47: The cleanup logic only deletes series for the current
ownedBy label value, leaving historical series behind; update the cleanup calls
that build label matchers (the code that computes ownedBy from catalog.Labels
and the subsequent DeleteSeries/cleanup calls) to remove/omit the owned_by label
from the matcher so deletion targets all series for the catalog+namespace
regardless of owned_by (i.e., build matchers using catalog.Name and
catalog.Namespace only, or explicitly delete across all owned_by values), and
apply the same change to the other occurrences referenced (the blocks around the
ownedBy usage at the other locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93268c46-9614-4fa1-b188-07efe0a09e5a
📒 Files selected for processing (3)
internal/controller/catalog/catalog_controller.gointernal/controller/catalog/metrics.gointernal/controller/catalog/source.go
💤 Files with no reviewable changes (1)
- internal/controller/catalog/source.go
Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): set secret missing and repo auth metrics Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): set catalog ready metric Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): add license header Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): update catalog object metrics func name Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): use simple if Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): suppress unparam lint issue Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): consolidate metrics with reason grouping Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): handle clean up of metrics when resource is deleted Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com> (chore): remove lint overrides Signed-off-by: abhijith-darshan <abhijith.ravindra@sap.com>
abhijith-darshan
force-pushed
the
feat/catalog_metrics
branch
from
9fad9d8 to
e812997
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/controller/catalog/metrics.go (2)
44-47:⚠️ Potential issue | 🟠 MajorClean up previous
owned_bylabel values too.The cleanup only targets the current
owned_byvalue. If that label changes, the old time series is left behind, so this metric can report the same catalog under multiple owners until the process restarts. Please either track the previous owner per catalog or delete series without matching onowned_by.Also applies to: 61-64, 76-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/catalog/metrics.go` around lines 44 - 47, The cleanup currently only deletes time series matching the current ownedBy value (ownedBy := catalog.Labels[greenhouseapis.LabelKeyOwnedBy]), leaving old owner series behind; update the deletion logic so it does not include the owned_by label in the Prometheus/TS deletion matcher (i.e., match on the catalog identity fields only, not owned_by) or alternatively track the previous owner per catalog and delete by that previous value — apply this change to all similar cleanup blocks that reference ownedBy (the blocks around the ownedBy assignment and the cleanup code at the other two places noted).
101-106:⚠️ Potential issue | 🟡 MinorDrop the bare
"auth"substring match here.
source.Messageis freeform text, so matching any"auth"substring is too broad and can misclassify non-auth failures asAuthError. Keep this to specific phrases only, or switch to a structured reason once that data is available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/catalog/metrics.go` around lines 101 - 106, The substring check for "auth" is too broad and should be removed from the Auth error detection condition; in the authentication detection block that examines the variable msg (the strings.Contains checks that determine an AuthError), delete the strings.Contains(msg, "auth") clause and leave the specific checks (e.g., "authentication", "unauthorized", "forbidden", "credentials"); ensure the conditional still maps to AuthError correctly and run/update any tests referencing this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/controller/catalog/metrics.go`:
- Around line 44-47: The cleanup currently only deletes time series matching the
current ownedBy value (ownedBy :=
catalog.Labels[greenhouseapis.LabelKeyOwnedBy]), leaving old owner series
behind; update the deletion logic so it does not include the owned_by label in
the Prometheus/TS deletion matcher (i.e., match on the catalog identity fields
only, not owned_by) or alternatively track the previous owner per catalog and
delete by that previous value — apply this change to all similar cleanup blocks
that reference ownedBy (the blocks around the ownedBy assignment and the cleanup
code at the other two places noted).
- Around line 101-106: The substring check for "auth" is too broad and should be
removed from the Auth error detection condition; in the authentication detection
block that examines the variable msg (the strings.Contains checks that determine
an AuthError), delete the strings.Contains(msg, "auth") clause and leave the
specific checks (e.g., "authentication", "unauthorized", "forbidden",
"credentials"); ensure the conditional still maps to AuthError correctly and
run/update any tests referencing this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a18cf51-3948-449e-b65b-8f5f4f3c913b
📒 Files selected for processing (3)
internal/controller/catalog/catalog_controller.gointernal/controller/catalog/metrics.gointernal/controller/catalog/source.go
💤 Files with no reviewable changes (1)
- internal/controller/catalog/source.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/catalog/catalog_controller.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
2 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.
Description
expose Catalog Controller metrics
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added tests?
Added to documentation?
Checklist
Summary by CodeRabbit