Add in-flight and richer duration Prometheus metrics on all Porch API operations by JamesMcDermott · Pull Request #1198 · kptdev/porch

JamesMcDermott · 2026-09-21T16:45:15Z

Description

What changed:
- enhance the existing API-call-duration histogram (porch_api_call_duration_seconds) into a Porch-API-operation-duration metric
  - adding new labels
    - operation
    - operation_outcome
    - lifecycle_after
    - package-revision key labels:
      - namespace
      - repository
      - package
      - workspace_name
- add a new porch_in_flight_api_operations gauge (an Int64UpDownCounter) to track
  currently-in-progress Porch operations
  - wired into both porch-server (v1alpha1 REST) flows and v1alpha2 controller reconcile loop
- new E2E tests in the v1alpha1 (api) suite and counterparts in the v1alpha2 (crd) suiterefactoring
Why it’s needed:
- improves insight into Porch API behaviour, allowing more detailed breakdown of operations according to
  per-operation/outcome/lifecycle
- improves visibility into in-progress operations.
- making it easier to observe latency per operation, spot error rates, or detect stuck/leaked
  operations
How it works:
- operation instrumentation:
  - records duration and outcome via improved RecordAPIOperationDuration/RecordControllerOperation
  - brackets each operation with new TrackInFlightOperation/TrackInFlightControllerOperation, which increments the in-flight gauge at start and decrements it via a deferred closure on completion (including error paths).

Type of Change

Checklist

Code follows project style guidelines
Self-reviewed changes
Tests added/updated
Documentation added/updated (will raise separate PR)
All tests and gating checks pass

Testing Instructions (Optional)

Run the unit tests: make test
Deploy Porch with all components in-cluster and run the appropriate E2E suite:
with v1alpha1
1. make setup-dev-env run-in-kind-db-cache test-e2e
with v1alpha2
1. make setup-dev-env run-in-kind-v1alpha2 test-e2e-crd
Scrape a porch-server / porch-controllers pod's /metrics endpoint and confirm the presence of
porch_api_call_duration_seconds_{bucket,count,sum} with the new labels and
porch_in_flight_api_operations settling to 0 once operations complete.

Additional Notes (Optional)

Known issues:
- The porch_in_flight_api_operations in-flight proof (the spammer/scraper approach) is only exercised in the v1alpha1 (API) suite; the v1alpha2 (CRD) suite asserts steady-state and per-operation presence but not mid-flight increment, because controller operations are driven by asynchronous reconciliation rather than synchronous, re-runnable REST calls.
- The e2e metrics tests currently require an all-in-cluster deployment and are skipped otherwise.
Review notes:
- source_test.go asserts the selected source action via fmt.Sprintf("%#v", ...) function-value
  comparison; this is intentional but relies on stable function formatting.

AI Disclosure

I have used AI in the creation of this PR.

If so, please describe how:

Kiro/Amazon Q to:
- analyse changes and draft this pull request description
- help design and generate the e2e metrics tests
- refactor the E2E tests' metrics collection/assertion helpers to share between API and CRD suites
- extract lifecycle resolution routines out into common functions

netlify · 2026-09-21T16:45:21Z

✅ Deploy Preview for kpt-porch canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`63caa82`
🔍 Latest deploy log	https://app.netlify.com/projects/kpt-porch/deploys/6af4dcf2048f923b5db70325

Copilot

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings affect metric correctness, test reliability, and existing performance dashboards.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 · 12 · 2

Open (17)

Use approval resource label for in-flight metric · New
Classify deletion errors without unsafe type assertion · New
Validate all required pods before dereferencing · New
Use consistent resource labels for in-flight and duration metrics · New
Record successful not-found cleanup with success outcome · New
Label lifecycle duration with resulting lifecycle · New
Capture render and stale-path errors in operation outcome · New
Capture list failures in deferred operation outcome · New
Avoid shadowing errors in delete outcome instrumentation · New
Avoid shadowing list errors in duration instrumentation · New
Handle missing scrape result before indexing · New
Defer metric evaluation until Eventually polling · New
Clear historical samples before settled-state assertion · New
Make CreateL log errors without failing tests · New
Preserve performance histogram or update dashboard consumers · New
Instrument malformed Create requests before type assertion · New
Instrument namespace validation failures · New

What changed in this PR

Adds richer Porch API duration metrics and in-flight operation tracking across REST and controller paths, with expanded E2E coverage and monitoring fixes.

Changes:

Adds operation, outcome, lifecycle, and package-revision labels.
Instruments REST and controller operations with in-flight gauges.
Updates telemetry helpers, tests, and monitoring paths.

File	Description
`test/performance/metrics_utils.go`	Routes performance durations through telemetry.
`test/e2e/suiteutils/suite.go`	Adds logging-oriented client helpers.
`test/e2e/suiteutils/suite_utils.go`	Refactors metrics collection and parsing.
`test/e2e/crd/suite_test.go`	Initializes the Kubernetes client.
`test/e2e/crd/metrics_test.go`	Adds controller metrics tests.
`test/e2e/crd/helpers_test.go`	Adds deletion polling support.
`test/e2e/api/metrics_test.go`	Adds API metrics and in-flight tests.
`scripts/monitoring/deploy-monitoring.sh`	Fixes monitoring resource paths.
`pkg/registry/porch/packagerevisionresources.go`	Instruments resource operations.
`pkg/registry/porch/packagerevisionresources_update_test.go`	Updates resource-operation mocks.
`pkg/registry/porch/packagerevisionresources_test.go`	Updates lifecycle mocks.
`pkg/registry/porch/packagerevision.go`	Instruments PackageRevision operations.
`pkg/registry/porch/packagerevision_test.go`	Updates revision-operation mocks.
`pkg/registry/porch/packagerevision_approval.go`	Instruments approval operations.
`pkg/registry/porch/packagerevision_approval_test.go`	Updates approval-operation mocks.
`pkg/registry/porch/packagecommon.go`	Returns typed updated revisions.
`internal/telemetry/metrics.go`	Defines enriched metrics.
`internal/telemetry/metrics_test.go`	Updates telemetry tests.
`controllers/packagerevisions/pkg/controllers/packagerevision/source.go`	Refactors source action selection.
`controllers/packagerevisions/pkg/controllers/packagerevision/source_test.go`	Tests source action selection.
`controllers/packagerevisions/pkg/controllers/packagerevision/render.go`	Instruments render operations.
`controllers/packagerevisions/pkg/controllers/packagerevision/packagerevision_controller.go`	Instruments reconciliation.
`controllers/packagerevisions/pkg/controllers/packagerevision/ownership.go`	Instruments deletion cleanup.
`controllers/packagerevisions/pkg/controllers/packagerevision/metadata.go`	Instruments metadata updates.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

liamfallon · 2026-10-22T11:30:58Z

+	telemetry.RecordRequestCount(ctx, prrTelemetryName, op.AllCaps, telemetry.APIVersionV1Alpha1)

-	name, resourceSelector, err := selector.ParsePRRGet(rawName)
+	parsedName, resourceSelector, err := selector.ParsePRRGet(pkgRevK8sName)


You have a lot of changes like this which, while improving the readability of the PR, are not related tot he actual telemetry changes. They make the PR much bigger and harder to review.

liamfallon · 2026-10-22T12:15:51Z

 		return false, err
 	}

+	saveErr := func(loseableErr error) error { err = loseableErr; return err }


This line can be deleted

liamfallon · 2026-10-22T12:16:25Z

+		telemetry.RecordControllerOperation(ctx, prrTelemetryName, op.AllCaps, op.TitleCase+prrTelemetryName, time.Since(start), err, pr.Spec.Lifecycle, &key)
+	}()
+
 	// Create draft and write updated resources.


Add this line here:

var draft repository.PackageRevisionDraftSlim

liamfallon · 2026-10-22T12:16:35Z

+	}()
+
 	// Create draft and write updated resources.
 	draft, err := r.ContentCache.CreateDraftFromExisting(ctx, repoKey, pr.Spec.PackageName, pr.Spec.WorkspaceName)


Suggested change

draft, err := r.ContentCache.CreateDraftFromExisting(ctx, repoKey, pr.Spec.PackageName, pr.Spec.WorkspaceName)

draft, err = r.ContentCache.CreateDraftFromExisting(ctx, repoKey, pr.Spec.PackageName, pr.Spec.WorkspaceName)

liamfallon · 2026-10-22T12:16:48Z

 	// UpdateResources is a full replace — include all files to avoid data loss.
 	resources["Kptfile"] = string(updatedKfBytes)
 	log.Info("metadata sync writing resources", "resourceCount", len(resources))
 	if err := draft.UpdateResources(ctx, resources, "metadata-sync"); err != nil {


Suggested change

if err := draft.UpdateResources(ctx, resources, "metadata-sync"); err != nil {

if err = draft.UpdateResources(ctx, resources, "metadata-sync"); err != nil {

liamfallon · 2026-10-22T12:17:03Z

 	if err != nil {
 		log.Error(err, "failed to create draft")
-		return false, err
+		return false, saveErr(err)


Suggested change

return false, saveErr(err)

return false, err

liamfallon · 2026-10-22T12:17:17Z

 	if err := draft.UpdateResources(ctx, resources, "metadata-sync"); err != nil {
 		log.Error(err, "failed to write resources")
-		return false, err
+		return false, saveErr(err)


Suggested change

return false, saveErr(err)

return false, err

liamfallon · 2026-10-22T12:17:28Z

 	if err := r.ContentCache.CloseDraft(ctx, repoKey, draft, 0); err != nil {
 		log.Error(err, "failed to close draft")
-		return false, err
+		return false, saveErr(err)


Suggested change

return false, saveErr(err)

return false, err

Please check other places where this saveErr pattern is used and change the code to not use local block err variables.

liamfallon · 2026-10-22T13:39:00Z

+	saveErr := func(loseableErr error) error { err = loseableErr; return err }
+	op := telemetry.Operations.Update
+	start := time.Now()
+	key := telemetry.ParsePkgRevK8sName2Key(pr.Namespace, pr.Name)


What does this function do that key.PkgRevK8sName2Key does not do?

liamfallon · 2026-10-22T13:48:43Z

+	defer telemetry.TrackInFlightControllerOperation(ctx, prrTelemetryName, op.AllCaps, op.TitleCase+prrTelemetryName, pr.Spec.Lifecycle, &key)()
+	defer func() {
+		telemetry.RecordControllerOperation(ctx, prrTelemetryName, op.AllCaps, op.TitleCase+prrTelemetryName, time.Since(start), err, pr.Spec.Lifecycle, &key)
+	}()


I'd really like to boil this down to two function calls.

- enhance existing API-call-duration histogram to include more information from Porch perspective - making it a Porch-API-operation-duration metric - add new metric to track in-flight (currently-in-progress) Porch operations Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

- wire porch_in_flight_api_operations gauge into v1alpha2 controller reconciler operations - track lifecycle, source, metadata-sync, render, and delete reconcile paths - Published lifecycle transitions recorded under PackageRevisionApproval. - refactor applySource into selectPackageSourceAction + execution so the operation type (init/clone/copy/upgrade) is known for telemetry; update source_test.go accordingly. - new E2E tests TestAPIOperationDurationMetricExists, TestAPIOperationDurationLabels, and TestInFlightOperationsMetric in v1alpha1 suite - counterparts in v1alpha2 suite testing against PorchControllerMetrics. - small fix in deploy-monitoring.sh config paths Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

in case of errors - fix unit tests to account for extra calls to GetPackageRevision(), Lifecycle(), etc. Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

- cover cases where error occurs but is hidden from telemetry calls - prevent recording errored operations as success - revisits lifecycle acquisition for telemetry calls - make it as accurate as possible without extra queries to Git or database - changes to packagecommon.updatePackageRevision() to include existing lifecycle in returns - only way to get it back into higher-level Update() methods to include in telemetry Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

- also fix build Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

…hods - as per SonarQube duplication report Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

- wire contexts into metric recording functions - add explicit saveErr calls into API methods' early returns - ensure packageRevisions.Create() has in-flight recording for earliest possible return - ensure controller's reconcileLifecycle doesn't leak in-flight counter for ContentCache.Get() - when recording in case package revision key is nil, at least get namespace from context and record that - for LIST PackageRevision, pluralise telemetry name to get "ListPackageRevisions" operation Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

- also add dedicated function for name-to-key parsing to get PackageRevisionKeys for metrics Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

sonarqubecloud · 2026-11-11T21:27:40Z

Quality Gate failed

Failed conditions
79.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

JamesMcDermott requested review from a team and a lite review from Copilot September 21, 2026 16:45

JamesMcDermott requested review from efiacor, kispaljr, liamfallon and mozesl-nokia as code owners September 21, 2026 16:45

Copilot started reviewing on behalf of JamesMcDermott September 21, 2026 16:45

View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

JamesMcDermott force-pushed the feature-porch-operation-metrics branch from 31ccc38 to d4d35c0 Compare September 21, 2026 16:53

JamesMcDermott changed the title ~~Add in-flight and richer duration Prometheus metrics on all Porch API operations~~ Add in-flight and richer duration Prometheus metrics on all Porch API operations Sep 23, 2026

kushnaidu reviewed Sep 23, 2026

View reviewed changes

Comment thread pkg/registry/porch/packagerevision.go Resolved

kushnaidu reviewed Sep 23, 2026

View reviewed changes

Comment thread pkg/registry/porch/packagerevision_approval.go Resolved

kushnaidu reviewed Sep 23, 2026

View reviewed changes

Comment thread pkg/registry/porch/packagerevision_approval.go Resolved

kushnaidu reviewed Sep 23, 2026

View reviewed changes

Comment thread pkg/registry/porch/packagerevision.go Resolved

This comment was marked as resolved.

Sign in to view

efiacor reviewed Oct 12, 2026

View reviewed changes

JamesMcDermott mentioned this pull request Oct 13, 2026

Docs: update docs for API operation histogram and in-flight counter; add dashboard for in-flight operations #1281

Merged

6 of 13 tasks

liamfallon reviewed Oct 22, 2026

View reviewed changes

JamesMcDermott added 8 commits November 11, 2026 21:15

Fix tests and update Update flows to cover unchanged lifecycles

9a32bdc

in case of errors - fix unit tests to account for extra calls to GetPackageRevision(), Lifecycle(), etc. Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Add telemetry to controllers' subpackage operation

53cd13b

- also fix build Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Linter fixes and save errors for telemetry in subpackage reconcile

5ff480f

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

More linting fixes

12a0ee5

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Extract lifecycle resolution for metric recording out to separate met…

6043b8c

…hods - as per SonarQube duplication report Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

JamesMcDermott added 6 commits November 11, 2026 21:15

Linter fix

1217a9f

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Linter fix

1a676cc

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

Fix repository/key build troubles from rebase

84ff1ab

- also add dedicated function for name-to-key parsing to get PackageRevisionKeys for metrics Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

fix tests; update missed copyright notices

306b5e2

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

rebase and fix tests

63caa82

Signed-off-by: James McDermott <james.j.mcdermott@ericsson.com>

JamesMcDermott force-pushed the feature-porch-operation-metrics branch from 490f9f5 to 63caa82 Compare November 11, 2026 21:23

	draft, err := r.ContentCache.CreateDraftFromExisting(ctx, repoKey, pr.Spec.PackageName, pr.Spec.WorkspaceName)
	draft, err = r.ContentCache.CreateDraftFromExisting(ctx, repoKey, pr.Spec.PackageName, pr.Spec.WorkspaceName)

	if err := draft.UpdateResources(ctx, resources, "metadata-sync"); err != nil {
	if err = draft.UpdateResources(ctx, resources, "metadata-sync"); err != nil {

Conversation

JamesMcDermott commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Checklist

Testing Instructions (Optional)

Additional Notes (Optional)

AI Disclosure

Uh oh!

netlify Bot commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kpt-porch canceled.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamfallon Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamfallon Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Nov 11, 2026

Quality Gate failed

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

JamesMcDermott commented Sep 21, 2026 •

edited

Loading

netlify Bot commented Sep 21, 2026 •

edited

Loading

liamfallon Oct 22, 2026 •

edited

Loading

liamfallon Oct 22, 2026 •

edited

Loading