Skip to content

refactor(patterns): 100% emit consolidation, drop wrappers#523

Merged
trilamsr merged 1 commit into
mainfrom
refactor/emit-100-consolidation
Jun 4, 2026
Merged

refactor(patterns): 100% emit consolidation, drop wrappers#523
trilamsr merged 1 commit into
mainfrom
refactor/emit-100-consolidation

Conversation

@trilamsr

@trilamsr trilamsr commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes the loop on PR #521 (Lane N). #521's body claimed "full appendVerdict consolidation" but left two typed wrappers in place — emitPodEvicted and emitIBLinkFlap — because the reviewer correctly flagged the gating + telemetry-label divergence as a behavior risk, and the original carve-out comment justified them with "Go generics cannot abstract over a struct field." That's an 85% consolidation, not 100%.

This PR removes both wrappers and routes their callsites through the generic emitAll[V verdictAttrer] directly.

Root cause of the wrapper residue

The wrappers existed because PodEvictedVerdict.Confidence and IBLinkFlapVerdict.Confidence are exported struct fields, and adding a Confidence() method to the verdictAttrer interface would have shadowed the field name on the same type. The fix is one level up: VerdictCommon (the existing shared-boilerplate carrier returned by every verdict's Common() method) was the right place for Confidence all along. It's already the interface seam emitAll reads through — moving Confidence there lets the generic path gate partial verdicts and stamp the real confidence label on IncVerdict without touching field names on individual verdict types.

What changes

  • module/pkg/patterns/verdict.go — add Confidence Confidence to VerdictCommon.
  • 7 verdict Common() methods — populate Confidence: v.Confidence (PodEvicted, IBLinkFlap, CUDAOOM, Checkpointer, DataLoader, NCCLBootstrap, SilentDataCorruption). The other 5 detectors (NCCLHang, XidCorrelation, HBMECC, ThermalThrottle, PCIeAER) have no Confidence field, so their Common() returns the zero value ("") — byte-identical to today's IncVerdict(patternID, "") tick.
  • patterndetector.goemitAll now reads c := v.Common(), gates c.Confidence == ConfidencePartial && !emitPartial, and ticks IncVerdict(c.PatternID, string(c.Confidence)). Both wrapper funcs deleted; both callsites call emitAll directly.

Behavior-identical proof

Detector Path before Path after Telemetry label before Telemetry label after
PodEvicted emitPodEvicted emitAll string(v.Confidence) string(c.Confidence) — same
IBLinkFlap emitIBLinkFlap emitAll string(v.Confidence) string(c.Confidence) — same
NCCLHang / XidCorrelation / HBMECC / ThermalThrottle / PCIeAER emitAll emitAll "" "" (zero-value Confidence) — same
CUDAOOM / Checkpointer / SDC / NCCLBootstrap / DataLoader own run* helper (unchanged) own run* helper (unchanged) n/a n/a

Partial-gating preserved: TestPatternDetector_IBLinkFlapWiringPartialSuppressed, TestPatternDetector_PodEvictedWiringPartial*, TestPatternDetector_CUDAOOMWiringPartialVerdictWhenFBMissing all pass.

Pinned by TestNoopFallback_IncVerdict_TableDriven (15 sub-cases covering every pattern × confidence combination) — green.

Success criteria

$ grep -nE '^func emit[A-Z]' module/processor/patterndetectorprocessor/patterndetector.go
285:func emitAll[V verdictAttrer](p *patterndetectorProcessor, ld plog.Logs, verdicts []V) {

Only emitAll remains.

Test plan

  • cd module && go build ./... — clean
  • cd module && go test ./... — 12 packages green
  • make lint — 0 issues
  • grep '^func emit[A-Z]' module/processor/patterndetectorprocessor/patterndetector.go returns only emitAll
  • TestNoopFallback_IncVerdict_TableDriven — 15/15 pass (pins per-pattern confidence labels)
  • TestPatternDetector_IBLinkFlapWiring* (3 cases) — pass
  • TestPatternDetector_PodEvicted* — pass
  • TestPatternDetector_CUDAOOMWiringPartialVerdictWhenFBMissing — pass
Internal refactor only. No wire-format, config, or operator-facing change.
Generic `emitAll` is now the single emit path for all twelve registered
detectors; partial-confidence gating + IncVerdict telemetry labels
unchanged.

Hoists Confidence onto VerdictCommon so the generic emitAll path
handles partial-gating + real-confidence-label IncVerdict uniformly.
Deletes emitPodEvicted + emitIBLinkFlap (the last 2 typed wrappers
left after PR #521's appendVerdict consolidation) and routes both
callsites through emitAll directly.

Behavior-identical: the 5 emitAll-path detectors without a
Confidence field (NCCLHang, XidCorrelation, HBMECC, ThermalThrottle,
PCIeAER) return the zero-value Confidence from Common(), so
IncVerdict still ticks with the legacy empty-string label pinned by
TestNoopFallback_IncVerdict_TableDriven. PodEvicted + IBLinkFlap
now flow through emitAll with their real Confidence label — same
gating + telemetry shape the deleted wrappers provided.

success criteria:
- grep '^func emit[A-Z]' returns only emitAll
- go test ./... green (module)
- make lint clean (0 issues)

Signed-off-by: Tri Lam <tree@lumalabs.ai>
@trilamsr trilamsr enabled auto-merge (squash) June 4, 2026 02:31
@trilamsr trilamsr merged commit 980b0f6 into main Jun 4, 2026
27 checks passed
@trilamsr trilamsr deleted the refactor/emit-100-consolidation branch June 4, 2026 02:41
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