Skip to content

feat(agents): incident-auto-cleanup-phase4b-metrics-full - #17

Merged
mashkovd merged 2 commits into
mainfrom
feat/agents-incident-auto-cleanup-phase4b-metrics-full
May 7, 2026
Merged

mashkovd merged 2 commits into
mainfrom
feat/agents-incident-auto-cleanup-phase4b-metrics-full

Conversation

@mashkovd

@mashkovd mashkovd commented May 7, 2026

Copy link
Copy Markdown
Member

Implements accepted proposal mctl-agent/incident-auto-cleanup-phase4b-metrics-full.

Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-agent/proposals/incident-auto-cleanup-phase4b-metrics-full/

Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.

mctl-agents Bot added 2 commits May 7, 2026 07:20
platform-gitops/bootstrap/templates/mctl-platform/mctl-agent-monitor.yaml
was added by the Phase 4a implementer whose sandbox could not push to
mctl-gitops. The canonical copy already lives at the same path in
mctl-gitops main (bac7212). The mctl-agent-side copy is dead bytes.

Proposal: platform-gitops/agents-state/mctl-agent/proposals/incident-auto-cleanup-phase4b-metrics-full/
Extends internal/metrics/metrics.go with OrphanPruned (counter),
AMReconcileResolved (counter), CleanupSkipped (counter vec by reason),
AMRequestDuration (histogram vec by outcome), and OpenTickets (gauge
vec by status/source). The init() pre-populates all known label
combinations so series appear at zero on first scrape.

Wires the counters into pruneOrphans() and reconcileWithAlertManager()
in poller.go, adds request-duration observation in
alertmanager_client.go via a deferred outcome-tracking closure, and
appends the OpenTicketBreakdown gauge refresh at the end of each poll
cycle.

Adds Store.OpenTicketBreakdown() (a grouped SELECT on non-terminal
tickets) to internal/ticket/store.go.

Extends 7 existing tests with counter delta assertions and adds 3 new
tests: TestAMRequestDurationObservesOutcomes,
TestStoreOpenTicketBreakdown, TestPollerUpdatesOpenTicketsGauge.

Proposal: platform-gitops/agents-state/mctl-agent/proposals/incident-auto-cleanup-phase4b-metrics-full/

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f41defca5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines -1 to -2
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore ServiceMonitor manifest for agent metrics

This commit deletes the only ServiceMonitor template for mctl-agent and does not add an equivalent replacement, so Prometheus will stop scraping /metrics after bootstrap sync. That makes all newly added cleanup/Alertmanager metrics effectively invisible in-cluster, which breaks the monitoring path this change is introducing.

Useful? React with 👍 / 👎.

// Pre-populate AMRequestDuration with a synthetic zero observation so
// the histogram series appears at first scrape for all outcome labels.
for _, outcome := range []string{"success", "http_error", "decode_error", "transport_error"} {
AMRequestDuration.WithLabelValues(outcome).Observe(0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid seeding AM latency histogram with fake samples

Calling Observe(0) during init adds one synthetic request to each outcome bucket before any real Alertmanager call happens, which permanently inflates _count and skews rate/error calculations derived from this histogram. If the goal is to pre-create label series, instantiate the child metric without observing a sample.

Useful? React with 👍 / 👎.

Comment on lines 56 to 57
req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, url, nil)
if err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Classify request-construction failures as non-success

When http.NewRequestWithContext fails (for example due to a malformed BaseURL), the function returns early while outcome is still "success", so the latency metric records a failed call as a success. That underreports failures exactly in misconfiguration cases where this metric is most useful for diagnosis.

Useful? React with 👍 / 👎.

@mashkovd
mashkovd merged commit df7cb22 into main May 7, 2026
1 check passed
@mashkovd
mashkovd deleted the feat/agents-incident-auto-cleanup-phase4b-metrics-full branch May 7, 2026 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant