[REG-157] Thread ctx through the incident-create path (partial) - #168
Merged
Merged
Conversation
…-157)
Repository plugins from REG-9 were correctly registered but never fed a
context carrying a real span: a live smoke test sending a real
Alertmanager webhook produced zero DB spans, only the rate limiter's
Redis span. Fixes the actual gap.
Repository layer — 8 methods across 3 files now take ctx and call
db.WithContext(ctx): IncidentRepository.{Create,GetByID,Update,
LinkAlert}, AlertRepository.{Create,GetByExternalID,Update},
TimelineRepository.Create. The rest of each interface intentionally
does not — same mixed pattern user_repository.go's Upsert already
established, not a uniform migration of every method.
Service layer, real ctx threaded: AlertService.
{ProcessAlertmanagerPayload,ProcessNormalizedAlerts,createOrUpdateAlert},
IncidentService.{CreateIncidentFromAlert,
CreateIncidentFromAlertWithGrouping,LinkAlertToExistingIncident},
AcknowledgeAlertWithTimeline, coordinator.SeedDemoData,
PostMortemAgent.writeTimelineEntry and TeamsEventHandler.
syncMessageToTimeline (both already had ctx via their callers, just
weren't using it — renamed from _ to ctx).
Handlers updated to pass c.Request.Context() — all 4 alert-intake
entry points, not just Prometheus: prometheus_webhook.go,
webhook_handler.go (the shared Grafana/CloudWatch/generic path),
alerts.go, neuri.go, setup.go.
REG-10's WithoutCancel criterion, done for these paths: both
CreateIncidentFromAlert and CreateIncidentFromAlertWithGrouping now
capture bgCtx := context.WithoutCancel(ctx) before spawning their
Telegram/push goroutines and pass it to recoverAsyncPanic — real
trace-linked spans on panic recovery. Also added panic recovery to a
goroutine in LinkAlertToExistingIncident that had none before.
Explicit placeholder, not silently dropped: every remaining
IncidentService method sharing the 4 changed repo methods
(GetIncident, CreateIncident, UpdateIncident, AcknowledgeIncident,
ResolveIncident, UpdateIncidentStatus, CreateTimelineEntry, the
private createTimelineEntry helper) keeps its existing signature and
passes context.Background() internally. Their callers include
slack_event_handler.go (14 sites, no ctx plumbed through that file
today) and post_mortem_service.go — a structurally different surface
(bot event loop, not request/response) that deserves its own scoped
pass rather than riding along here. Full scope and rationale recorded
in REG-157.
Testing: internal/repository/ctx_propagation_test.go proves real span
correlation (not mocked) for all three repos' Create, using the actual
gorm tracing plugin against an in-memory sqlite DB with an injected
TracerProvider. Live-verified against a real Jaeger collector: the
same webhook that produced 1 span before this change now produces 5 —
incidents Create, two alerts spans (dedup check + Create), the Redis
eval, and the root HTTP span — all under one trace ID. db.query.text
confirmed empty (redacted by default, per REG-9).
Also fixed: a mock/stub method touched for this change
(fullIncidentRepoForNeuri, mockIncidentRepoForSetup,
mockAlertRepository, mockIncidentService) updated to the new
signatures; ~25 test call sites across 9 test files updated to pass
context.Background() or a real request context as appropriate.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
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
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.
Partial REG-157 — the incident-create-path slice, per REG-9's own suggested slice-order. Remaining scope (Slack event handler, post-mortem service, the broader IncidentService CRUD surface) documented precisely in REG-157, not silently dropped.
What
Fixes the actual gap REG-9 found: the gorm/redis tracing plugins were registered correctly, but a live smoke test sending a real Alertmanager webhook produced zero DB spans — only the rate limiter's Redis span — because no repository method ever threaded a context carrying a real span into
db.WithContext().Scope
Repository layer — 8 methods across 3 files:
IncidentRepository.{Create,GetByID,Update,LinkAlert},AlertRepository.{Create,GetByExternalID,Update},TimelineRepository.Create. The rest of each interface intentionally doesn't takectx— same mixed patternuser_repository.go'sUpsertalready established.Service layer, real ctx threaded (not a placeholder):
AlertService.{ProcessAlertmanagerPayload,ProcessNormalizedAlerts,createOrUpdateAlert},IncidentService.{CreateIncidentFromAlert,CreateIncidentFromAlertWithGrouping,LinkAlertToExistingIncident},AcknowledgeAlertWithTimeline,coordinator.SeedDemoData, and two functions that already hadctxavailable via their callers but were discarding it (PostMortemAgent.writeTimelineEntry,TeamsEventHandler.syncMessageToTimeline— both had it as_).Handlers, all 4 alert-intake entry points:
prometheus_webhook.go,webhook_handler.go(the shared Grafana/CloudWatch/generic path — same fix covers all three sources with one signature change),alerts.go,neuri.go,setup.go.REG-10's
WithoutCancelcriterion, done for these two functions: both incident-creation paths now capturebgCtx := context.WithoutCancel(ctx)before spawning their Telegram/push goroutines and pass it torecoverAsyncPanic— real trace-linked spans on panic recovery, notcontext.Background().Where the line was drawn, and why
Once the 4 shared repository methods took
ctx, every caller anywhere in the codebase had to compile against the new signature. Tracing the fan-in:GetIncident/CreateIncident/UpdateIncident/CreateTimelineEntryare called from both real Gin handlers andslack_event_handler.go(14 call sites, no ctx plumbed through that file at all) andpost_mortem_service.go.AcknowledgeIncident/ResolveIncident/UpdateIncidentStatusturned out to have zero Gin-handler callers — they're used exclusively by the Slack/Teams bot event handlers.Threading real ctx through those 7 methods would have forced fixing Slack's entire event-handling surface in this same PR — a structurally different problem (bot callback loop, not request/response) that deserves its own scoped design pass. Left them on their existing signatures, passing
context.Background()internally to the newly-ctx-aware repo calls: zero caller changes, compiles clean, and it's an explicit, documented placeholder rather than a silent gap — exactly the same discipline asrecoverAsyncPanic's 16 call sites in REG-10.An incidental fix
LinkAlertToExistingIncident's Slack-notification goroutine had no panic recovery at all before this change — any panic insideBuildAlertLinkedMessageorPostMessagewould have crashed the whole server. AddedrecoverAsyncPanicsince I was already touching this exact function.Testing
internal/repository/ctx_propagation_test.go: real span-correlation proofs (not mocked) for all three repositories'Create, using the actual gorm tracing plugin against an in-memory sqlite DB with an explicitly injectedTracerProvider(never touching global otel state — the REG-7 finding)go test ./... -shuffle=on -count=3clean on every touched packagegolangci-lint run ./...clean (0 issues)eval). After this change: 5 spans under one trace ID —incidentsCreate, twoalertsspans (the dedupGetByExternalIDcheck +Create), the Rediseval, and the root HTTP span. Confirmeddb.query.textis empty (redacted by default, per REG-9's own criterion) even with real traffic flowing through instrumented repositories.