PMM-15271 Keep collecting Enhanced Monitoring when one log stream is missing

[marcuscruz-percona]

PMM-15271 Keep collecting Enhanced Monitoring when one log stream is missing

https://perconadev.atlassian.net/browse/PMM-15271

Problem

After an RDS blue/green switchover, the exporter stopped collecting OS metrics (CPU, memory, disk,
load) for every RDS instance sharing a region and AWS key — not only the instance that switched
over. One monitored instance without an RDSOSMetrics log stream (Enhanced Monitoring off in AWS, or
a freshly promoted instance whose stream does not exist yet) was enough.

The outage was also invisible: the collector re-emitted its last cached sample forever, so graphs
showed flat lines instead of gaps, dashboards looked healthy, and CPU/memory alerts evaluated stale
numbers and could not fire. Recovery required a human to notice and enable Enhanced Monitoring in AWS
(37 minutes in the reported incident).

Root causes

1. Instances with Enhanced Monitoring disabled in AWS were requested anyway. MonitoringInterval
   was read from DescribeDBInstances and used only to pick the scrape interval; nothing filtered on
   it, so an instance that can never have a log stream went into every request.
2. Batch abort. One FilterLogEvents per 100 log streams, and CloudWatch fails the whole
   request with ResourceNotFoundException when any single listed stream is absent. The old code
   logged and breaked, so up to 100 instances got nothing, every scrape, forever.
3. The cache never expired. Collect re-emitted every stored sample on every Prometheus scrape.
4. The request window advanced on failure. nextStartTime moved to time.Now() even when a
   scrape collected nothing, so events that landed late — the blue/green recovery case — were skipped
   permanently.
5. Switchover race. The 5-minute resource-ID refresh repointed the request at the promoted
   instance's new DbiResourceId before CloudWatch had created its stream.

What changed

The fix proper:

Commit	Change
Allow the CloudWatch Logs client to be faked	scraper.svc narrowed to the SDK's own cloudwatchlogs.FilterLogEventsAPIClient; scriptable fake, so everything below is tested without AWS
Skip instances AWS has no Enhanced Monitoring for	ResourceIDResolver returns InstanceState{ResourceID, MonitoringInterval}; the interval is refreshed by the existing 5-minute DescribeDBInstances call, so enabling EM later re-admits an instance and disabling it later stops poisoning the batch — zero extra AWS calls (RC1)
Split the enhanced scrape into batch, page and event steps	Pure extraction: batches / collectBatch / collectPages / handleEvent / eventSink
Keep collecting when a log stream is missing	On ResourceNotFoundException the batch is halved until the missing streams are singled out, then excluded with a 5-minute TTL and staggered re-probes. Only not_found may exclude a stream — throttling and expired credentials never do. Also stops the window advancing on a failed scrape, and clamps it to a 3-minute lookback (RC2, RC4, RC5)
Expire stale enhanced metrics and report collection health	Samples expire after max(3 × interval, 3m); the cache is keyed by {region, instance} instead of resource ID, so a switchover overwrites instead of duplicating a label set; three self-metrics (RC3)
Report collection health for every monitored instance	An instance that never delivered a sample reports up 0 instead of having no series at all, and a long outage keeps reporting up 0 rather than resolving its own alert when the entry is pruned
Ignore log events timestamped in the future	CloudWatch accepts event timestamps up to 2 hours ahead. Since expiry and the request window both follow the event timestamp, one such event would freeze the cursor and suppress every later sample of that instance
Give each batch its own log stream isolation budget	A batch where every stream is missing no longer spends the recovery budget of the batches behind it

Defects found in the above while reviewing it, fixed in the same PR:

Commit	What it fixes
Build the monitored instance set before starting scrapers	The set was filled inside the loop that starts the scrapers, unlocked, while an earlier session's drain goroutine already read it under the lock — a data race with two or more sessions
Follow the monitoring interval AWS reports while running	The scrape interval and the TTL derived from it were fixed at startup, so a fleet that booted with Enhanced Monitoring off kept scraping every 60s after AWS reported 1s
Re-arm the probe of a log stream that returned no events	An excluded stream that exists again but published nothing kept a probe deadline in the past, stayed due every scrape and held one of the eight probe slots for good — with more such streams than slots, genuinely recovered streams were never probed
Restore an instance whose sample payload was released	Once prune released a payload, a replacement instance publishing timestamps older than the retired one's last event was refused for good
Bound the first enhanced scrape by the scrape interval	Every scrape in the loop gets the interval as its deadline; the first one, run synchronously so the collector has all metric descriptions, did not. Since isolation spends extra requests per batch, a region where no log stream exists could hold NewCollector — and with it the metrics server — for as long as AWS kept answering
Name the log stream bisect helpers for what they do	halves read as a count rather than the divisor it is, and due answered true for a stream that was never excluded — a branch its only caller cannot reach

Raised in review and fixed here:

Commit	What it fixes
Stop excluding a stream CloudWatch answered	Exclusion was lifted only when a stream delivered an event, so a stream that existed again but published nothing inside the request window bought another 5-minute exclusion. The window is only as wide as the fastest instance's reporting interval, so on a fleet mixing intervals a recovered stream usually publishes nothing inside it and healing took hours instead of ~5 minutes. A rejection names no stream, so answering the request is the only positive evidence every stream in it exists — that is now what clears the exclusion, and it retires the probe re-arming the row above added
Spend one probe slot per log stream	Probe accounting walked configured instances, so instances sharing a resource ID each listed their stream and each spent a probe slot; enough duplicates of one missing stream took all eight and a recovered stream was never probed. The duplicate names in LogStreamNames are gone with it
Keep the instance states already resolved	InstanceStates discarded the pages it had read when a later DescribeDBInstances page failed. Since New drops an instance with no resource ID and then deletes a session left without instances, one failed page could stop basic and enhanced collection for a whole AWS key until a restart — a regression against the loop it replaced, which kept the pages that succeeded

Plus no-behaviour-change commits: the clock passed into setMetrics, the write-only sessions field
dropped, the error-counter help text corrected, comments that restated the code removed,
betterTimes renamed to newestEventTimes, WaitGroup.Go in place of the Add/go/Done trio,
tests grouped under their subject, and three commits closing test gaps — the last of which covers errorKind's classification (including the ordering
that keeps a refused credential out of not_found), Stop, a session with no instances, and a
sample whose event timestamp is already older than its TTL.

Behaviour changes reviewers should know about

• Flat lines become gaps. This is the fix, but it will look like a regression on dashboards and to
  anything that relied on the last value always being present. Worth a release note.
• New series. rds_exporter_enhanced_up is 0 for an instance whose Enhanced Monitoring is off
  in AWS, for as long as it is monitored — so up == 0 alerts want to exclude those instances or
  accept a permanent firing.
• Instances configured twice with the same region and name now collapse into one series (last
  writer wins) instead of producing a duplicate label set that promhttp rejects.

New metrics

Metric	Type	Labels
rds_exporter_enhanced_up	gauge 0/1	region, instance
rds_exporter_enhanced_last_event_timestamp_seconds	gauge	region, instance
rds_exporter_enhanced_scrape_errors_total	counter	region, kind

kind is a closed set: context, throttling, auth, not_found, future_event, other.
not_found counts log streams as they are excluded, not once per scrape, so a permanently missing
stream does not inflate it.

Local verification

$ go test -count=1 ./enhanced/... ./sessions/...
118 passed, 1 failed, 1 skipped
  [FAIL] sessions.TestSession — AWS_ACCESS_KEY and AWS_SECRET_KEY environment variables must be set
                                (it talks to live AWS; CI has the credentials, this machine does not)

$ go test -count=1 -cover ./enhanced/...
coverage: 94.5% of statements        # hermetic; sessions needs the credentials above

$ go test -race -count=1 ./...
118 passed, 3 failed, 1 skipped
  the 3 failures are sessions.TestSession, basic.TestCollector and
  basic.TestCollectorDisableBasicMetrics — all pre-existing and all requiring AWS credentials;
  identical on main

$ go vet ./...
(clean)

$ gofmt -l .
(clean)

$ git diff --check main...HEAD
(clean)

$ bin/golangci-lint run ./enhanced/... ./sessions/... .
165 issues (main: 245), 0 of them on a line this branch changed

One commit here is about that test rather than the bug. sessions.TestSession was red on main too,
and for the same reason: it asserted a hardcoded DbiResourceId for
pmm-qa-aurora3-mysql-instance-1, and that QA instance has been recreated since
(db-DFQSTXQUYKPPAFNHGTJ27P5PTU → db-XI52OXEYO3ANQECF54M7WS46EA). main's own last CI run is red
on exactly that test and nothing else. Since AWS mints a new resource ID on every instance recreate,
the test now asserts the ID's shape and compares against what AWS reported — it still checks
everything it was written to check, without depending on a value it does not control. The other three
QA instances got the same treatment, so the next recreate does not turn CI red again.

One commit here is about the linter rather than the bug. This branch adds the first new files to
enhanced and sessions, and reviewdog only annotates changed lines, so four checks the existing
code has always violated surfaced for the first time: depguard with no allow list (it rejects the
AWS SDK; gomodguard, the same policy at module level, was already disabled), testpackage against
in-package tests that cover unexported identifiers, revive's package-comments on any new file
since no package here has one, and exhaustruct against the empty promlog.Config that main.go
itself uses. Configuring those rather than sprinkling 17 //nolint directives is why .golangci.yml
is in the diff.

Test functions in enhanced and sessions: 11 → 49, 118 cases including subtests.
Statement coverage of enhanced: 67.3% → 94.5%, measured without AWS credentials in both cases.

New tests cover: the reported scenario end to end, EM-state filtering and its refresh in both
directions, the scrape interval following AWS while running, missing-stream isolation, TTL re-probing
including a probe that finds the stream still missing and one that finds it silent, staggered probes,
resource-ID change invalidation, batch boundaries at 0/1/99/100/101/150 streams, partial results when
a page or an earlier batch fails, a scrape that runs out of time mid-isolation, throttling and expired
credentials never excluding a stream, context cancellation, window advance / non-advance / lookback
clamp, the future-timestamp boundary, sample expiry, the retention boundary, recovery after a payload
is released, redelivered events not extending expiry, the self-metric label sets, concurrent Collect
and setMetrics under -race, log injection through an AWS-controlled log stream name, and the raw
event message never reaching the log.

Not covered by this PR

• Manual AWS acceptance test (needs an account with two RDS instances and a blue/green deployment):
  EM off on one instance → the others keep reporting and the disabled one is never requested; enabling
  EM without restarting the exporter → self-heal within ~5 minutes; switchover → a gap, not a flat
  line, and no -old1 series left behind; a 1-second interval on a large fleet →
  scrape_errors_total{kind="throttling"} rises and no stream is marked missing.
• Isolation is bounded by the scrape's own deadline (the scrape interval). On a large fleet where many
  streams are missing at once, a scrape can run out of time while bisecting and leaves the batches
  behind it for the next scrape. The request window does not advance, so those events are delayed
  rather than lost, and the exclusions accumulated so far make each round cheaper until it converges.
  Pinned by a test rather than papered over with another timeout constant.
• The request window is global to a session and starts at the oldest of the newest timestamps
  collected, taken over the instances that reported in that scrape. An instance that reported nothing
  in a given scrape therefore does not hold the window back, so an event of its own still inside
  CloudWatch's ingestion lag (10–60 s) can fall before the new start time. Only reachable on a fleet
  mixing Enhanced Monitoring intervals, unchanged from main, and out of scope here — a per-instance
  window would be the fix.
• Two AWS accounts in one region behind role ARNs still collapse into one session, because the session
  key is region + AWS access key and role-based instances have no access key. Tracked separately
  under PMM-13045.
• Collector.Stop() is tested but still unreachable: main registers the collector and blocks in
  ListenAndServe with no shutdown path. Not this bug.

Delivery

VERSION 0.7.4 → 0.7.5. Tag v0.7.5 after merge, then point the sources/rds_exporter submodule
in Percona-Lab/pmm-submodules (v3) at the tag; the pmm docs version table is bumped in a companion
pmm PR.

Backport: the exporter ships from tags with no release branches, so landing this on an earlier PMM 3.x
is a patch tag plus a submodule bump. PMM 2.x is not a cherry-pick — that line predates aws-sdk-go-v2
and ResourceIDResolver.

[it-percona-cla]

All committers have signed the CLA.

[marcuscruz-percona]

@percona/platform-code-review-be, please review the current pull request

[Copilot]

Pull request overview

Improves Enhanced Monitoring resilience when CloudWatch log streams are unavailable.

Changes:

• Filters disabled monitoring and isolates missing streams.
• Expires stale samples and adds health metrics.
• Adds extensive scraper, resolver, and collector tests.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
VERSION	Bumps release to 0.7.5.
sessions/sessions.go	Resolves resource IDs and monitoring intervals.
sessions/sessions_test.go	Removes brittle resource-ID assertions.
sessions/resolver_test.go	Tests paginated state resolution.
enhanced/window_test.go	Tests request-window behavior.
enhanced/streams.go	Tracks and reprobes missing streams.
enhanced/streams_test.go	Tests batching, isolation, and probes.
enhanced/scraper.go	Implements resilient collection flow.
enhanced/scraper_test.go	Tests scraper refresh and collection.
enhanced/fake_logs_test.go	Adds a scriptable CloudWatch fake.
enhanced/events_test.go	Tests event handling and lifecycle.
enhanced/errors.go	Classifies collection errors.
enhanced/errors_test.go	Tests error classification.
enhanced/collector.go	Adds expiry and health metrics.
enhanced/collector_test.go	Tests caching, expiry, and concurrency.
.golangci.yml	Adjusts lint configuration.

---

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

[marcuscruz-percona]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved. I merged origin/main into this branch and fixed the conflict in sessions/sessions_test.go by keeping the dynamic ResourceID expectation. Commit: 15be636.

[marcuscruz-percona]

Review findings and what happened to each

Two passes over main...HEAD — one against the repo's conventions, one against the ticket — plus the two Copilot threads above. Every claim below was checked against the code before acting on it; the ones I did not change are listed with the reason, not omitted.

Fixed

#	Finding	Resolution
1	Self-heal took hours, not minutes. Exclusion was lifted only when a stream delivered an event. The request window is only as wide as the fastest instance's reporting interval, so on a fleet mixing Enhanced Monitoring intervals a recovered stream usually publishes nothing inside it, and each probe bought another 5-minute exclusion.	0715946 — a rejection names no stream, so answering the request is the only positive evidence every stream in it exists. That now clears the exclusion, which also retires the probe re-arming added earlier in the branch: a stream no longer excluded needs no probe slot.
2	Probe slots were counted per instance, not per stream (Copilot). Instances sharing a resource ID each listed their stream and each spent a slot; enough duplicates of one missing stream took all eight and a recovered stream was never probed.	f4f53a9 — enhancedStreams keeps a requested set: one slot, one list entry per stream. The duplicate names it was putting in LogStreamNames are gone with it.
3	A failed DescribeDBInstances page discarded the pages already read (Copilot). New drops an instance with no resource ID and then deletes a session left without instances, so one transient page failure could stop basic and enhanced collection for a whole AWS key until a restart — a regression against the loop it replaced.	63795e2 — InstanceStates returns the states read so far alongside the wrapped error, and New applies them.
4	wg.Add / go func / wg.Done plus a constant that had to agree with the goroutine count.	619308f — WaitGroup.Go, constant deleted.
5	prune's comment claimed it drops "a retired resource ID", but the cache is keyed by region and instance. collectSamples had no subject.	c8d4f32 — both corrected.
6	Two scrape tests built their own background context.	7106caa — t.Context().
7	A comment restating the loop under it in sessions.go.	Removed in 63795e2.

Findings 1 and 2 were caught by tests written to fail first: TestScrapeStopsExcludingSilentStream, TestScrapeSpendsOneProbeSlotPerLogStream, and TestInstanceStates/keeps_the_states_of_the_pages_it_read. Each was confirmed red with its fix reverted and green with it restored.

Checked, deliberately not changed

Finding	Why not
metricsTTL is max(3 × interval, 3m), and since interval ≤ maxInterval = 60s it is always exactly 3 minutes — the formula never binds.	Behaviour is correct and intended; the formula stays right if maxInterval ever rises. Flagging it here rather than pretending it is load-bearing.
prune's unconfigured-delete branch is unreachable by construction — every key written comes from the configured set.	Kept as a guard: without it, a key that should not be there would report health forever. Its comment was the actual defect, fixed in c8d4f32. TestSetMetrics/removes_long_expired_instances does cover the branch; my first pass claimed it did not, and removing the branch proved otherwise by turning that test red.
scrapeOnce's deadline also bounds refreshInstanceStates, so a slow DescribeDBInstances could consume the budget and every batch then fails with a context error.	Impact is one scrape yielding nothing: the refresh runs at most every 5 minutes, the window does not advance on a failed scrape, and the 3-minute sample TTL is far longer than any scrape interval, so no gap appears. Left as is, with the test gap recorded below.
fmt.Sprintf inside a "msg" field instead of structured fields (3 sites).	All three pre-exist on main; this branch only moved them. Out of scope.
Trailing code // comment on struct fields and the monotonic-clock strip.	A preceding-line comment would be noisier for a field annotation, and this is the shape the surrounding file already uses.
Several top-level Test* functions per file rather than one parent with subtests.	Each is its own subject; subtests are used where cases vary within a subject.
Naming and structural smells: ResourceIDResolver now returning InstanceStates, sessions.InstanceState next to enhanced.instanceState, the parallel metrics/messages maps in eventSink, scrape's test-only second return, the repeated fakeLogsClient literals.	Real, and none of them a defect. Renaming an exported type and reshaping the sink would widen a diff that is already large; noting them for a follow-up instead of smuggling them in here.

Test gaps still open

Recorded rather than closed, since none of them hides a defect I could demonstrate:

1. A non-ResourceNotFoundException error inside a bisect half, joined with a sibling's RNFE — does the sibling still get marked, and is errorKind on the join meaningful?
2. refreshInstanceStates consuming the scrape deadline, so batches are skipped on an already-expired context.
3. Fairness across more than maxProbesPerScrape distinct due streams. Duplicates no longer starve other streams (TestScrapeSpendsOneProbeSlotPerLogStream), but slice-order iteration can still favour the same prefix.
4. errIsolationBudget is classified other; the call-count bound is tested, the emitted scrape_errors_total{kind} is not.

Verification after all of the above

$ gofmt -l .                                     (clean)
$ go vet ./...                                   rc=0
$ go test -count=1 ./enhanced/... ./sessions/...  118 passed, 1 failed, 1 skipped
    the failure is sessions.TestSession, which talks to live AWS; CI has the credentials
$ go test -race -count=1 ./enhanced/...          111 passed
$ go test -count=1 -cover ./enhanced/...         coverage: 94.5% of statements (main: 67.3%)
$ bin/golangci-lint run ./enhanced/... ./sessions/... .
    165 issues, 0 of them on a line this round changed

Does the reported bug still get fixed: yes, and the same two layers as before. enhancedStreams never requests an instance AWS reports no Enhanced Monitoring for, refreshed live by updateMonitoringInterval, so the EM-off replica of the ticket never enters a request; if a stream is missing anyway — the switchover race — collectBatch bisects and excludes it while the healthy streams' events still land. The outage renders as a gap because collectSamples drops an expired payload and setMetrics refuses to advance expiry on a re-delivered event. With finding 1 fixed, recovery no longer waits for the recovered stream to publish inside a narrow window.

[marcuscruz-percona]

Test gaps closed

The four gaps I listed as still open in my earlier review summary now have tests, in fa1fa20.

Gap	Test	What it pins
A bisect half rejected for a reason other than a missing stream	TestScrapeKeepsIsolatingWhenAHalfFailsForAnotherReason	Four streams, the last one missing, the first half throttled: only the rejected stream is excluded, the healthy sibling still reports, throttling and not_found are each counted once, the window is held, and the throttled streams are requested again on the next scrape
refreshInstanceStates spending the scrape deadline	TestScrapeOnceSurvivesRefreshSpendingTheDeadline	A DescribeDBInstances that never answers costs one empty scrape: nothing is excluded, the window is held, kind=context is counted once, and the next scrape reports normally because the refresh is no longer due
Probe fairness across more due streams than one scrape may probe	TestScrapeProbesEveryMissingStreamAcrossScrapes	32 missing streams, all due: four scrapes probe all 32, each exactly once. A failed probe waits another TTL, so the first slots cannot keep the streams behind them from ever being retried
The error an exhausted isolation budget reports	TestScrapeReportsIsolationBudgetExhaustion	collectBatch returns errIsolationBudget (errors.Is) and classifies as other; the scrape counts exactly one other for the batch it could not attribute, while still reporting the streams the budget did reach

Each one was confirmed to fail first, by reverting the behaviour it covers and watching it go red:

Test	Reverted	Result
...WhenAHalfFailsForAnotherReason	dropped !isResourceNotFound(err) from isolateHalf	the throttled half is marked missing → FAIL
...RefreshSpendingTheDeadline	removed context.WithTimeout from scrapeOnce	panic: test timed out after 20s
...ProbesEveryMissingStreamAcrossScrapes	mark no longer refreshes probeAfter for a stream it already knows	the same 8 streams are probed every scrape → FAIL
...ReportsIsolationBudgetExhaustion	isolateHalf returns nil instead of the sentinel	the batch left unattributed becomes silent → FAIL

One new helper: blockingStateResolver, a state resolver that waits for ctx.Done() instead of answering.

Verification

gofmt -l enhanced/ sessions/       (no output)
go vet ./...                       rc=0
go test -count=1 ./enhanced/...    ok  117 passed  coverage: 94.7% of statements  (was 94.5%)
bin/golangci-lint run ./enhanced/...  133 issues, 0 of them on a line this commit changed

The two wrapcheck / wsl_v5 findings my own new lines drew are fixed in the same commit, which is why the package total went from 135 to 133.

The remaining local test failures are the pre-existing ones that need live AWS credentials: sessions.TestSession, basic.TestCollector, basic.TestCollectorDisableBasicMetrics.

Not changed

Budget exhaustion still reports scrape_errors_total{kind="other"}, so it is not distinguishable from an unexpected AWS error. Giving it its own kind is a one-const change and the label set stays a closed set — happy to do it if a reviewer would rather see it separated.

The naming and structural smells I listed as deliberately deferred are still deferred.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

enhanced/scraper.go:419

• A successful first page already proves that every requested stream exists, but clearAccepted runs only after the entire paginator finishes. If a recovered stream's probe returns one page and a later page is throttled or otherwise fails, this early return leaves the stream excluded for another five minutes; repeated pagination failures can prevent it from healing indefinitely. Clear the exclusions after the first successful NextPage (the operation is idempotent), while still returning the later page error so the request window does not advance.

		output, err := paginator.NextPage(ctx)
		if err != nil {
			return fmt.Errorf("failed to filter log events: %w", err)
		}

[marcuscruz-percona]

Picking up the suppressed comment from the last review, on enhanced/scraper.go:419.

Fixed in b415805. clearAccepted now runs after each page the paginator gets back, so a later page failing no longer holds a recovered stream out of the next request. It stays after the error check, so a request rejected on its first page still clears nothing — that case is no evidence either way. The window is untouched: advanceStartTime already gates on scrapeErr == nil.

TestScrapeStopsExcludingStreamAnsweredBeforeAPageFailed covers both halves, and the first subtest fails without the change with Should be zero, but was 1. Removing the post-loop call is safe because HasMorePages() is p.firstPage || …, so the body always runs at least once.

One correction to the reasoning, since it matters for anyone reading this later. The comment said the stream would be "excluded for another five minutes" and that repeated pagination failures "can prevent it from healing indefinitely". Neither holds. missing.mark is the only writer of probeAfter, and it is reachable only through markMissing ← isolateMissing ← a ResourceNotFoundException check. A throttled page never re-arms the TTL, so probeAfter stays in the past and the stream is due() again on the next scrape — 2–60 s, not 5 min, and it cannot stall indefinitely. Page 1's events also always published, because scrape returns the sink regardless of the scrape error, so there was never a gap or an up == 0 from this.

So the real cost was narrower than described: the stream kept its exclusion flag for one extra scrape and so kept consuming one of the eight maxProbesPerScrape slots until some request completed cleanly. Worth fixing for that, which is why it landed. The second subtest pins the other half of it — a rejected request must neither clear nor re-arm — so the "due again next scrape" property is now enforced rather than incidental.