diff --git a/.gitignore b/.gitignore index feacbb1..946d338 100644 --- a/.gitignore +++ b/.gitignore @@ -7,8 +7,11 @@ search/target/ /ingest/ingest /api/api /cli/sentryctl +/alerting/alerting /hack/windows-fixture/windows-fixture /hack/benchmark-fixture/benchmark-fixture +/hack/alert-load-test/alert-load-test +/hack/webhook-sink/webhook-sink # Node / SvelteKit (web/ has its own more detailed .gitignore too) web/node_modules/ diff --git a/CLAUDE.md b/CLAUDE.md index d02d8c9..0d0d712 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,6 +95,42 @@ ClickHouse/Tantivy routing strategy, and `/docs/query-language-reference.md` for the user-facing syntax reference once built. +## What "done" looks like for Phase 3 + +A user can build a multi-panel dashboard from saved Phase 2 queries (at +least a line chart panel and a table panel, working end-to-end against +live data), save an alert rule that fires a Slack webhook when a +condition is met (threshold comparison, or "absence" — the query returned +zero rows in its own time window), and see the delivery attempt logged — +all from the web UI, without touching the API directly. See +`/docs/phase-3-dashboard-design.md` and `/docs/phase-3-alerting-design.md` +for the data models and the alerting evaluator's firing/resolved state +machine, and `/docs/phase-3-runbook.md` for the live-stack verification, +including a load test of the alert evaluator against ~500 concurrent +rules. + +This phase adds PostgreSQL as a new pinned-stack component (see the +dashboard design doc for why ClickHouse can't do this job — dashboards +and alert state need real row-level locking and transactional +read-modify-write, which ClickHouse's MergeTree family doesn't provide), +scoped strictly to control-plane config: dashboards, panels, notification +targets, alert rules, alert state, delivery log. Log data itself stays on +ClickHouse/Tantivy only, unchanged. + +Non-goals for this phase (same discipline as every phase so far): +- No multi-tenancy enforcement and no `enterprise/` module work — single + tenant/org assumed. New tables carry a `tenant_id` column so Phase 4's + retrofit doesn't require a schema migration + backfill, but nothing + reads or enforces it yet. +- No raw-SQL dashboard panels (time-range injection isn't reliable + against arbitrary SQL) — pipe-syntax queries only. +- No per-group/multi-row threshold alerting (e.g. "alert separately per + host") — a threshold rule's query must resolve to a single row. +- No debounce on the way down — a firing alert resolves on the first + false evaluation, no symmetric "stay firing for N more minutes" hold. +- No Kubernetes Operator/Helm deployment work — still docker-compose, + `/deploy` remains stubbed. + ## When in doubt Ask before: changing the pinned stack, adding a new external dependency that pulls in a large transitive tree, or making an architectural decision diff --git a/alerting/Dockerfile b/alerting/Dockerfile new file mode 100644 index 0000000..cc42b53 --- /dev/null +++ b/alerting/Dockerfile @@ -0,0 +1,12 @@ +# alerting never needs /proto (it talks to /api over plain HTTP, no gRPC), +# so unlike api/ingest/search this build context is just alerting/ itself, +# same shape as cli/Dockerfile: +# docker build -f alerting/Dockerfile -t sentry-alerting alerting/ +FROM golang:1.25-alpine AS builder +WORKDIR /src +COPY . . +RUN CGO_ENABLED=0 GOOS=linux go build -o /out/alerting ./cmd/alerting + +FROM gcr.io/distroless/static-debian12 +COPY --from=builder /out/alerting /alerting +ENTRYPOINT ["/alerting"] diff --git a/alerting/README.md b/alerting/README.md new file mode 100644 index 0000000..9dc0549 --- /dev/null +++ b/alerting/README.md @@ -0,0 +1,98 @@ +# alerting + +Alert rule CRUD, the ticker-driven evaluator, and webhook/Slack/PagerDuty +delivery. See `/docs/phase-3-alerting-design.md` for the full design +(data model, the ok/pending/firing state machine, and the four +correctness properties this implementation follows exactly). + +## Running + +```sh +POSTGRES_PASSWORD=sentry-dev-only API_QUERY_URL=http://localhost:8080 go run ./cmd/alerting +``` + +Talks to the same `sentry_metadata` Postgres database as `/api` +(different tables — see `/metadata/README.md`), and to `/api`'s +`POST /query` over plain HTTP for rule evaluation. Never connects to +ClickHouse or Tantivy directly. + +## HTTP API + +``` +POST /rules create a rule +GET /rules list rules (with current state) +GET /rules/{id} get a rule (with current state) +DELETE /rules/{id} +GET /rules/{id}/deliveries delivery log for a rule, most recent first + +POST /targets create a notification target +GET /targets +GET /targets/{id} +DELETE /targets/{id} + +GET /healthz +``` + +A rule's `condition_type` is `"threshold"` (requires `comparator` + +`threshold_value`, and the query must resolve to exactly one row) or +`"absence"` (fires when the query returns zero rows in its own +`earliest=`/`latest=` window — no separate window field). A notification +target's `kind` is `"webhook"`, `"slack"`, or `"pagerduty"` — all three +deliver via the same HTTP POST + retry/backoff mechanism +(`internal/delivery/webhook.go`); slack/pagerduty are payload formatters +only, not separate delivery paths. + +## Environment variables + +| Var | Default | +|---|---| +| `HTTP_LISTEN_ADDR` | `:8081` | +| `POSTGRES_ADDR` | `localhost:5432` | +| `POSTGRES_DATABASE` | `sentry_metadata` | +| `POSTGRES_USERNAME` | `sentry` | +| `POSTGRES_PASSWORD` | (empty — must be set) | +| `API_QUERY_URL` | `http://localhost:8080` | +| `CORS_ALLOWED_ORIGIN` | `*` | +| `EVALUATOR_TICK_SECONDS` | `5` — how often the scheduler checks for due rules | +| `EVALUATOR_CLAIM_BATCH_SIZE` | `1000` — how many due rules one tick can pull off the queue | +| `EVALUATOR_WORKER_POOL_SIZE` | `20` — bounded concurrency for `/query` calls within a claimed batch | +| `EVALUATOR_QUERY_TIMEOUT_SECONDS` | `30` — per-evaluation `POST /query` timeout | + +`EVALUATOR_CLAIM_BATCH_SIZE` and `EVALUATOR_WORKER_POOL_SIZE` are +deliberately separate knobs, not the same number — see +`internal/config/config.go`'s doc comment for the real bug this +separation fixes (found by `hack/alert-load-test`, see +`/docs/phase-3-runbook.md`): with both capped at 20, 500 rules due at +once took 125s to cycle through instead of the configured 60s. + +## Package layout + +``` +cmd/alerting/ wires config, Postgres pool, api client; runs the + HTTP server + evaluator + delivery worker concurrently (errgroup) +internal/httpapi/ REST handlers -- Handler/RegisterRoutes, same shape as api/internal/dashboards +internal/rulestore/ pgx CRUD for alert_rules + alert_state; ClaimDueRules + (fix 1's atomic claim) and ApplyTransition (fix 2's transactional outbox) +internal/notifystore/ pgx CRUD for notification_targets +internal/queryclient/ thin HTTP client to api's POST /query -- no querylang import here +internal/evaluator/ the ticker + worker pool; transitions.go is the pure, + exhaustively-tested ok/pending/firing state machine; + condition.go implements fixes 3/4 (errors never + coerced to "condition false"; threshold zero-rows + is an error, not a 0) +internal/delivery/ webhook.go is the claim-and-send worker (all three + kinds go through it); slack.go/pagerduty.go are + payload formatters only +``` + +## Building & testing + +```sh +go build ./... +go vet ./... +go test ./... +``` + +```sh +docker build -f Dockerfile -t sentry-alerting . # context is alerting/, not the repo root -- no /proto needed +``` diff --git a/alerting/cmd/alerting/main.go b/alerting/cmd/alerting/main.go new file mode 100644 index 0000000..7cefcd2 --- /dev/null +++ b/alerting/cmd/alerting/main.go @@ -0,0 +1,141 @@ +// Command alerting is Sentry's alert rule evaluator and delivery +// service: rule/target CRUD, the ticker-driven ok/pending/firing +// evaluator, and the webhook/Slack/PagerDuty delivery worker. See +// /docs/phase-3-alerting-design.md. Never talks to ClickHouse/Tantivy +// directly -- rule queries run through /api's POST /query +// (internal/queryclient), same precedent sentryctl query and the web +// UI's dashboard panels already set. +package main + +import ( + "context" + "fmt" + "log/slog" + "net/http" + "os" + "os/signal" + "strings" + "syscall" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + "golang.org/x/sync/errgroup" + + "github.com/sentry/sentry/alerting/internal/config" + "github.com/sentry/sentry/alerting/internal/delivery" + "github.com/sentry/sentry/alerting/internal/evaluator" + "github.com/sentry/sentry/alerting/internal/httpapi" + "github.com/sentry/sentry/alerting/internal/httpserver" + "github.com/sentry/sentry/alerting/internal/notifystore" + "github.com/sentry/sentry/alerting/internal/queryclient" + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +func main() { + logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + + cfg, err := config.Load() + if err != nil { + logger.Error("loading config", "error", err) + os.Exit(1) + } + + // -healthcheck: self-check mode for Docker's HEALTHCHECK, mirrors + // api/cmd/api/main.go's runHealthcheck -- this image is distroless too + // (no shell, no wget), so the compose healthcheck execs this binary + // against itself. + if len(os.Args) > 1 && os.Args[1] == "-healthcheck" { + os.Exit(runHealthcheck(cfg.HTTPListenAddr)) + } + + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + pgDSN := fmt.Sprintf("postgres://%s:%s@%s/%s", cfg.Postgres.Username, cfg.Postgres.Password, cfg.Postgres.Addr, cfg.Postgres.Database) + pgPool, err := pgxpool.New(ctx, pgDSN) + if err != nil { + logger.Error("opening postgres pool", "error", err) + os.Exit(1) + } + defer pgPool.Close() + + if err := pgPool.Ping(ctx); err != nil { + logger.Error("pinging postgres", "error", err) + os.Exit(1) + } + + rules := rulestore.NewStore(pgPool) + targets := notifystore.NewStore(pgPool) + qc := queryclient.New(cfg.APIQueryURL) + + handler := httpapi.NewHandler(logger, rules, targets, rules) + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + srv := &http.Server{ + Addr: cfg.HTTPListenAddr, + Handler: httpserver.WithCORS(mux, cfg.CORSAllowedOrigin), + } + + eval := evaluator.New(rules, targets, qc, cfg.Evaluator.QueryTimeout, cfg.Evaluator.ClaimBatchSize, cfg.Evaluator.WorkerPoolSize, logger) + deliveryWorker := delivery.NewWorker(pgPool, targets, logger) + + g, ctx := errgroup.WithContext(ctx) + + g.Go(func() error { + logger.Info("alerting listening", "addr", cfg.HTTPListenAddr) + errCh := make(chan error, 1) + go func() { errCh <- srv.ListenAndServe() }() + select { + case <-ctx.Done(): + shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := srv.Shutdown(shutdownCtx); err != nil { + return fmt.Errorf("graceful shutdown: %w", err) + } + return nil + case err := <-errCh: + if err != nil && err != http.ErrServerClosed { + return fmt.Errorf("http server exited: %w", err) + } + return nil + } + }) + + g.Go(func() error { + logger.Info("evaluator started", "tick_interval", cfg.Evaluator.TickInterval, "worker_pool_size", cfg.Evaluator.WorkerPoolSize) + if err := eval.Run(ctx, cfg.Evaluator.TickInterval); err != nil && ctx.Err() == nil { + return fmt.Errorf("evaluator exited: %w", err) + } + return nil + }) + + g.Go(func() error { + logger.Info("delivery worker started") + if err := deliveryWorker.Run(ctx, cfg.Evaluator.TickInterval); err != nil && ctx.Err() == nil { + return fmt.Errorf("delivery worker exited: %w", err) + } + return nil + }) + + if err := g.Wait(); err != nil { + logger.Error("alerting exited with error", "error", err) + os.Exit(1) + } +} + +func runHealthcheck(listenAddr string) int { + addr := listenAddr + if strings.HasPrefix(addr, ":") { + addr = "localhost" + addr + } + client := http.Client{Timeout: 3 * time.Second} + resp, err := client.Get("http://" + addr + "/healthz") + if err != nil { + return 1 + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return 1 + } + return 0 +} diff --git a/alerting/go.mod b/alerting/go.mod new file mode 100644 index 0000000..082b3ae --- /dev/null +++ b/alerting/go.mod @@ -0,0 +1,16 @@ +module github.com/sentry/sentry/alerting + +go 1.25.0 + +require ( + github.com/google/uuid v1.6.0 + github.com/jackc/pgx/v5 v5.10.0 + golang.org/x/sync v0.22.0 +) + +require ( + github.com/jackc/pgpassfile v1.0.0 // indirect + github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect + github.com/jackc/puddle/v2 v2.2.2 // indirect + golang.org/x/text v0.29.0 // indirect +) diff --git a/alerting/go.sum b/alerting/go.sum new file mode 100644 index 0000000..00ecaf7 --- /dev/null +++ b/alerting/go.sum @@ -0,0 +1,28 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= +github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= +github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= +github.com/jackc/pgx/v5 v5.10.0 h1:VhSvgU2jSli8o3AqIEOTJr7rZwAEUVo4E4XhR94Zfr0= +github.com/jackc/pgx/v5 v5.10.0/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= +github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= +github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/text v0.29.0 h1:1neNs90w9YzJ9BocxfsQNHKuAT4pkghyXc4nhZ6sJvk= +golang.org/x/text v0.29.0/go.mod h1:7MhJOA9CD2qZyOKYazxdYMF85OwPdEr9jTtBpO7ydH4= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/alerting/internal/config/config.go b/alerting/internal/config/config.go new file mode 100644 index 0000000..802e8b2 --- /dev/null +++ b/alerting/internal/config/config.go @@ -0,0 +1,96 @@ +// Package config loads alerting's configuration from environment +// variables, same convention as /api and /ingest: no config file format. +package config + +import ( + "fmt" + "os" + "strconv" + "time" +) + +type Config struct { + HTTPListenAddr string + Postgres PostgresConfig + APIQueryURL string // base URL of /api, e.g. http://api:8080 -- alerting never talks to ClickHouse/Tantivy directly + CORSAllowedOrigin string + Evaluator EvaluatorConfig +} + +type PostgresConfig struct { + Addr string + Database string + Username string + Password string +} + +type EvaluatorConfig struct { + TickInterval time.Duration // how often the scheduler checks for due rules + // ClaimBatchSize and WorkerPoolSize are deliberately separate knobs, + // not the same number: ClaimBatchSize bounds how many due rules one + // tick pulls off the queue (needs to be large enough to drain a + // backlog when many rules share a due time, e.g. right after bulk + // creation), while WorkerPoolSize bounds concurrent /query calls + // within that batch. Found by actually running hack/alert-load-test + // with 500 rules and both numbers defaulted to the same value + // (20): the evaluator took 125s to cycle through 500 due rules + // instead of the configured 60s eval_interval_seconds, because each + // 5s tick could only claim 20 rules regardless of how many more were + // already due -- see /docs/phase-3-runbook.md's load-test section. + ClaimBatchSize int + WorkerPoolSize int + QueryTimeout time.Duration // per-evaluation POST /query timeout +} + +func Load() (Config, error) { + cfg := Config{ + HTTPListenAddr: getenv("HTTP_LISTEN_ADDR", ":8081"), + Postgres: PostgresConfig{ + Addr: getenv("POSTGRES_ADDR", "localhost:5432"), + Database: getenv("POSTGRES_DATABASE", "sentry_metadata"), + Username: getenv("POSTGRES_USERNAME", "sentry"), + Password: getenv("POSTGRES_PASSWORD", ""), + }, + APIQueryURL: getenv("API_QUERY_URL", "http://localhost:8080"), + // Same "no auth yet" tradeoff as api's CORSAllowedOrigin default -- + // see api/internal/config/config.go's comment, same reasoning here. + CORSAllowedOrigin: getenv("CORS_ALLOWED_ORIGIN", "*"), + } + + tickSec, err := strconv.Atoi(getenv("EVALUATOR_TICK_SECONDS", "5")) + if err != nil { + return Config{}, fmt.Errorf("EVALUATOR_TICK_SECONDS: %w", err) + } + cfg.Evaluator.TickInterval = time.Duration(tickSec) * time.Second + + poolSize, err := strconv.Atoi(getenv("EVALUATOR_WORKER_POOL_SIZE", "20")) + if err != nil { + return Config{}, fmt.Errorf("EVALUATOR_WORKER_POOL_SIZE: %w", err) + } + cfg.Evaluator.WorkerPoolSize = poolSize + + // Default well above WorkerPoolSize: this is "how many due rules can + // one tick pull off the queue," not a concurrency limit -- the + // worker pool below still bounds actual concurrent /query calls + // regardless of how large a batch gets claimed. + claimBatchSize, err := strconv.Atoi(getenv("EVALUATOR_CLAIM_BATCH_SIZE", "1000")) + if err != nil { + return Config{}, fmt.Errorf("EVALUATOR_CLAIM_BATCH_SIZE: %w", err) + } + cfg.Evaluator.ClaimBatchSize = claimBatchSize + + queryTimeoutSec, err := strconv.Atoi(getenv("EVALUATOR_QUERY_TIMEOUT_SECONDS", "30")) + if err != nil { + return Config{}, fmt.Errorf("EVALUATOR_QUERY_TIMEOUT_SECONDS: %w", err) + } + cfg.Evaluator.QueryTimeout = time.Duration(queryTimeoutSec) * time.Second + + return cfg, nil +} + +func getenv(key, fallback string) string { + if v := os.Getenv(key); v != "" { + return v + } + return fallback +} diff --git a/alerting/internal/delivery/delivery_test.go b/alerting/internal/delivery/delivery_test.go new file mode 100644 index 0000000..726e05f --- /dev/null +++ b/alerting/internal/delivery/delivery_test.go @@ -0,0 +1,88 @@ +package delivery + +import ( + "encoding/json" + "testing" + "time" + + "github.com/sentry/sentry/alerting/internal/notifystore" +) + +func TestBuildPayloadGenericDefaultShape(t *testing.T) { + target := notifystore.Target{Kind: notifystore.KindWebhook} + value := 142.0 + payload, err := BuildPayload(target, Event{ + RuleID: "r1", RuleName: "High error rate", EventType: "firing", + Value: &value, Timestamp: time.Date(2026, 8, 14, 0, 0, 0, 0, time.UTC), + }) + if err != nil { + t.Fatalf("BuildPayload: %v", err) + } + var got defaultGenericPayload + if err := json.Unmarshal(payload, &got); err != nil { + t.Fatalf("unmarshaling payload: %v", err) + } + if got.RuleName != "High error rate" || got.EventType != "firing" || got.Value == nil || *got.Value != 142.0 { + t.Fatalf("unexpected payload: %+v", got) + } +} + +func TestBuildPayloadGenericUsesTemplate(t *testing.T) { + tmpl := `{"custom": "{{.RuleName}} is {{.EventType}}"}` + target := notifystore.Target{Kind: notifystore.KindWebhook, PayloadTemplate: &tmpl} + payload, err := BuildPayload(target, Event{RuleName: "disk full", EventType: "firing"}) + if err != nil { + t.Fatalf("BuildPayload: %v", err) + } + var got map[string]string + if err := json.Unmarshal(payload, &got); err != nil { + t.Fatalf("unmarshaling templated payload: %v", err) + } + if got["custom"] != "disk full is firing" { + t.Fatalf("got %q", got["custom"]) + } +} + +func TestBuildPayloadSlackShape(t *testing.T) { + target := notifystore.Target{Kind: notifystore.KindSlack} + payload, err := BuildPayload(target, Event{RuleName: "High error rate", EventType: "resolved"}) + if err != nil { + t.Fatalf("BuildPayload: %v", err) + } + var got slackPayload + if err := json.Unmarshal(payload, &got); err != nil { + t.Fatalf("unmarshaling slack payload: %v", err) + } + if got.Text == "" { + t.Fatalf("expected non-empty Slack text") + } +} + +func TestBuildPayloadPagerDutyMapsEventActionAndRoutingKey(t *testing.T) { + secret := "R0UT1NGKEY" + target := notifystore.Target{Kind: notifystore.KindPagerDuty, Secret: &secret} + + firing, err := BuildPayload(target, Event{RuleID: "r1", RuleName: "disk full", EventType: "firing"}) + if err != nil { + t.Fatalf("BuildPayload firing: %v", err) + } + var gotFiring pagerDutyPayload + if err := json.Unmarshal(firing, &gotFiring); err != nil { + t.Fatalf("unmarshaling: %v", err) + } + if gotFiring.EventAction != "trigger" || gotFiring.RoutingKey != secret || gotFiring.DedupKey != "r1" { + t.Fatalf("unexpected firing payload: %+v", gotFiring) + } + + resolved, err := BuildPayload(target, Event{RuleID: "r1", RuleName: "disk full", EventType: "resolved"}) + if err != nil { + t.Fatalf("BuildPayload resolved: %v", err) + } + var gotResolved pagerDutyPayload + if err := json.Unmarshal(resolved, &gotResolved); err != nil { + t.Fatalf("unmarshaling: %v", err) + } + if gotResolved.EventAction != "resolve" { + t.Fatalf("expected event_action=resolve, got %q", gotResolved.EventAction) + } +} diff --git a/alerting/internal/delivery/pagerduty.go b/alerting/internal/delivery/pagerduty.go new file mode 100644 index 0000000..7307eb8 --- /dev/null +++ b/alerting/internal/delivery/pagerduty.go @@ -0,0 +1,57 @@ +package delivery + +import ( + "encoding/json" + "fmt" + + "github.com/sentry/sentry/alerting/internal/notifystore" +) + +// pagerDutyPayload is PagerDuty's Events API v2 shape. routing_key comes +// from the target's `secret` field -- PagerDuty calls this an +// "integration key," not a delivery credential in the auth sense, but +// it's stored in the same plaintext `secret` column as any other +// target's credential (see /docs/phase-3-alerting-design.md's "Known +// gaps"). +type pagerDutyPayload struct { + RoutingKey string `json:"routing_key"` + EventAction string `json:"event_action"` // "trigger" | "resolve" + DedupKey string `json:"dedup_key"` // rule_id -- lets PagerDuty correlate trigger/resolve as one incident + Payload pagerDutyEventPayload `json:"payload"` +} + +type pagerDutyEventPayload struct { + Summary string `json:"summary"` + Source string `json:"source"` + Severity string `json:"severity"` +} + +func buildPagerDutyPayload(target notifystore.Target, event Event) ([]byte, error) { + action := "trigger" + severity := "critical" + if event.EventType == "resolved" { + action = "resolve" + severity = "info" + } + + var routingKey string + if target.Secret != nil { + routingKey = *target.Secret + } + + summary := fmt.Sprintf("%s: %s", event.RuleName, event.EventType) + if event.Value != nil && event.ThresholdValue != nil { + summary = fmt.Sprintf("%s (value %.4g %s %.4g)", summary, *event.Value, comparatorSymbol(event.Comparator), *event.ThresholdValue) + } + + return json.Marshal(pagerDutyPayload{ + RoutingKey: routingKey, + EventAction: action, + DedupKey: event.RuleID, + Payload: pagerDutyEventPayload{ + Summary: summary, + Source: "sentry", + Severity: severity, + }, + }) +} diff --git a/alerting/internal/delivery/slack.go b/alerting/internal/delivery/slack.go new file mode 100644 index 0000000..0fd1af8 --- /dev/null +++ b/alerting/internal/delivery/slack.go @@ -0,0 +1,48 @@ +package delivery + +import ( + "encoding/json" + "fmt" +) + +// slackPayload is Slack's incoming-webhook shape: {"text": "..."}. No +// user template for this kind -- see webhook.go's BuildPayload doc +// comment for why (thin formatter, not a user-configurable path). +type slackPayload struct { + Text string `json:"text"` +} + +func buildSlackPayload(event Event) ([]byte, error) { + var text string + switch event.EventType { + case "firing": + text = fmt.Sprintf(":rotating_light: *%s* is firing", event.RuleName) + if event.Value != nil && event.ThresholdValue != nil { + text += fmt.Sprintf(" (value %.4g %s %.4g)", *event.Value, comparatorSymbol(event.Comparator), *event.ThresholdValue) + } + case "resolved": + text = fmt.Sprintf(":white_check_mark: *%s* resolved", event.RuleName) + default: + text = fmt.Sprintf("%s: %s", event.RuleName, event.EventType) + } + return json.Marshal(slackPayload{Text: text}) +} + +func comparatorSymbol(c string) string { + switch c { + case "gt": + return ">" + case "gte": + return ">=" + case "lt": + return "<" + case "lte": + return "<=" + case "eq": + return "==" + case "ne": + return "!=" + default: + return c + } +} diff --git a/alerting/internal/delivery/webhook.go b/alerting/internal/delivery/webhook.go new file mode 100644 index 0000000..4107da9 --- /dev/null +++ b/alerting/internal/delivery/webhook.go @@ -0,0 +1,229 @@ +// Package delivery sends notifications for firing/resolved alert +// events. All three notification_targets.kind values -- webhook, slack, +// pagerduty -- go through the exact same claim-then-POST-with-backoff +// mechanism in this file; slack.go and pagerduty.go are payload +// *formatters* only, never a separate delivery path, per +// /docs/phase-3-alerting-design.md's "thin wrappers over the generic +// webhook" requirement. +package delivery + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "log/slog" + "net/http" + "text/template" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/sentry/sentry/alerting/internal/notifystore" +) + +// Event is the firing/resolved occurrence a payload is rendered from. +type Event struct { + RuleID string + RuleName string + EventType string // "firing" | "resolved" + ConditionType string + Comparator string + ThresholdValue *float64 + Value *float64 + Timestamp time.Time +} + +// defaultGenericPayload is used when a webhook target has no +// payload_template. +type defaultGenericPayload struct { + RuleID string `json:"rule_id"` + RuleName string `json:"rule_name"` + EventType string `json:"event_type"` + Value *float64 `json:"value,omitempty"` + Timestamp string `json:"timestamp"` +} + +// BuildPayload dispatches by target.Kind to the right formatter. This is +// the only place kind-specific formatting logic lives -- everything +// downstream of this (worker.go's send loop) is kind-agnostic. +func BuildPayload(target notifystore.Target, event Event) ([]byte, error) { + switch target.Kind { + case notifystore.KindSlack: + return buildSlackPayload(event) + case notifystore.KindPagerDuty: + return buildPagerDutyPayload(target, event) + default: // webhook + return buildGenericPayload(target, event) + } +} + +func buildGenericPayload(target notifystore.Target, event Event) ([]byte, error) { + if target.PayloadTemplate == nil || *target.PayloadTemplate == "" { + return json.Marshal(defaultGenericPayload{ + RuleID: event.RuleID, RuleName: event.RuleName, EventType: event.EventType, + Value: event.Value, Timestamp: event.Timestamp.UTC().Format(time.RFC3339), + }) + } + tmpl, err := template.New("payload").Parse(*target.PayloadTemplate) + if err != nil { + return nil, fmt.Errorf("parsing payload_template: %w", err) + } + var buf bytes.Buffer + if err := tmpl.Execute(&buf, event); err != nil { + return nil, fmt.Errorf("rendering payload_template: %w", err) + } + return buf.Bytes(), nil +} + +// Worker claims pending/retrying delivery_log rows and sends them. +// Deliberately separate from the evaluator: "decided to notify" +// (rulestore.ApplyTransition's transactional outbox insert) is +// transactionally certain; "the HTTP call succeeded" is best-effort with +// retries, handled entirely here. +type Worker struct { + pool *pgxpool.Pool + notifications *notifystore.Store + http *http.Client + logger *slog.Logger +} + +func NewWorker(pool *pgxpool.Pool, notifications *notifystore.Store, logger *slog.Logger) *Worker { + return &Worker{pool: pool, notifications: notifications, http: &http.Client{Timeout: 10 * time.Second}, logger: logger} +} + +// Run claims and sends due deliveries every tickInterval until ctx is +// cancelled. +func (w *Worker) Run(ctx context.Context, tickInterval time.Duration) error { + ticker := time.NewTicker(tickInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + w.processDue(ctx) + } + } +} + +type claimedDelivery struct { + id int64 + ruleID string + notificationTargetID string + eventType string + attemptCount int + maxAttempts int + payload []byte +} + +// processDue claims due rows (same SKIP LOCKED pattern as the +// evaluator's rule claim -- see rulestore.ClaimDueRules) and attempts +// delivery for each. One claim batch per tick; a stuck/slow target +// doesn't block others since each send happens independently. +func (w *Worker) processDue(ctx context.Context) { + rows, err := w.pool.Query(ctx, ` + WITH due AS ( + SELECT id FROM delivery_log + WHERE status IN ('pending', 'retrying') AND (next_attempt_at IS NULL OR next_attempt_at <= now()) + ORDER BY created_at + LIMIT 50 + FOR UPDATE SKIP LOCKED + ) + UPDATE delivery_log d + SET last_attempt_at = now() + FROM due + WHERE d.id = due.id + RETURNING d.id, d.rule_id, d.notification_target_id, d.event_type, d.attempt_count, d.max_attempts, d.payload`) + if err != nil { + w.logger.Error("claiming due deliveries", "error", err) + return + } + var claimed []claimedDelivery + for rows.Next() { + var c claimedDelivery + if err := rows.Scan(&c.id, &c.ruleID, &c.notificationTargetID, &c.eventType, &c.attemptCount, &c.maxAttempts, &c.payload); err != nil { + w.logger.Error("scanning claimed delivery", "error", err) + continue + } + claimed = append(claimed, c) + } + rows.Close() + + for _, c := range claimed { + w.attempt(ctx, c) + } +} + +func (w *Worker) attempt(ctx context.Context, c claimedDelivery) { + target, err := w.notifications.Get(ctx, c.notificationTargetID) + if err != nil { + w.fail(ctx, c, 0, fmt.Sprintf("looking up notification target: %v", err)) + return + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, target.WebhookURL, bytes.NewReader(c.payload)) + if err != nil { + w.fail(ctx, c, 0, fmt.Sprintf("building request: %v", err)) + return + } + req.Header.Set("Content-Type", "application/json") + + resp, err := w.http.Do(req) + if err != nil { + w.fail(ctx, c, 0, err.Error()) + return + } + defer resp.Body.Close() + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + w.markSent(ctx, c.id, resp.StatusCode) + return + } + w.fail(ctx, c, resp.StatusCode, fmt.Sprintf("non-2xx response: %d", resp.StatusCode)) +} + +func (w *Worker) markSent(ctx context.Context, id int64, statusCode int) { + _, err := w.pool.Exec(ctx, ` + UPDATE delivery_log SET status = 'sent', attempt_count = attempt_count + 1, response_status = $1 WHERE id = $2`, + statusCode, id) + if err != nil { + w.logger.Error("marking delivery sent", "delivery_id", id, "error", err) + } +} + +// fail records a failed attempt. If attempts remain, it's rescheduled +// with exponential backoff (2^attempt_count seconds, capped at 1 hour); +// otherwise it's marked permanently failed. +func (w *Worker) fail(ctx context.Context, c claimedDelivery, statusCode int, errMsg string) { + nextAttempt := c.attemptCount + 1 + if nextAttempt >= c.maxAttempts { + _, err := w.pool.Exec(ctx, ` + UPDATE delivery_log SET status = 'failed', attempt_count = attempt_count + 1, response_status = $1, last_error = $2 WHERE id = $3`, + nullIfZero(statusCode), errMsg, c.id) + if err != nil { + w.logger.Error("marking delivery failed", "delivery_id", c.id, "error", err) + } + return + } + + backoff := time.Duration(1< time.Hour { + backoff = time.Hour + } + _, err := w.pool.Exec(ctx, ` + UPDATE delivery_log + SET status = 'retrying', attempt_count = attempt_count + 1, response_status = $1, last_error = $2, next_attempt_at = now() + $3 + WHERE id = $4`, + nullIfZero(statusCode), errMsg, backoff, c.id) + if err != nil { + w.logger.Error("scheduling delivery retry", "delivery_id", c.id, "error", err) + } +} + +func nullIfZero(n int) *int { + if n == 0 { + return nil + } + return &n +} diff --git a/alerting/internal/evaluator/condition.go b/alerting/internal/evaluator/condition.go new file mode 100644 index 0000000..3aca893 --- /dev/null +++ b/alerting/internal/evaluator/condition.go @@ -0,0 +1,74 @@ +package evaluator + +import ( + "fmt" + + "github.com/sentry/sentry/alerting/internal/queryclient" + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +// evaluateCondition implements fixes 3 and 4 from +// /docs/phase-3-alerting-design.md: a threshold rule's query must +// resolve to exactly one row, and zero (or more than one) rows is +// returned as an error, never coerced to a value -- "nothing ran" and +// "ran and found nothing" are different failure/success modes, and +// conflating them can hide the more alarming case. This function never +// itself decides "condition false" on an error; it returns an error, and +// the caller (evaluator.go) routes that to rulestore.RecordError, never +// to ComputeTransition. +func evaluateCondition(rule rulestore.Rule, result *queryclient.Result) (conditionTrue bool, value *float64, err error) { + switch rule.ConditionType { + case rulestore.ConditionAbsence: + // The window is whatever earliest=/latest= the rule's own query + // already expresses -- no separate window field, per the design doc. + return len(result.Rows) == 0, nil, nil + + case rulestore.ConditionThreshold: + if len(result.Rows) != 1 { + return false, nil, fmt.Errorf("threshold rule query returned %d rows, want exactly 1", len(result.Rows)) + } + row := result.Rows[0] + if len(row) == 0 { + return false, nil, fmt.Errorf("threshold rule query returned a row with no columns") + } + v, ok := toFloat64(row[0]) + if !ok { + return false, nil, fmt.Errorf("threshold rule's first column value %v is not numeric", row[0]) + } + if rule.Comparator == nil || rule.ThresholdValue == nil { + return false, nil, fmt.Errorf("threshold rule is missing comparator or threshold_value") + } + return compare(v, *rule.Comparator, *rule.ThresholdValue), &v, nil + + default: + return false, nil, fmt.Errorf("unknown condition_type %q", rule.ConditionType) + } +} + +func compare(value float64, comparator rulestore.Comparator, threshold float64) bool { + switch comparator { + case rulestore.Gt: + return value > threshold + case rulestore.Gte: + return value >= threshold + case rulestore.Lt: + return value < threshold + case rulestore.Lte: + return value <= threshold + case rulestore.Eq: + return value == threshold + case rulestore.Ne: + return value != threshold + default: + return false + } +} + +// toFloat64 handles the one shape /query responses actually come back +// as: Go's encoding/json always decodes JSON numbers into interface{} +// as float64, regardless of whether ClickHouse serialized an integer or +// a float -- there's no separate int64 case to handle. +func toFloat64(v any) (float64, bool) { + f, ok := v.(float64) + return f, ok +} diff --git a/alerting/internal/evaluator/condition_test.go b/alerting/internal/evaluator/condition_test.go new file mode 100644 index 0000000..7d69087 --- /dev/null +++ b/alerting/internal/evaluator/condition_test.go @@ -0,0 +1,118 @@ +package evaluator + +import ( + "testing" + + "github.com/sentry/sentry/alerting/internal/queryclient" + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +func thresholdRule(comparator rulestore.Comparator, threshold float64) rulestore.Rule { + return rulestore.Rule{ + ConditionType: rulestore.ConditionThreshold, + Comparator: &comparator, + ThresholdValue: &threshold, + } +} + +func TestEvaluateConditionThresholdTrue(t *testing.T) { + rule := thresholdRule(rulestore.Gt, 100) + result := &queryclient.Result{Columns: []string{"count"}, Rows: [][]any{{150.0}}} + + got, value, err := evaluateCondition(rule, result) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !got { + t.Fatalf("expected condition true for 150 > 100") + } + if value == nil || *value != 150.0 { + t.Fatalf("expected value=150, got %v", value) + } +} + +func TestEvaluateConditionThresholdFalse(t *testing.T) { + rule := thresholdRule(rulestore.Gt, 100) + result := &queryclient.Result{Columns: []string{"count"}, Rows: [][]any{{50.0}}} + + got, _, err := evaluateCondition(rule, result) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got { + t.Fatalf("expected condition false for 50 > 100") + } +} + +// TestEvaluateConditionThresholdZeroRowsIsError pins down fix 4: zero +// rows on a threshold rule must be an error, not silently coerced to 0 +// (which would make `count > 100` falsely report "fine" when actually +// nothing ran). +func TestEvaluateConditionThresholdZeroRowsIsError(t *testing.T) { + rule := thresholdRule(rulestore.Gt, 100) + result := &queryclient.Result{Columns: []string{"count"}, Rows: [][]any{}} + + _, value, err := evaluateCondition(rule, result) + if err == nil { + t.Fatalf("expected an error for zero rows on a threshold rule, got condition evaluated with value=%v", value) + } +} + +func TestEvaluateConditionThresholdMultipleRowsIsError(t *testing.T) { + rule := thresholdRule(rulestore.Gt, 100) + result := &queryclient.Result{Columns: []string{"host", "count"}, Rows: [][]any{{"h1", 50.0}, {"h2", 200.0}}} + + _, _, err := evaluateCondition(rule, result) + if err == nil { + t.Fatalf("expected an error for multiple rows on a threshold rule (no per-group alerting, a named non-goal)") + } +} + +func TestEvaluateConditionThresholdNonNumericIsError(t *testing.T) { + rule := thresholdRule(rulestore.Gt, 100) + result := &queryclient.Result{Columns: []string{"host"}, Rows: [][]any{{"host-01"}}} + + _, _, err := evaluateCondition(rule, result) + if err == nil { + t.Fatalf("expected an error for a non-numeric first column") + } +} + +func TestEvaluateConditionAbsenceTrueWhenZeroRows(t *testing.T) { + rule := rulestore.Rule{ConditionType: rulestore.ConditionAbsence} + result := &queryclient.Result{Columns: []string{"message"}, Rows: [][]any{}} + + got, value, err := evaluateCondition(rule, result) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !got { + t.Fatalf("expected absence condition true for zero rows") + } + if value != nil { + t.Fatalf("expected nil value for an absence rule, got %v", value) + } +} + +func TestEvaluateConditionAbsenceFalseWhenRowsPresent(t *testing.T) { + rule := rulestore.Rule{ConditionType: rulestore.ConditionAbsence} + result := &queryclient.Result{Columns: []string{"message"}, Rows: [][]any{{"something happened"}}} + + got, _, err := evaluateCondition(rule, result) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got { + t.Fatalf("expected absence condition false when rows are present") + } +} + +func TestEvaluateConditionUnknownTypeIsError(t *testing.T) { + rule := rulestore.Rule{ConditionType: "bogus"} + result := &queryclient.Result{Columns: []string{"count"}, Rows: [][]any{{1.0}}} + + _, _, err := evaluateCondition(rule, result) + if err == nil { + t.Fatalf("expected an error for an unknown condition_type") + } +} diff --git a/alerting/internal/evaluator/evaluator.go b/alerting/internal/evaluator/evaluator.go new file mode 100644 index 0000000..df1c23a --- /dev/null +++ b/alerting/internal/evaluator/evaluator.go @@ -0,0 +1,170 @@ +package evaluator + +import ( + "context" + "log/slog" + "sync" + "time" + + "github.com/sentry/sentry/alerting/internal/delivery" + "github.com/sentry/sentry/alerting/internal/notifystore" + "github.com/sentry/sentry/alerting/internal/queryclient" + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +// Evaluator is the ticker-driven scheduler -- a bounded worker pool, not +// a workflow engine, per the design doc's explicit instruction. Each +// tick claims up to claimBatchSize due rules (rulestore.ClaimDueRules, +// fix 1's atomic claim) and evaluates them concurrently up to +// workerPoolSize at a time. These are deliberately different numbers -- +// see config.EvaluatorConfig's doc comment for the real bug this fixes +// (500 rules due at once, both capped at 20, took 125s to cycle through +// instead of the configured 60s). +type Evaluator struct { + rules *rulestore.Store + notifications *notifystore.Store + queryClient *queryclient.Client + queryTimeout time.Duration + claimBatchSize int + workerPoolSize int + logger *slog.Logger +} + +func New(rules *rulestore.Store, notifications *notifystore.Store, queryClient *queryclient.Client, queryTimeout time.Duration, claimBatchSize, workerPoolSize int, logger *slog.Logger) *Evaluator { + return &Evaluator{ + rules: rules, notifications: notifications, queryClient: queryClient, + queryTimeout: queryTimeout, claimBatchSize: claimBatchSize, workerPoolSize: workerPoolSize, logger: logger, + } +} + +// Run ticks every tickInterval until ctx is cancelled. +func (e *Evaluator) Run(ctx context.Context, tickInterval time.Duration) error { + ticker := time.NewTicker(tickInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + e.tick(ctx) + } + } +} + +func (e *Evaluator) tick(ctx context.Context) { + claimed, err := e.rules.ClaimDueRules(ctx, e.claimBatchSize) + if err != nil { + e.logger.Error("claiming due rules", "error", err) + return + } + if len(claimed) == 0 { + return + } + + sem := make(chan struct{}, e.workerPoolSize) + var wg sync.WaitGroup + for _, rule := range claimed { + wg.Add(1) + sem <- struct{}{} + go func(rule rulestore.RuleWithState) { + defer wg.Done() + defer func() { <-sem }() + e.evaluateOne(ctx, rule) + }(rule) + } + wg.Wait() +} + +func (e *Evaluator) evaluateOne(ctx context.Context, rule rulestore.RuleWithState) { + result, err := e.queryClient.Query(ctx, rule.Query, rule.QueryLanguage, e.queryTimeout) + if err != nil { + // A failed /query call is an evaluation error, never + // "condition false" -- fix 3. Recording it here, not routing it + // through ComputeTransition at all, is what makes that guarantee + // hold structurally rather than by convention. + e.recordError(ctx, rule.ID, err.Error()) + return + } + + conditionTrue, value, evalErr := evaluateCondition(rule.Rule, result) + if evalErr != nil { + e.recordError(ctx, rule.ID, evalErr.Error()) + return + } + + var renotify *time.Duration + if rule.RenotifyIntervalMinutes != nil { + d := time.Duration(*rule.RenotifyIntervalMinutes) * time.Minute + renotify = &d + } + + now := time.Now().UTC() + transition := ComputeTransition(TransitionInput{ + CurrentState: rule.State.State, + ConditionTrueSince: rule.State.ConditionTrueSince, + FiredAt: rule.State.FiredAt, + LastNotifiedAt: rule.State.LastNotifiedAt, + ForMinutes: time.Duration(rule.ForMinutes) * time.Minute, + RenotifyInterval: renotify, + Now: now, + ConditionTrue: conditionTrue, + }) + + next := rulestore.AlertState{ + State: transition.NextState, + ConditionTrueSince: transition.NextConditionTrueSince, + FiredAt: transition.NextFiredAt, + LastNotifiedAt: transition.NextLastNotifiedAt, + LastValue: value, + } + + notify := e.buildNotifyEvent(ctx, rule, transition, value, now) + + if err := e.rules.ApplyTransition(ctx, rule.ID, next, notify); err != nil { + e.logger.Error("applying alert state transition", "rule_id", rule.ID, "error", err) + } +} + +// buildNotifyEvent resolves the notification target and renders the +// payload for a firing/resolved transition. A lookup or template +// failure here is logged and treated as "no notification this time" -- +// the state still transitions correctly (that's the more important +// guarantee), it just means a misconfigured target/template silently +// drops one notification rather than blocking the whole evaluation. +func (e *Evaluator) buildNotifyEvent(ctx context.Context, rule rulestore.RuleWithState, transition TransitionResult, value *float64, now time.Time) *rulestore.NotifyEvent { + if transition.Notify == nil { + return nil + } + + target, err := e.notifications.Get(ctx, rule.NotificationTargetID) + if err != nil { + e.logger.Error("looking up notification target", "rule_id", rule.ID, "target_id", rule.NotificationTargetID, "error", err) + return nil + } + + comparator := "" + if rule.Comparator != nil { + comparator = string(*rule.Comparator) + } + payload, err := delivery.BuildPayload(*target, delivery.Event{ + RuleID: rule.ID, RuleName: rule.Name, EventType: *transition.Notify, + ConditionType: string(rule.ConditionType), Comparator: comparator, + ThresholdValue: rule.ThresholdValue, Value: value, Timestamp: now, + }) + if err != nil { + e.logger.Error("building notification payload", "rule_id", rule.ID, "error", err) + return nil + } + + return &rulestore.NotifyEvent{ + NotificationTargetID: rule.NotificationTargetID, + EventType: *transition.Notify, + Payload: payload, + } +} + +func (e *Evaluator) recordError(ctx context.Context, ruleID, msg string) { + if err := e.rules.RecordError(ctx, ruleID, msg); err != nil { + e.logger.Error("recording evaluation error", "rule_id", ruleID, "error", err) + } +} diff --git a/alerting/internal/evaluator/transitions.go b/alerting/internal/evaluator/transitions.go new file mode 100644 index 0000000..93500a6 --- /dev/null +++ b/alerting/internal/evaluator/transitions.go @@ -0,0 +1,121 @@ +// Package evaluator implements the ticker-driven rule scheduler and the +// firing/resolved state machine described in +// /docs/phase-3-alerting-design.md. transitions.go is deliberately pure +// (no DB, no HTTP, no clock reads beyond the `Now` passed in) -- +// evaluator.go is the only caller, and it's the single place worth +// exhaustive table-driven testing given how easy this state machine is +// to get subtly wrong. +package evaluator + +import ( + "time" + + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +// TransitionInput is everything ComputeTransition needs: the rule's +// current persisted state plus this evaluation's outcome. Evaluation +// *errors* never reach this function at all -- per fix 3 in the design +// doc, an error is recorded separately (rulestore.RecordError) and never +// treated as ConditionTrue: false. +type TransitionInput struct { + CurrentState rulestore.State + ConditionTrueSince *time.Time + FiredAt *time.Time + LastNotifiedAt *time.Time + ForMinutes time.Duration + RenotifyInterval *time.Duration // nil = notify once per firing episode, never again + Now time.Time + ConditionTrue bool +} + +// TransitionResult is what to persist (via rulestore.ApplyTransition) +// and, if Notify is non-nil, which event ("firing" or "resolved") to +// enqueue in the same transaction. +type TransitionResult struct { + NextState rulestore.State + NextConditionTrueSince *time.Time + NextFiredAt *time.Time + NextLastNotifiedAt *time.Time + Notify *string +} + +// ComputeTransition implements the ok/pending/firing state machine. +// ConditionTrueSince is always a wall-clock timestamp, never a +// consecutive-evaluation counter -- this is what makes the for_minutes +// debounce survive evaluator restarts correctly (a counter would +// silently lose progress across downtime; wall-clock math resumes +// exactly where it left off). Don't "simplify" this into a counter. +func ComputeTransition(in TransitionInput) TransitionResult { + if in.ConditionTrue { + return computeConditionTrue(in) + } + return computeConditionFalse(in) +} + +func computeConditionTrue(in TransitionInput) TransitionResult { + switch in.CurrentState { + case rulestore.StatePending: + since := in.ConditionTrueSince + if since == nil { + // Defensive: pending with no recorded since is inconsistent + // persisted state (shouldn't happen -- ok->pending always sets + // it) -- treat as if the condition just became true rather than + // dereferencing a nil or panicking. + now := in.Now + since = &now + } + if in.Now.Sub(*since) >= in.ForMinutes { + return fire(*since, in.Now) + } + return TransitionResult{NextState: rulestore.StatePending, NextConditionTrueSince: since} + + case rulestore.StateFiring: + result := TransitionResult{ + NextState: rulestore.StateFiring, + NextConditionTrueSince: in.ConditionTrueSince, + NextFiredAt: in.FiredAt, + NextLastNotifiedAt: in.LastNotifiedAt, + } + if in.RenotifyInterval != nil && in.LastNotifiedAt != nil && in.Now.Sub(*in.LastNotifiedAt) >= *in.RenotifyInterval { + now := in.Now + event := "firing" + result.NextLastNotifiedAt = &now + result.Notify = &event + } + return result + + default: // ok (or any unexpected value -- the DB CHECK constraint keeps this to 3 values) + since := in.Now + if in.ForMinutes <= 0 { + // for_minutes=0 means "fire on first true evaluation" -- + // literally, not after one extra tick spent in `pending`. + return fire(since, in.Now) + } + return TransitionResult{NextState: rulestore.StatePending, NextConditionTrueSince: &since} + } +} + +func computeConditionFalse(in TransitionInput) TransitionResult { + switch in.CurrentState { + case rulestore.StateFiring: + event := "resolved" + return TransitionResult{NextState: rulestore.StateOK, Notify: &event} + default: // ok or pending: a false evaluation while pending is a blip + // inside the debounce window -- deliberately no notification, that's + // the entire point of for_minutes existing. + return TransitionResult{NextState: rulestore.StateOK} + } +} + +func fire(since, now time.Time) TransitionResult { + firedAt := now + event := "firing" + return TransitionResult{ + NextState: rulestore.StateFiring, + NextConditionTrueSince: &since, + NextFiredAt: &firedAt, + NextLastNotifiedAt: &firedAt, + Notify: &event, + } +} diff --git a/alerting/internal/evaluator/transitions_test.go b/alerting/internal/evaluator/transitions_test.go new file mode 100644 index 0000000..ce6965a --- /dev/null +++ b/alerting/internal/evaluator/transitions_test.go @@ -0,0 +1,193 @@ +package evaluator + +import ( + "testing" + "time" + + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +var t0 = time.Date(2026, 8, 14, 0, 0, 0, 0, time.UTC) + +func ptr[T any](v T) *T { return &v } + +func TestComputeTransition(t *testing.T) { + tests := []struct { + name string + in TransitionInput + want TransitionResult + }{ + { + name: "ok, condition true, for_minutes>0 -> pending, no notify", + in: TransitionInput{ + CurrentState: rulestore.StateOK, ForMinutes: 5 * time.Minute, + Now: t0, ConditionTrue: true, + }, + want: TransitionResult{NextState: rulestore.StatePending, NextConditionTrueSince: ptr(t0)}, + }, + { + name: "ok, condition true, for_minutes=0 -> fires immediately, no extra tick in pending", + in: TransitionInput{ + CurrentState: rulestore.StateOK, ForMinutes: 0, + Now: t0, ConditionTrue: true, + }, + want: TransitionResult{ + NextState: rulestore.StateFiring, NextConditionTrueSince: ptr(t0), + NextFiredAt: ptr(t0), NextLastNotifiedAt: ptr(t0), Notify: ptr("firing"), + }, + }, + { + name: "pending, still within for_minutes window -> stays pending, no notify", + in: TransitionInput{ + CurrentState: rulestore.StatePending, ConditionTrueSince: ptr(t0), + ForMinutes: 5 * time.Minute, Now: t0.Add(2 * time.Minute), ConditionTrue: true, + }, + want: TransitionResult{NextState: rulestore.StatePending, NextConditionTrueSince: ptr(t0)}, + }, + { + name: "pending, exactly at for_minutes boundary -> fires (>=, not >)", + in: TransitionInput{ + CurrentState: rulestore.StatePending, ConditionTrueSince: ptr(t0), + ForMinutes: 5 * time.Minute, Now: t0.Add(5 * time.Minute), ConditionTrue: true, + }, + want: TransitionResult{ + NextState: rulestore.StateFiring, NextConditionTrueSince: ptr(t0), + NextFiredAt: ptr(t0.Add(5 * time.Minute)), NextLastNotifiedAt: ptr(t0.Add(5 * time.Minute)), Notify: ptr("firing"), + }, + }, + { + name: "pending, past for_minutes -> fires", + in: TransitionInput{ + CurrentState: rulestore.StatePending, ConditionTrueSince: ptr(t0), + ForMinutes: 5 * time.Minute, Now: t0.Add(10 * time.Minute), ConditionTrue: true, + }, + want: TransitionResult{ + NextState: rulestore.StateFiring, NextConditionTrueSince: ptr(t0), + NextFiredAt: ptr(t0.Add(10 * time.Minute)), NextLastNotifiedAt: ptr(t0.Add(10 * time.Minute)), Notify: ptr("firing"), + }, + }, + { + name: "firing, condition still true, no renotify interval -> stays firing, silent, preserves fired_at/last_notified", + in: TransitionInput{ + CurrentState: rulestore.StateFiring, ConditionTrueSince: ptr(t0), FiredAt: ptr(t0), LastNotifiedAt: ptr(t0), + RenotifyInterval: nil, Now: t0.Add(time.Hour), ConditionTrue: true, + }, + want: TransitionResult{ + NextState: rulestore.StateFiring, NextConditionTrueSince: ptr(t0), + NextFiredAt: ptr(t0), NextLastNotifiedAt: ptr(t0), Notify: nil, + }, + }, + { + name: "firing, condition still true, renotify interval not yet elapsed -> stays firing, silent", + in: TransitionInput{ + CurrentState: rulestore.StateFiring, ConditionTrueSince: ptr(t0), FiredAt: ptr(t0), LastNotifiedAt: ptr(t0), + RenotifyInterval: ptr(4 * time.Hour), Now: t0.Add(time.Hour), ConditionTrue: true, + }, + want: TransitionResult{ + NextState: rulestore.StateFiring, NextConditionTrueSince: ptr(t0), + NextFiredAt: ptr(t0), NextLastNotifiedAt: ptr(t0), Notify: nil, + }, + }, + { + name: "firing, condition still true, renotify interval elapsed -> re-fires, updates last_notified only", + in: TransitionInput{ + CurrentState: rulestore.StateFiring, ConditionTrueSince: ptr(t0), FiredAt: ptr(t0), LastNotifiedAt: ptr(t0), + RenotifyInterval: ptr(4 * time.Hour), Now: t0.Add(5 * time.Hour), ConditionTrue: true, + }, + want: TransitionResult{ + NextState: rulestore.StateFiring, NextConditionTrueSince: ptr(t0), + // fired_at (the episode start) is NOT bumped on a renotify -- only last_notified_at moves. + NextFiredAt: ptr(t0), NextLastNotifiedAt: ptr(t0.Add(5 * time.Hour)), Notify: ptr("firing"), + }, + }, + { + name: "ok, condition false -> stays ok, no-op", + in: TransitionInput{CurrentState: rulestore.StateOK, Now: t0, ConditionTrue: false}, + want: TransitionResult{NextState: rulestore.StateOK}, + }, + { + name: "pending, condition false -> back to ok, NO notification (a blip inside the debounce window)", + in: TransitionInput{ + CurrentState: rulestore.StatePending, ConditionTrueSince: ptr(t0), + Now: t0.Add(time.Minute), ConditionTrue: false, + }, + want: TransitionResult{NextState: rulestore.StateOK}, + }, + { + name: "firing, condition false -> resolves, sends resolved notification", + in: TransitionInput{ + CurrentState: rulestore.StateFiring, ConditionTrueSince: ptr(t0), FiredAt: ptr(t0), LastNotifiedAt: ptr(t0), + Now: t0.Add(time.Hour), ConditionTrue: false, + }, + want: TransitionResult{NextState: rulestore.StateOK, Notify: ptr("resolved")}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ComputeTransition(tt.in) + if got.NextState != tt.want.NextState { + t.Errorf("NextState = %v, want %v", got.NextState, tt.want.NextState) + } + if !timePtrEqual(got.NextConditionTrueSince, tt.want.NextConditionTrueSince) { + t.Errorf("NextConditionTrueSince = %v, want %v", got.NextConditionTrueSince, tt.want.NextConditionTrueSince) + } + if !timePtrEqual(got.NextFiredAt, tt.want.NextFiredAt) { + t.Errorf("NextFiredAt = %v, want %v", got.NextFiredAt, tt.want.NextFiredAt) + } + if !timePtrEqual(got.NextLastNotifiedAt, tt.want.NextLastNotifiedAt) { + t.Errorf("NextLastNotifiedAt = %v, want %v", got.NextLastNotifiedAt, tt.want.NextLastNotifiedAt) + } + if !strPtrEqual(got.Notify, tt.want.Notify) { + t.Errorf("Notify = %v, want %v", strPtrDeref(got.Notify), strPtrDeref(tt.want.Notify)) + } + }) + } +} + +// TestPendingConditionTrueSinceIsWallClockNotCounter pins down the +// specific property the design doc calls out as easy to accidentally +// "simplify" away: debounce progress survives an evaluator gap (e.g. +// downtime) because condition_true_since is a timestamp, not a tick +// count. Two evaluations three hours apart, both condition=true, with a +// for_minutes=5m debounce -- the second evaluation must fire immediately +// because wall-clock time already satisfies the debounce, regardless of +// how many (or how few) evaluations happened in between. +func TestPendingConditionTrueSinceIsWallClockNotCounter(t *testing.T) { + first := ComputeTransition(TransitionInput{ + CurrentState: rulestore.StateOK, ForMinutes: 5 * time.Minute, Now: t0, ConditionTrue: true, + }) + if first.NextState != rulestore.StatePending { + t.Fatalf("first eval: state = %v, want pending", first.NextState) + } + + // Simulate a large gap (evaluator downtime) before the next evaluation. + second := ComputeTransition(TransitionInput{ + CurrentState: rulestore.StatePending, ConditionTrueSince: first.NextConditionTrueSince, + ForMinutes: 5 * time.Minute, Now: t0.Add(3 * time.Hour), ConditionTrue: true, + }) + if second.NextState != rulestore.StateFiring { + t.Fatalf("second eval after gap: state = %v, want firing (wall-clock debounce should already be satisfied)", second.NextState) + } +} + +func timePtrEqual(a, b *time.Time) bool { + if a == nil || b == nil { + return a == b + } + return a.Equal(*b) +} + +func strPtrEqual(a, b *string) bool { + if a == nil || b == nil { + return a == b + } + return *a == *b +} + +func strPtrDeref(s *string) string { + if s == nil { + return "" + } + return *s +} diff --git a/alerting/internal/httpapi/handler.go b/alerting/internal/httpapi/handler.go new file mode 100644 index 0000000..3497c5e --- /dev/null +++ b/alerting/internal/httpapi/handler.go @@ -0,0 +1,261 @@ +// Package httpapi is alerting's REST surface: rule CRUD, notification +// target CRUD, and a read-only delivery log -- mirrors +// api/internal/dashboards' Handler/Store shape (narrow interfaces, +// pgx-backed production implementation, fakes in tests). +package httpapi + +import ( + "context" + "encoding/json" + "errors" + "log/slog" + "net/http" + + "github.com/sentry/sentry/alerting/internal/notifystore" + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +type ruleStore interface { + Create(ctx context.Context, r *rulestore.Rule) error + List(ctx context.Context) ([]rulestore.RuleWithState, error) + Get(ctx context.Context, id string) (*rulestore.RuleWithState, error) + Delete(ctx context.Context, id string) error +} + +type targetStore interface { + Create(ctx context.Context, t *notifystore.Target) error + List(ctx context.Context) ([]notifystore.Target, error) + Get(ctx context.Context, id string) (*notifystore.Target, error) + Delete(ctx context.Context, id string) error +} + +type deliveryReader interface { + ListForRule(ctx context.Context, ruleID string, limit int) ([]rulestore.DeliveryLogEntry, error) +} + +type Handler struct { + logger *slog.Logger + rules ruleStore + targets targetStore + deliveries deliveryReader +} + +func NewHandler(logger *slog.Logger, rules ruleStore, targets targetStore, deliveries deliveryReader) *Handler { + return &Handler{logger: logger, rules: rules, targets: targets, deliveries: deliveries} +} + +func (h *Handler) RegisterRoutes(mux *http.ServeMux) { + mux.HandleFunc("GET /healthz", h.handleHealthz) + + mux.HandleFunc("POST /rules", h.handleCreateRule) + mux.HandleFunc("GET /rules", h.handleListRules) + mux.HandleFunc("GET /rules/{id}", h.handleGetRule) + mux.HandleFunc("DELETE /rules/{id}", h.handleDeleteRule) + mux.HandleFunc("GET /rules/{id}/deliveries", h.handleListDeliveries) + + mux.HandleFunc("POST /targets", h.handleCreateTarget) + mux.HandleFunc("GET /targets", h.handleListTargets) + mux.HandleFunc("GET /targets/{id}", h.handleGetTarget) + mux.HandleFunc("DELETE /targets/{id}", h.handleDeleteTarget) +} + +const maxBodyBytes = 1 << 20 // 1 MiB, same cap as api/internal/queryapi and dashboards + +func (h *Handler) handleHealthz(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) +} + +// createRuleRequest mirrors rulestore.Rule except Enabled is a pointer: +// a plain bool can't distinguish "omitted" from "explicitly false" on +// decode, and Go's zero value for bool is false -- without this, a +// create request that simply doesn't mention "enabled" would silently +// create a rule the evaluator's claim query never picks up (found by +// actually creating a rule through this endpoint and checking the +// response). Omitted means enabled; only an explicit "enabled": false +// creates a disabled rule. +type createRuleRequest struct { + rulestore.Rule + Enabled *bool `json:"enabled"` +} + +func (h *Handler) handleCreateRule(w http.ResponseWriter, r *http.Request) { + var req createRuleRequest + if !decodeJSON(w, r, &req) { + return + } + rule := req.Rule + rule.Enabled = req.Enabled == nil || *req.Enabled + if err := validateRule(&rule); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + if err := h.rules.Create(r.Context(), &rule); err != nil { + h.logger.Error("creating rule", "error", err) + writeError(w, http.StatusInternalServerError, "creating rule failed") + return + } + writeJSON(w, http.StatusCreated, rule) +} + +func (h *Handler) handleListRules(w http.ResponseWriter, r *http.Request) { + rules, err := h.rules.List(r.Context()) + if err != nil { + h.logger.Error("listing rules", "error", err) + writeError(w, http.StatusInternalServerError, "listing rules failed") + return + } + writeJSON(w, http.StatusOK, rules) +} + +func (h *Handler) handleGetRule(w http.ResponseWriter, r *http.Request) { + rule, err := h.rules.Get(r.Context(), r.PathValue("id")) + if err != nil { + h.writeStoreErr(w, err, "fetching rule") + return + } + writeJSON(w, http.StatusOK, rule) +} + +func (h *Handler) handleDeleteRule(w http.ResponseWriter, r *http.Request) { + if err := h.rules.Delete(r.Context(), r.PathValue("id")); err != nil { + h.writeStoreErr(w, err, "deleting rule") + return + } + w.WriteHeader(http.StatusNoContent) +} + +func (h *Handler) handleListDeliveries(w http.ResponseWriter, r *http.Request) { + entries, err := h.deliveries.ListForRule(r.Context(), r.PathValue("id"), 100) + if err != nil { + h.logger.Error("listing deliveries", "rule_id", r.PathValue("id"), "error", err) + writeError(w, http.StatusInternalServerError, "listing deliveries failed") + return + } + writeJSON(w, http.StatusOK, entries) +} + +func (h *Handler) handleCreateTarget(w http.ResponseWriter, r *http.Request) { + var target notifystore.Target + if !decodeJSON(w, r, &target) { + return + } + if target.Name == "" { + writeError(w, http.StatusBadRequest, "name must not be empty") + return + } + if !notifystore.ValidKind(target.Kind) { + writeError(w, http.StatusBadRequest, "kind must be one of webhook, slack, pagerduty") + return + } + if target.WebhookURL == "" { + writeError(w, http.StatusBadRequest, "webhook_url must not be empty") + return + } + if err := h.targets.Create(r.Context(), &target); err != nil { + h.logger.Error("creating notification target", "error", err) + writeError(w, http.StatusInternalServerError, "creating notification target failed") + return + } + writeJSON(w, http.StatusCreated, target) +} + +func (h *Handler) handleListTargets(w http.ResponseWriter, r *http.Request) { + targets, err := h.targets.List(r.Context()) + if err != nil { + h.logger.Error("listing notification targets", "error", err) + writeError(w, http.StatusInternalServerError, "listing notification targets failed") + return + } + writeJSON(w, http.StatusOK, targets) +} + +func (h *Handler) handleGetTarget(w http.ResponseWriter, r *http.Request) { + target, err := h.targets.Get(r.Context(), r.PathValue("id")) + if err != nil { + h.writeStoreErr(w, err, "fetching notification target") + return + } + writeJSON(w, http.StatusOK, target) +} + +func (h *Handler) handleDeleteTarget(w http.ResponseWriter, r *http.Request) { + if err := h.targets.Delete(r.Context(), r.PathValue("id")); err != nil { + h.writeStoreErr(w, err, "deleting notification target") + return + } + w.WriteHeader(http.StatusNoContent) +} + +// validateRule enforces the shape the evaluator assumes: +// condition_type-appropriate fields present, a sane interval floor. +// Mirrors the DB CHECK constraints so bad input is rejected with a +// clear message here rather than surfacing as an opaque constraint +// violation from Postgres. +func validateRule(r *rulestore.Rule) error { + if r.Name == "" { + return errBadRequest("name must not be empty") + } + if r.Query == "" { + return errBadRequest("query must not be empty") + } + if r.NotificationTargetID == "" { + return errBadRequest("notification_target_id must not be empty") + } + if r.EvalIntervalSeconds < 30 { + return errBadRequest("eval_interval_seconds must be at least 30") + } + switch r.ConditionType { + case rulestore.ConditionThreshold: + if r.Comparator == nil || !rulestore.ValidComparator(*r.Comparator) { + return errBadRequest("threshold rules require a valid comparator (gt, gte, lt, lte, eq, ne)") + } + if r.ThresholdValue == nil { + return errBadRequest("threshold rules require threshold_value") + } + case rulestore.ConditionAbsence: + // No comparator/threshold_value needed -- absence is "the query + // returned zero rows in its own earliest=/latest= window." + default: + return errBadRequest("condition_type must be \"threshold\" or \"absence\"") + } + return nil +} + +type badRequestError string + +func (e badRequestError) Error() string { return string(e) } +func errBadRequest(msg string) error { return badRequestError(msg) } + +func (h *Handler) writeStoreErr(w http.ResponseWriter, err error, action string) { + if errors.Is(err, rulestore.ErrNotFound) || errors.Is(err, notifystore.ErrNotFound) { + writeError(w, http.StatusNotFound, "not found") + return + } + h.logger.Error(action, "error", err) + writeError(w, http.StatusInternalServerError, action+" failed") +} + +func decodeJSON(w http.ResponseWriter, r *http.Request, v any) bool { + r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes) + if err := json.NewDecoder(r.Body).Decode(v); err != nil { + writeError(w, http.StatusBadRequest, "invalid JSON body: "+err.Error()) + return false + } + return true +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} + +type errorResponse struct { + Error string `json:"error"` +} + +func writeError(w http.ResponseWriter, status int, msg string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(errorResponse{Error: msg}) +} diff --git a/alerting/internal/httpapi/handler_test.go b/alerting/internal/httpapi/handler_test.go new file mode 100644 index 0000000..74efb23 --- /dev/null +++ b/alerting/internal/httpapi/handler_test.go @@ -0,0 +1,261 @@ +package httpapi + +import ( + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/sentry/sentry/alerting/internal/notifystore" + "github.com/sentry/sentry/alerting/internal/rulestore" +) + +type fakeRuleStore struct { + rules map[string]*rulestore.RuleWithState +} + +func newFakeRuleStore() *fakeRuleStore { + return &fakeRuleStore{rules: map[string]*rulestore.RuleWithState{}} +} + +func (f *fakeRuleStore) Create(_ context.Context, r *rulestore.Rule) error { + r.ID = "rule-1" + f.rules[r.ID] = &rulestore.RuleWithState{Rule: *r, State: rulestore.AlertState{RuleID: r.ID, State: rulestore.StateOK}} + return nil +} + +func (f *fakeRuleStore) List(_ context.Context) ([]rulestore.RuleWithState, error) { + var out []rulestore.RuleWithState + for _, r := range f.rules { + out = append(out, *r) + } + return out, nil +} + +func (f *fakeRuleStore) Get(_ context.Context, id string) (*rulestore.RuleWithState, error) { + r, ok := f.rules[id] + if !ok { + return nil, rulestore.ErrNotFound + } + return r, nil +} + +func (f *fakeRuleStore) Delete(_ context.Context, id string) error { + if _, ok := f.rules[id]; !ok { + return rulestore.ErrNotFound + } + delete(f.rules, id) + return nil +} + +type fakeTargetStore struct { + targets map[string]*notifystore.Target +} + +func newFakeTargetStore() *fakeTargetStore { + return &fakeTargetStore{targets: map[string]*notifystore.Target{}} +} + +func (f *fakeTargetStore) Create(_ context.Context, t *notifystore.Target) error { + t.ID = "target-1" + f.targets[t.ID] = t + return nil +} +func (f *fakeTargetStore) List(_ context.Context) ([]notifystore.Target, error) { + var out []notifystore.Target + for _, t := range f.targets { + out = append(out, *t) + } + return out, nil +} +func (f *fakeTargetStore) Get(_ context.Context, id string) (*notifystore.Target, error) { + t, ok := f.targets[id] + if !ok { + return nil, notifystore.ErrNotFound + } + return t, nil +} +func (f *fakeTargetStore) Delete(_ context.Context, id string) error { + if _, ok := f.targets[id]; !ok { + return notifystore.ErrNotFound + } + delete(f.targets, id) + return nil +} + +type fakeDeliveryReader struct { + entries []rulestore.DeliveryLogEntry +} + +func (f *fakeDeliveryReader) ListForRule(_ context.Context, _ string, _ int) ([]rulestore.DeliveryLogEntry, error) { + return f.entries, nil +} + +func newTestMux(rules ruleStore, targets targetStore, deliveries deliveryReader) *http.ServeMux { + h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), rules, targets, deliveries) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + return mux +} + +func doRequest(t *testing.T, mux *http.ServeMux, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + var r io.Reader + if body != "" { + r = strings.NewReader(body) + } + req := httptest.NewRequest(method, path, r) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + return rec +} + +func TestCreateThresholdRule(t *testing.T) { + targets := newFakeTargetStore() + targets.targets["target-1"] = ¬ifystore.Target{ID: "target-1"} + mux := newTestMux(newFakeRuleStore(), targets, &fakeDeliveryReader{}) + + rec := doRequest(t, mux, http.MethodPost, "/rules", `{ + "name": "High error rate", "query": "service=api | where status>=500 | stats count", + "condition_type": "threshold", "comparator": "gt", "threshold_value": 100, + "eval_interval_seconds": 60, "notification_target_id": "target-1" + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } +} + +// TestCreateRuleDefaultsToEnabledWhenOmitted guards against a real bug +// caught by actually calling this endpoint: a plain `bool` JSON field +// can't distinguish "omitted" from "explicitly false," and Go's zero +// value for bool is false -- without createRuleRequest's *bool handling, +// a create request that simply didn't mention "enabled" silently created +// a rule the evaluator's claim query would never pick up. +func TestCreateRuleDefaultsToEnabledWhenOmitted(t *testing.T) { + rules := newFakeRuleStore() + mux := newTestMux(rules, newFakeTargetStore(), &fakeDeliveryReader{}) + + rec := doRequest(t, mux, http.MethodPost, "/rules", `{ + "name": "no enabled field", "query": "service=api", "condition_type": "absence", + "eval_interval_seconds": 60, "notification_target_id": "target-1" + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } + if !rules.rules["rule-1"].Enabled { + t.Fatalf("expected a rule created without an explicit \"enabled\" field to default to enabled=true") + } +} + +func TestCreateRuleRespectsExplicitDisabled(t *testing.T) { + rules := newFakeRuleStore() + mux := newTestMux(rules, newFakeTargetStore(), &fakeDeliveryReader{}) + + rec := doRequest(t, mux, http.MethodPost, "/rules", `{ + "name": "explicitly disabled", "query": "service=api", "condition_type": "absence", + "eval_interval_seconds": 60, "notification_target_id": "target-1", "enabled": false + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } + if rules.rules["rule-1"].Enabled { + t.Fatalf("expected an explicit \"enabled\": false to be respected") + } +} + +func TestCreateThresholdRuleRejectsMissingComparator(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodPost, "/rules", `{ + "name": "bad rule", "query": "service=api", "condition_type": "threshold", + "eval_interval_seconds": 60, "notification_target_id": "target-1" + }`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCreateAbsenceRuleDoesNotRequireComparator(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodPost, "/rules", `{ + "name": "no heartbeat", "query": "service=payments earliest=-5m", "condition_type": "absence", + "eval_interval_seconds": 60, "notification_target_id": "target-1" + }`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCreateRuleRejectsShortInterval(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodPost, "/rules", `{ + "name": "too fast", "query": "service=api", "condition_type": "absence", + "eval_interval_seconds": 5, "notification_target_id": "target-1" + }`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestGetRuleNotFound(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodGet, "/rules/nope", "") + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", rec.Code) + } +} + +func TestDeleteRule(t *testing.T) { + rules := newFakeRuleStore() + rules.rules["rule-1"] = &rulestore.RuleWithState{Rule: rulestore.Rule{ID: "rule-1"}} + mux := newTestMux(rules, newFakeTargetStore(), &fakeDeliveryReader{}) + + rec := doRequest(t, mux, http.MethodDelete, "/rules/rule-1", "") + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204", rec.Code) + } + if _, ok := rules.rules["rule-1"]; ok { + t.Fatalf("expected rule to be deleted") + } +} + +func TestCreateTargetRejectsInvalidKind(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodPost, "/targets", `{"name": "x", "kind": "carrier-pigeon", "webhook_url": "https://example.com"}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCreateSlackTarget(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodPost, "/targets", `{"name": "oncall", "kind": "slack", "webhook_url": "https://hooks.slack.com/services/x"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestListDeliveriesForRule(t *testing.T) { + deliveries := &fakeDeliveryReader{entries: []rulestore.DeliveryLogEntry{ + {ID: 1, RuleID: "rule-1", EventType: "firing", Status: "sent"}, + }} + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), deliveries) + + rec := doRequest(t, mux, http.MethodGet, "/rules/rule-1/deliveries", "") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), `"status":"sent"`) { + t.Fatalf("expected delivery entry in response, got: %s", rec.Body.String()) + } +} + +func TestHandleHealthz(t *testing.T) { + mux := newTestMux(newFakeRuleStore(), newFakeTargetStore(), &fakeDeliveryReader{}) + rec := doRequest(t, mux, http.MethodGet, "/healthz", "") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } +} diff --git a/alerting/internal/httpserver/cors.go b/alerting/internal/httpserver/cors.go new file mode 100644 index 0000000..cfaef75 --- /dev/null +++ b/alerting/internal/httpserver/cors.go @@ -0,0 +1,24 @@ +// Package httpserver holds cross-handler HTTP concerns for /alerting. +// Deliberately duplicated from api/internal/httpserver rather than +// shared -- see /docs/phase-3-alerting-design.md's component-boundary +// note: no shared Go store/HTTP code between api and alerting, the same +// repo convention that only /proto is shared code. +package httpserver + +import "net/http" + +// WithCORS is deliberately permissive by default, same posture and +// reasoning as api's: no auth yet, the SvelteKit dev server runs on a +// different origin. Tighten alongside adding real auth. +func WithCORS(next http.Handler, allowedOrigin string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Access-Control-Allow-Origin", allowedOrigin) + w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type") + if r.Method == http.MethodOptions { + w.WriteHeader(http.StatusNoContent) + return + } + next.ServeHTTP(w, r) + }) +} diff --git a/alerting/internal/notifystore/store.go b/alerting/internal/notifystore/store.go new file mode 100644 index 0000000..c23e129 --- /dev/null +++ b/alerting/internal/notifystore/store.go @@ -0,0 +1,114 @@ +// Package notifystore is pgx-backed CRUD for notification_targets. +package notifystore + +import ( + "context" + "encoding/json" + "errors" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +var ErrNotFound = errors.New("not found") + +type Kind string + +const ( + KindWebhook Kind = "webhook" + KindSlack Kind = "slack" + KindPagerDuty Kind = "pagerduty" +) + +func ValidKind(k Kind) bool { + switch k { + case KindWebhook, KindSlack, KindPagerDuty: + return true + default: + return false + } +} + +type Target struct { + ID string `json:"id"` + TenantID string `json:"tenant_id"` + Name string `json:"name"` + Kind Kind `json:"kind"` + WebhookURL string `json:"webhook_url"` + PayloadTemplate *string `json:"payload_template,omitempty"` + Headers json.RawMessage `json:"headers,omitempty"` + Secret *string `json:"secret,omitempty"` + CreatedBy string `json:"created_by"` +} + +type Store struct { + pool *pgxpool.Pool +} + +func NewStore(pool *pgxpool.Pool) *Store { + return &Store{pool: pool} +} + +func (s *Store) Create(ctx context.Context, t *Target) error { + t.ID = uuid.NewString() + if t.TenantID == "" { + t.TenantID = "default" + } + if t.CreatedBy == "" { + t.CreatedBy = "anonymous" + } + if len(t.Headers) == 0 { + t.Headers = json.RawMessage(`{}`) + } + _, err := s.pool.Exec(ctx, ` + INSERT INTO notification_targets (id, tenant_id, name, kind, webhook_url, payload_template, headers, secret, created_by) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)`, + t.ID, t.TenantID, t.Name, t.Kind, t.WebhookURL, t.PayloadTemplate, t.Headers, t.Secret, t.CreatedBy) + return err +} + +func (s *Store) List(ctx context.Context) ([]Target, error) { + rows, err := s.pool.Query(ctx, ` + SELECT id, tenant_id, name, kind, webhook_url, payload_template, headers, secret, created_by + FROM notification_targets ORDER BY created_at DESC`) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []Target + for rows.Next() { + var t Target + if err := rows.Scan(&t.ID, &t.TenantID, &t.Name, &t.Kind, &t.WebhookURL, &t.PayloadTemplate, &t.Headers, &t.Secret, &t.CreatedBy); err != nil { + return nil, err + } + out = append(out, t) + } + return out, rows.Err() +} + +func (s *Store) Get(ctx context.Context, id string) (*Target, error) { + var t Target + row := s.pool.QueryRow(ctx, ` + SELECT id, tenant_id, name, kind, webhook_url, payload_template, headers, secret, created_by + FROM notification_targets WHERE id = $1`, id) + if err := row.Scan(&t.ID, &t.TenantID, &t.Name, &t.Kind, &t.WebhookURL, &t.PayloadTemplate, &t.Headers, &t.Secret, &t.CreatedBy); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrNotFound + } + return nil, err + } + return &t, nil +} + +func (s *Store) Delete(ctx context.Context, id string) error { + tag, err := s.pool.Exec(ctx, `DELETE FROM notification_targets WHERE id = $1`, id) + if err != nil { + return err + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} diff --git a/alerting/internal/queryclient/client.go b/alerting/internal/queryclient/client.go new file mode 100644 index 0000000..800b16c --- /dev/null +++ b/alerting/internal/queryclient/client.go @@ -0,0 +1,75 @@ +// Package queryclient is a thin HTTP client to /api's POST /query -- +// alerting never imports querylang or talks to ClickHouse/Tantivy +// directly, same precedent sentryctl query and the web UI's dashboard +// panels already set: one query-execution path, reused everywhere. +package queryclient + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + "time" +) + +type Result struct { + Columns []string `json:"columns"` + Rows [][]any `json:"rows"` +} + +type errorResponse struct { + Error string `json:"error"` +} + +type Client struct { + baseURL string + http *http.Client +} + +func New(baseURL string) *Client { + return &Client{baseURL: baseURL, http: &http.Client{}} +} + +// Query runs query (already time-range-injected by the caller, if +// applicable) against /api's POST /query and returns the result. A +// non-2xx response or a request/decode failure is returned as an error +// -- callers (the evaluator) treat any error here as an evaluation +// error, never as "condition false" (see +// /docs/phase-3-alerting-design.md's fix 3). +func (c *Client) Query(ctx context.Context, query, language string, timeout time.Duration) (*Result, error) { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + body, err := json.Marshal(map[string]string{"query": query, "language": language}) + if err != nil { + return nil, fmt.Errorf("encoding query request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/query", bytes.NewReader(body)) + if err != nil { + return nil, fmt.Errorf("building query request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + + resp, err := c.http.Do(req) + if err != nil { + return nil, fmt.Errorf("calling api /query: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + var errBody errorResponse + _ = json.NewDecoder(resp.Body).Decode(&errBody) + if errBody.Error != "" { + return nil, fmt.Errorf("api /query failed (%d): %s", resp.StatusCode, errBody.Error) + } + return nil, fmt.Errorf("api /query failed with status %d", resp.StatusCode) + } + + var result Result + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, fmt.Errorf("decoding query response: %w", err) + } + return &result, nil +} diff --git a/alerting/internal/rulestore/store.go b/alerting/internal/rulestore/store.go new file mode 100644 index 0000000..825e09a --- /dev/null +++ b/alerting/internal/rulestore/store.go @@ -0,0 +1,357 @@ +// Package rulestore is pgx-backed CRUD for alert_rules/alert_state, plus +// the two operations that make the evaluator's concurrency and delivery +// guarantees hold (see /docs/phase-3-alerting-design.md's fixes 1 and 2): +// ClaimDueRules (atomic claim-before-evaluate) and ApplyTransition (the +// state transition and the delivery_log outbox insert in one DB +// transaction). +package rulestore + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +var ErrNotFound = errors.New("not found") + +type ConditionType string + +const ( + ConditionThreshold ConditionType = "threshold" + ConditionAbsence ConditionType = "absence" +) + +func ValidConditionType(c ConditionType) bool { + return c == ConditionThreshold || c == ConditionAbsence +} + +type Comparator string + +const ( + Gt Comparator = "gt" + Gte Comparator = "gte" + Lt Comparator = "lt" + Lte Comparator = "lte" + Eq Comparator = "eq" + Ne Comparator = "ne" +) + +func ValidComparator(c Comparator) bool { + switch c { + case Gt, Gte, Lt, Lte, Eq, Ne: + return true + default: + return false + } +} + +type State string + +const ( + StateOK State = "ok" + StatePending State = "pending" + StateFiring State = "firing" +) + +type Rule struct { + ID string `json:"id"` + TenantID string `json:"tenant_id"` + Name string `json:"name"` + Description string `json:"description"` + Query string `json:"query"` + QueryLanguage string `json:"query_language"` + ConditionType ConditionType `json:"condition_type"` + Comparator *Comparator `json:"comparator,omitempty"` + ThresholdValue *float64 `json:"threshold_value,omitempty"` + EvalIntervalSeconds int `json:"eval_interval_seconds"` + ForMinutes int `json:"for_minutes"` + RenotifyIntervalMinutes *int `json:"renotify_interval_minutes,omitempty"` + NotificationTargetID string `json:"notification_target_id"` + Enabled bool `json:"enabled"` + CreatedBy string `json:"created_by"` +} + +type AlertState struct { + RuleID string `json:"rule_id"` + State State `json:"state"` + ConditionTrueSince *time.Time `json:"condition_true_since,omitempty"` + FiredAt *time.Time `json:"fired_at,omitempty"` + LastNotifiedAt *time.Time `json:"last_notified_at,omitempty"` + LastEvaluatedAt *time.Time `json:"last_evaluated_at,omitempty"` + LastEvalStatus string `json:"last_eval_status"` + LastError *string `json:"last_error,omitempty"` + LastValue *float64 `json:"last_value,omitempty"` + ConsecutiveErrors int `json:"consecutive_errors"` +} + +// RuleWithState is a rule joined with its current live state, exactly +// what the evaluator needs to run one evaluation and what the read API +// returns. +type RuleWithState struct { + Rule + State AlertState `json:"state"` +} + +type Store struct { + pool *pgxpool.Pool +} + +func NewStore(pool *pgxpool.Pool) *Store { + return &Store{pool: pool} +} + +// Create inserts the rule and its initial alert_state row in one +// transaction. A rule with no alert_state row is silently never picked +// up by ClaimDueRules -- see the design doc's explicit warning about +// this exact failure mode. +func (s *Store) Create(ctx context.Context, r *Rule) error { + r.ID = uuid.NewString() + if r.TenantID == "" { + r.TenantID = "default" + } + if r.CreatedBy == "" { + r.CreatedBy = "anonymous" + } + + tx, err := s.pool.Begin(ctx) + if err != nil { + return err + } + defer tx.Rollback(ctx) + + _, err = tx.Exec(ctx, ` + INSERT INTO alert_rules (id, tenant_id, name, description, query, query_language, condition_type, + comparator, threshold_value, eval_interval_seconds, for_minutes, + renotify_interval_minutes, notification_target_id, enabled, created_by) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15)`, + r.ID, r.TenantID, r.Name, r.Description, r.Query, r.QueryLanguage, r.ConditionType, + r.Comparator, r.ThresholdValue, r.EvalIntervalSeconds, r.ForMinutes, + r.RenotifyIntervalMinutes, r.NotificationTargetID, r.Enabled, r.CreatedBy) + if err != nil { + return fmt.Errorf("inserting rule: %w", err) + } + + _, err = tx.Exec(ctx, ` + INSERT INTO alert_state (rule_id, state, last_eval_status, next_eval_at) + VALUES ($1, 'ok', 'ok', now())`, r.ID) + if err != nil { + return fmt.Errorf("inserting initial alert_state: %w", err) + } + + return tx.Commit(ctx) +} + +func (s *Store) List(ctx context.Context) ([]RuleWithState, error) { + rows, err := s.pool.Query(ctx, ruleWithStateSelect+" ORDER BY r.created_at DESC") + if err != nil { + return nil, err + } + defer rows.Close() + + var out []RuleWithState + for rows.Next() { + rs, err := scanRuleWithState(rows) + if err != nil { + return nil, err + } + out = append(out, rs) + } + return out, rows.Err() +} + +func (s *Store) Get(ctx context.Context, id string) (*RuleWithState, error) { + rows, err := s.pool.Query(ctx, ruleWithStateSelect+" WHERE r.id = $1", id) + if err != nil { + return nil, err + } + defer rows.Close() + + if !rows.Next() { + return nil, ErrNotFound + } + rs, err := scanRuleWithState(rows) + if err != nil { + return nil, err + } + return &rs, nil +} + +func (s *Store) Delete(ctx context.Context, id string) error { + tag, err := s.pool.Exec(ctx, `DELETE FROM alert_rules WHERE id = $1`, id) + if err != nil { + return err + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} + +const ruleWithStateSelect = ` + SELECT r.id, r.tenant_id, r.name, r.description, r.query, r.query_language, r.condition_type, + r.comparator, r.threshold_value, r.eval_interval_seconds, r.for_minutes, + r.renotify_interval_minutes, r.notification_target_id, r.enabled, r.created_by, + s.state, s.condition_true_since, s.fired_at, s.last_notified_at, s.last_evaluated_at, + s.last_eval_status, s.last_error, s.last_value, s.consecutive_errors + FROM alert_rules r JOIN alert_state s ON s.rule_id = r.id` + +func scanRuleWithState(rows pgx.Rows) (RuleWithState, error) { + var rs RuleWithState + err := rows.Scan( + &rs.ID, &rs.TenantID, &rs.Name, &rs.Description, &rs.Query, &rs.QueryLanguage, &rs.ConditionType, + &rs.Comparator, &rs.ThresholdValue, &rs.EvalIntervalSeconds, &rs.ForMinutes, + &rs.RenotifyIntervalMinutes, &rs.NotificationTargetID, &rs.Enabled, &rs.CreatedBy, + &rs.State.State, &rs.State.ConditionTrueSince, &rs.State.FiredAt, &rs.State.LastNotifiedAt, &rs.State.LastEvaluatedAt, + &rs.State.LastEvalStatus, &rs.State.LastError, &rs.State.LastValue, &rs.State.ConsecutiveErrors) + rs.State.RuleID = rs.ID + return rs, err +} + +// ClaimDueRules atomically claims up to batchSize due, enabled rules -- +// see /docs/phase-3-alerting-design.md's fix 1. next_eval_at is bumped +// forward *before* returning, so a second scheduler tick (or a second +// evaluator replica, later) can't re-select the same rule while this +// one is still being evaluated. SKIP LOCKED means a concurrent claim +// query never blocks on rows another claim already has locked -- it +// just moves on to the next candidate. +func (s *Store) ClaimDueRules(ctx context.Context, batchSize int) ([]RuleWithState, error) { + rows, err := s.pool.Query(ctx, ` + WITH due AS ( + SELECT s.rule_id + FROM alert_state s + JOIN alert_rules r ON r.id = s.rule_id + WHERE s.next_eval_at <= now() AND r.enabled + ORDER BY s.next_eval_at + LIMIT $1 + FOR UPDATE OF s SKIP LOCKED + ) + UPDATE alert_state s + SET next_eval_at = now() + make_interval(secs => r.eval_interval_seconds), + claimed_at = now() + FROM due, alert_rules r + WHERE s.rule_id = due.rule_id AND r.id = s.rule_id + RETURNING r.id, r.tenant_id, r.name, r.description, r.query, r.query_language, r.condition_type, + r.comparator, r.threshold_value, r.eval_interval_seconds, r.for_minutes, + r.renotify_interval_minutes, r.notification_target_id, r.enabled, r.created_by, + s.state, s.condition_true_since, s.fired_at, s.last_notified_at, s.last_evaluated_at, + s.last_eval_status, s.last_error, s.last_value, s.consecutive_errors`, + batchSize) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []RuleWithState + for rows.Next() { + rs, err := scanRuleWithState(rows) + if err != nil { + return nil, err + } + out = append(out, rs) + } + return out, rows.Err() +} + +// NotifyEvent, if non-nil, is inserted into delivery_log in the same +// transaction as the state update -- the transactional outbox from fix +// 2. Payload is the already-rendered notification body (rulestore +// doesn't know how to format one; that's internal/delivery's job). +type NotifyEvent struct { + NotificationTargetID string + EventType string // "firing" | "resolved" + Payload []byte // JSON +} + +// ApplyTransition writes a successful evaluation's outcome: the new +// alert_state fields, and -- atomically, in the same transaction -- a +// pending delivery_log row if notify is non-nil. See fix 2: "decided to +// notify" becomes durable exactly once, in lockstep with the state +// change, before any network call to a notification target is +// attempted. +func (s *Store) ApplyTransition(ctx context.Context, ruleID string, next AlertState, notify *NotifyEvent) error { + tx, err := s.pool.Begin(ctx) + if err != nil { + return err + } + defer tx.Rollback(ctx) + + _, err = tx.Exec(ctx, ` + UPDATE alert_state SET + state = $1, condition_true_since = $2, fired_at = $3, last_notified_at = $4, + last_evaluated_at = now(), last_eval_status = 'ok', last_error = NULL, + last_value = $5, consecutive_errors = 0 + WHERE rule_id = $6`, + next.State, next.ConditionTrueSince, next.FiredAt, next.LastNotifiedAt, next.LastValue, ruleID) + if err != nil { + return fmt.Errorf("updating alert_state: %w", err) + } + + if notify != nil { + _, err = tx.Exec(ctx, ` + INSERT INTO delivery_log (rule_id, notification_target_id, event_type, status, next_attempt_at, payload) + VALUES ($1, $2, $3, 'pending', now(), $4)`, + ruleID, notify.NotificationTargetID, notify.EventType, notify.Payload) + if err != nil { + return fmt.Errorf("inserting delivery_log outbox row: %w", err) + } + } + + return tx.Commit(ctx) +} + +// DeliveryLogEntry is the read shape the web UI's delivery log view and +// the httpapi read endpoint use -- deliberately excludes `payload` +// (the rendered notification body isn't needed for the list view, and +// keeping it out of the default read path avoids echoing +// target-specific formatting, e.g. a rendered webhook secret, back over +// HTTP by default). +type DeliveryLogEntry struct { + ID int64 `json:"id"` + RuleID string `json:"rule_id"` + NotificationTargetID string `json:"notification_target_id"` + EventType string `json:"event_type"` + Status string `json:"status"` + AttemptCount int `json:"attempt_count"` + LastError *string `json:"last_error,omitempty"` + ResponseStatus *int `json:"response_status,omitempty"` + CreatedAt time.Time `json:"created_at"` +} + +// ListForRule reads delivery_log's UI-visible role -- "why didn't I get +// paged" -- most recent first. +func (s *Store) ListForRule(ctx context.Context, ruleID string, limit int) ([]DeliveryLogEntry, error) { + rows, err := s.pool.Query(ctx, ` + SELECT id, rule_id, notification_target_id, event_type, status, attempt_count, last_error, response_status, created_at + FROM delivery_log WHERE rule_id = $1 ORDER BY created_at DESC LIMIT $2`, ruleID, limit) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []DeliveryLogEntry + for rows.Next() { + var e DeliveryLogEntry + if err := rows.Scan(&e.ID, &e.RuleID, &e.NotificationTargetID, &e.EventType, &e.Status, &e.AttemptCount, &e.LastError, &e.ResponseStatus, &e.CreatedAt); err != nil { + return nil, err + } + out = append(out, e) + } + return out, rows.Err() +} + +// RecordError updates only the error-tracking fields on alert_state -- +// state itself is left untouched, per fix 3: an evaluation error must +// never transition state (an error is not "condition false"). +func (s *Store) RecordError(ctx context.Context, ruleID, errMsg string) error { + _, err := s.pool.Exec(ctx, ` + UPDATE alert_state SET + last_evaluated_at = now(), last_eval_status = 'error', last_error = $1, + consecutive_errors = consecutive_errors + 1 + WHERE rule_id = $2`, errMsg, ruleID) + return err +} diff --git a/api/cmd/api/main.go b/api/cmd/api/main.go index 7d7ce1a..bc66862 100644 --- a/api/cmd/api/main.go +++ b/api/cmd/api/main.go @@ -7,18 +7,23 @@ package main import ( "context" + "fmt" "log/slog" "net/http" "os" "os/signal" + "strings" "syscall" "time" "github.com/ClickHouse/clickhouse-go/v2" + "github.com/jackc/pgx/v5/pgxpool" "github.com/sentry/sentry/api/internal/config" - "github.com/sentry/sentry/api/internal/querylang/executor" + "github.com/sentry/sentry/api/internal/dashboards" + "github.com/sentry/sentry/api/internal/httpserver" "github.com/sentry/sentry/api/internal/queryapi" + "github.com/sentry/sentry/api/internal/querylang/executor" "github.com/sentry/sentry/api/internal/searchclient" ) @@ -31,6 +36,16 @@ func main() { os.Exit(1) } + // -healthcheck: a self-check mode for Docker's HEALTHCHECK, not a + // flag anyone runs by hand. The api image is distroless (no shell, + // no wget/curl -- see api/Dockerfile), so docker-compose's + // healthcheck execs this binary against itself instead of an + // external tool. Exits before any ClickHouse/Postgres/search dial, + // since those aren't what "is the HTTP server up" is asking. + if len(os.Args) > 1 && os.Args[1] == "-healthcheck" { + os.Exit(runHealthcheck(cfg.HTTPListenAddr)) + } + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() @@ -60,12 +75,33 @@ func main() { } defer search.Close() + pgDSN := fmt.Sprintf("postgres://%s:%s@%s/%s", cfg.Postgres.Username, cfg.Postgres.Password, cfg.Postgres.Addr, cfg.Postgres.Database) + pgPool, err := pgxpool.New(ctx, pgDSN) + if err != nil { + logger.Error("opening postgres pool", "error", err) + os.Exit(1) + } + defer pgPool.Close() + + if err := pgPool.Ping(ctx); err != nil { + logger.Error("pinging postgres", "error", err) + os.Exit(1) + } + sqlRunner := executor.NewChRunner(conn) - handler := queryapi.NewHandler(logger, sqlRunner, search, cfg.QueryTimeout, cfg.CORSAllowedOrigin) + queryHandler := queryapi.NewHandler(logger, sqlRunner, search, cfg.QueryTimeout) + dashboardsHandler := dashboards.NewHandler(logger, dashboards.NewStore(pgPool)) + + // One shared mux, CORS applied once around the whole thing -- see + // internal/httpserver's doc comment for why this changed from each + // handler wrapping itself individually. + mux := http.NewServeMux() + queryHandler.RegisterRoutes(mux) + dashboardsHandler.RegisterRoutes(mux) srv := &http.Server{ Addr: cfg.HTTPListenAddr, - Handler: handler.Routes(), + Handler: httpserver.WithCORS(mux, cfg.CORSAllowedOrigin), } errCh := make(chan error, 1) @@ -88,3 +124,25 @@ func main() { } } } + +// runHealthcheck GETs its own /healthz and returns an exit code, for +// Docker's HEALTHCHECK to exec directly (see the -healthcheck flag +// above). listenAddr is HTTP_LISTEN_ADDR-shaped (e.g. ":8080") -- +// "localhost" replaces a bare host part since that's this same +// container reaching itself, not another service. +func runHealthcheck(listenAddr string) int { + addr := listenAddr + if strings.HasPrefix(addr, ":") { + addr = "localhost" + addr + } + client := http.Client{Timeout: 3 * time.Second} + resp, err := client.Get("http://" + addr + "/healthz") + if err != nil { + return 1 + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return 1 + } + return 0 +} diff --git a/api/go.mod b/api/go.mod index 92ddfff..47a7da5 100644 --- a/api/go.mod +++ b/api/go.mod @@ -17,6 +17,10 @@ require ( github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/go-faster/city v1.0.1 // indirect github.com/go-faster/errors v0.7.1 // indirect + github.com/jackc/pgpassfile v1.0.0 // indirect + github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect + github.com/jackc/pgx/v5 v5.10.0 // indirect + github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/klauspost/compress v1.19.1 // indirect github.com/paulmach/orb v0.13.0 // indirect github.com/pierrec/lz4/v4 v4.1.27 // indirect @@ -25,6 +29,7 @@ require ( go.opentelemetry.io/otel v1.44.0 // indirect go.opentelemetry.io/otel/trace v1.44.0 // indirect golang.org/x/net v0.57.0 // indirect + golang.org/x/sync v0.22.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.40.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect diff --git a/api/go.sum b/api/go.sum index 4001ba2..2f1b3cc 100644 --- a/api/go.sum +++ b/api/go.sum @@ -6,6 +6,7 @@ github.com/andybalholm/brotli v1.2.2 h1:HzTuoo2ErYQqf5qvcJInB8uvqSVxRttzkFexPWtn github.com/andybalholm/brotli v1.2.2/go.mod h1:rzTDkvFWvIrjDXZHkuS16NPggd91W3kUSvPlQ1pLaKY= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/go-faster/city v1.0.1 h1:4WAxSZ3V2Ws4QRDrscLEDcibJY8uf41H6AhXDrNDcGw= @@ -22,18 +23,30 @@ github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/jackc/pgpassfile v1.0.0 h1:/6Hmqy13Ss2zCq62VdNG8tM1wchn8zjSGOBJ6icpsIM= +github.com/jackc/pgpassfile v1.0.0/go.mod h1:CEx0iS5ambNFdcRtxPj5JhEz+xB6uRky5eyVu/W2HEg= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 h1:iCEnooe7UlwOQYpKFhBabPMi4aNAfoODPEFNiAnClxo= +github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761/go.mod h1:5TJZWKEWniPve33vlWYSoGYefn3gLQRzjfDlhSJ9ZKM= +github.com/jackc/pgx/v5 v5.10.0 h1:VhSvgU2jSli8o3AqIEOTJr7rZwAEUVo4E4XhR94Zfr0= +github.com/jackc/pgx/v5 v5.10.0/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= +github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= +github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= github.com/klauspost/compress v1.19.1 h1:VsB4HPswih7mmZ8WleSFQ75c/Ui1M4trX5oAsJnhSlk= github.com/klauspost/compress v1.19.1/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= github.com/paulmach/orb v0.13.0 h1:r7n7mQGGF+cj/CbcivEj9J3HGK+XR+yXnvzRdq9saIw= github.com/paulmach/orb v0.13.0/go.mod h1:6scRWINywA2Jf05dcjOfLfxrUIMECvTSG2MVbRLxu/k= github.com/pierrec/lz4/v4 v4.1.27 h1:+PhzhWDrjRj89TH2sw43nE3+4+W8lSxIuQadEHZyjUk= github.com/pierrec/lz4/v4 v4.1.27/go.mod h1:EoQMVJgeeEOMsCqCzqFm2O0cJvljX2nGZjcRIPL34O4= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/segmentio/asm v1.2.1 h1:DTNbBqs57ioxAD4PrArqftgypG4/qNpXoJx8TVXxPR0= github.com/segmentio/asm v1.2.1/go.mod h1:BqMnlJP91P8d+4ibuonYZw9mfnzI9HfxselHZr5aAcs= github.com/shopspring/decimal v1.4.0 h1:bxl37RwXBklmTi0C79JfXCEBD1cqqHt0bbgBAGFp81k= github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/xyproto/randomstring v1.0.5 h1:YtlWPoRdgMu3NZtP45drfy1GKoojuR7hmRcnhZqKjWU= @@ -52,6 +65,8 @@ go.opentelemetry.io/otel/trace v1.44.0 h1:jxF5CsGYCe74MCRx2X4g7WsY/VBKRqqpNvXlX/ go.opentelemetry.io/otel/trace v1.44.0/go.mod h1:oLl1jrMQAVo6v3GAggN+1VH9VIz9iUSvW53sW1Q8PIE= golang.org/x/net v0.57.0 h1:K5+3DljvIuDG9/Jv9rvyMywYNFCQ9RSUY6OOTTkT+tE= golang.org/x/net v0.57.0/go.mod h1:KpXc8iv+r3XplLAG/f7Jsf9RPszJzdR0f58q9vGOuEU= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= @@ -64,5 +79,7 @@ google.golang.org/grpc v1.83.0 h1:JeNZEKJFbQxArAMl+hiytHauacDNqJUllNfmIMmpqnQ= google.golang.org/grpc v1.83.0/go.mod h1:kDyl6SKsiHKt0uylY5gtn5cEjkrIOhQOGDgIc4JGwzQ= google.golang.org/protobuf v1.36.12 h1:pJOKDDOyeXErUroCihFAd5LQuwXBSpVnKGrj5o/fwxc= google.golang.org/protobuf v1.36.12/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/api/internal/config/config.go b/api/internal/config/config.go index 3a61e62..3e83fb1 100644 --- a/api/internal/config/config.go +++ b/api/internal/config/config.go @@ -12,6 +12,7 @@ import ( type Config struct { HTTPListenAddr string ClickHouse ClickHouseConfig + Postgres PostgresConfig SearchGRPCAddr string QueryTimeout time.Duration CORSAllowedOrigin string @@ -24,6 +25,16 @@ type ClickHouseConfig struct { Password string } +// PostgresConfig is the control-plane metadata store (dashboards, panels +// -- see /docs/phase-3-dashboard-design.md), distinct from ClickHouse +// which remains log-data-only. +type PostgresConfig struct { + Addr string + Database string + Username string + Password string +} + func Load() (Config, error) { cfg := Config{ HTTPListenAddr: getenv("HTTP_LISTEN_ADDR", ":8080"), @@ -33,6 +44,12 @@ func Load() (Config, error) { Username: getenv("CLICKHOUSE_USERNAME", "default"), Password: getenv("CLICKHOUSE_PASSWORD", ""), }, + Postgres: PostgresConfig{ + Addr: getenv("POSTGRES_ADDR", "localhost:5432"), + Database: getenv("POSTGRES_DATABASE", "sentry_metadata"), + Username: getenv("POSTGRES_USERNAME", "sentry"), + Password: getenv("POSTGRES_PASSWORD", ""), + }, // Search service's gRPC address (see /search) -- default matches // /search's own default GRPC_LISTEN_ADDR. SearchGRPCAddr: getenv("SEARCH_GRPC_ADDR", "localhost:50052"), diff --git a/api/internal/dashboards/handler.go b/api/internal/dashboards/handler.go new file mode 100644 index 0000000..b58cbce --- /dev/null +++ b/api/internal/dashboards/handler.go @@ -0,0 +1,212 @@ +package dashboards + +import ( + "context" + "encoding/json" + "errors" + "log/slog" + "net/http" +) + +// store is the narrow interface Handler depends on -- *Store (store.go) +// is the production implementation; tests use a fake, same pattern as +// queryapi's SQLRunner/SearchClient. +type store interface { + CreateDashboard(ctx context.Context, d *Dashboard) error + ListDashboards(ctx context.Context) ([]Dashboard, error) + GetDashboard(ctx context.Context, id string) (*Dashboard, error) + UpdateDashboard(ctx context.Context, d *Dashboard) error + DeleteDashboard(ctx context.Context, id string) error + AddPanel(ctx context.Context, dashboardID string, p *Panel) error + UpdatePanel(ctx context.Context, p *Panel) error + DeletePanel(ctx context.Context, dashboardID, panelID string) error + ImportDashboard(ctx context.Context, d *Dashboard) (*Dashboard, error) +} + +type Handler struct { + logger *slog.Logger + store store +} + +func NewHandler(logger *slog.Logger, store store) *Handler { + return &Handler{logger: logger, store: store} +} + +func (h *Handler) RegisterRoutes(mux *http.ServeMux) { + mux.HandleFunc("POST /dashboards", h.handleCreate) + mux.HandleFunc("GET /dashboards", h.handleList) + mux.HandleFunc("POST /dashboards/import", h.handleImport) + mux.HandleFunc("GET /dashboards/{id}", h.handleGet) + mux.HandleFunc("PUT /dashboards/{id}", h.handleUpdate) + mux.HandleFunc("DELETE /dashboards/{id}", h.handleDelete) + mux.HandleFunc("GET /dashboards/{id}/export", h.handleExport) + mux.HandleFunc("POST /dashboards/{id}/panels", h.handleAddPanel) + mux.HandleFunc("PUT /dashboards/{id}/panels/{panelId}", h.handleUpdatePanel) + mux.HandleFunc("DELETE /dashboards/{id}/panels/{panelId}", h.handleDeletePanel) +} + +const maxBodyBytes = 1 << 20 // 1 MiB, same cap as queryapi + +func (h *Handler) handleCreate(w http.ResponseWriter, r *http.Request) { + var d Dashboard + if !decodeJSON(w, r, &d) { + return + } + if d.Name == "" { + writeError(w, http.StatusBadRequest, "name must not be empty") + return + } + if err := h.store.CreateDashboard(r.Context(), &d); err != nil { + h.logger.Error("creating dashboard", "error", err) + writeError(w, http.StatusInternalServerError, "creating dashboard failed") + return + } + writeJSON(w, http.StatusCreated, d) +} + +func (h *Handler) handleList(w http.ResponseWriter, r *http.Request) { + list, err := h.store.ListDashboards(r.Context()) + if err != nil { + h.logger.Error("listing dashboards", "error", err) + writeError(w, http.StatusInternalServerError, "listing dashboards failed") + return + } + writeJSON(w, http.StatusOK, list) +} + +func (h *Handler) handleGet(w http.ResponseWriter, r *http.Request) { + d, err := h.store.GetDashboard(r.Context(), r.PathValue("id")) + if err != nil { + h.writeStoreErr(w, err, "fetching dashboard") + return + } + writeJSON(w, http.StatusOK, d) +} + +func (h *Handler) handleExport(w http.ResponseWriter, r *http.Request) { + // Export is the same document GET /dashboards/{id} returns -- the + // import endpoint below consumes exactly this shape, and so does + // `sentryctl dashboards apply`, so there's one JSON contract used + // from every call site rather than a bespoke export format. + h.handleGet(w, r) +} + +func (h *Handler) handleImport(w http.ResponseWriter, r *http.Request) { + var d Dashboard + if !decodeJSON(w, r, &d) { + return + } + if d.Name == "" { + writeError(w, http.StatusBadRequest, "name must not be empty") + return + } + imported, err := h.store.ImportDashboard(r.Context(), &d) + if err != nil { + h.logger.Error("importing dashboard", "error", err) + writeError(w, http.StatusBadRequest, err.Error()) + return + } + writeJSON(w, http.StatusCreated, imported) +} + +func (h *Handler) handleUpdate(w http.ResponseWriter, r *http.Request) { + var d Dashboard + if !decodeJSON(w, r, &d) { + return + } + if d.Name == "" { + writeError(w, http.StatusBadRequest, "name must not be empty") + return + } + d.ID = r.PathValue("id") + if err := h.store.UpdateDashboard(r.Context(), &d); err != nil { + h.writeStoreErr(w, err, "updating dashboard") + return + } + writeJSON(w, http.StatusOK, d) +} + +func (h *Handler) handleDelete(w http.ResponseWriter, r *http.Request) { + if err := h.store.DeleteDashboard(r.Context(), r.PathValue("id")); err != nil { + h.writeStoreErr(w, err, "deleting dashboard") + return + } + w.WriteHeader(http.StatusNoContent) +} + +func (h *Handler) handleAddPanel(w http.ResponseWriter, r *http.Request) { + var p Panel + if !decodeJSON(w, r, &p) { + return + } + if err := validatePanel(&p); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + if err := h.store.AddPanel(r.Context(), r.PathValue("id"), &p); err != nil { + h.logger.Error("adding panel", "error", err) + writeError(w, http.StatusInternalServerError, "adding panel failed") + return + } + writeJSON(w, http.StatusCreated, p) +} + +func (h *Handler) handleUpdatePanel(w http.ResponseWriter, r *http.Request) { + var p Panel + if !decodeJSON(w, r, &p) { + return + } + if err := validatePanel(&p); err != nil { + writeError(w, http.StatusBadRequest, err.Error()) + return + } + p.ID = r.PathValue("panelId") + p.DashboardID = r.PathValue("id") + if err := h.store.UpdatePanel(r.Context(), &p); err != nil { + h.writeStoreErr(w, err, "updating panel") + return + } + writeJSON(w, http.StatusOK, p) +} + +func (h *Handler) handleDeletePanel(w http.ResponseWriter, r *http.Request) { + if err := h.store.DeletePanel(r.Context(), r.PathValue("id"), r.PathValue("panelId")); err != nil { + h.writeStoreErr(w, err, "deleting panel") + return + } + w.WriteHeader(http.StatusNoContent) +} + +func (h *Handler) writeStoreErr(w http.ResponseWriter, err error, action string) { + if errors.Is(err, ErrNotFound) { + writeError(w, http.StatusNotFound, "not found") + return + } + h.logger.Error(action, "error", err) + writeError(w, http.StatusInternalServerError, action+" failed") +} + +func decodeJSON(w http.ResponseWriter, r *http.Request, v any) bool { + r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes) + if err := json.NewDecoder(r.Body).Decode(v); err != nil { + writeError(w, http.StatusBadRequest, "invalid JSON body: "+err.Error()) + return false + } + return true +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} + +type errorResponse struct { + Error string `json:"error"` +} + +func writeError(w http.ResponseWriter, status int, msg string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(errorResponse{Error: msg}) +} diff --git a/api/internal/dashboards/handler_test.go b/api/internal/dashboards/handler_test.go new file mode 100644 index 0000000..f374ffb --- /dev/null +++ b/api/internal/dashboards/handler_test.go @@ -0,0 +1,279 @@ +package dashboards + +import ( + "context" + "encoding/json" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +type fakeStore struct { + dashboards map[string]*Dashboard + createErr error + importErr error +} + +func newFakeStore() *fakeStore { + return &fakeStore{dashboards: map[string]*Dashboard{}} +} + +func (f *fakeStore) CreateDashboard(_ context.Context, d *Dashboard) error { + if f.createErr != nil { + return f.createErr + } + d.ID = "dash-1" + f.dashboards[d.ID] = d + return nil +} + +func (f *fakeStore) ListDashboards(_ context.Context) ([]Dashboard, error) { + var out []Dashboard + for _, d := range f.dashboards { + out = append(out, *d) + } + return out, nil +} + +func (f *fakeStore) GetDashboard(_ context.Context, id string) (*Dashboard, error) { + d, ok := f.dashboards[id] + if !ok { + return nil, ErrNotFound + } + return d, nil +} + +func (f *fakeStore) UpdateDashboard(_ context.Context, d *Dashboard) error { + existing, ok := f.dashboards[d.ID] + if !ok { + return ErrNotFound + } + panels := existing.Panels + *existing = *d + existing.Panels = panels + return nil +} + +func (f *fakeStore) DeleteDashboard(_ context.Context, id string) error { + if _, ok := f.dashboards[id]; !ok { + return ErrNotFound + } + delete(f.dashboards, id) + return nil +} + +func (f *fakeStore) AddPanel(_ context.Context, dashboardID string, p *Panel) error { + d, ok := f.dashboards[dashboardID] + if !ok { + return ErrNotFound + } + p.ID = "panel-1" + p.DashboardID = dashboardID + d.Panels = append(d.Panels, *p) + return nil +} + +func (f *fakeStore) UpdatePanel(_ context.Context, p *Panel) error { + d, ok := f.dashboards[p.DashboardID] + if !ok { + return ErrNotFound + } + for i := range d.Panels { + if d.Panels[i].ID == p.ID { + d.Panels[i] = *p + return nil + } + } + return ErrNotFound +} + +func (f *fakeStore) DeletePanel(_ context.Context, dashboardID, panelID string) error { + d, ok := f.dashboards[dashboardID] + if !ok { + return ErrNotFound + } + for i := range d.Panels { + if d.Panels[i].ID == panelID { + d.Panels = append(d.Panels[:i], d.Panels[i+1:]...) + return nil + } + } + return ErrNotFound +} + +func (f *fakeStore) ImportDashboard(_ context.Context, d *Dashboard) (*Dashboard, error) { + if f.importErr != nil { + return nil, f.importErr + } + imported := *d + imported.ID = "dash-imported" + f.dashboards[imported.ID] = &imported + return &imported, nil +} + +func newTestMux(fs *fakeStore) *http.ServeMux { + h := NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), fs) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + return mux +} + +func doRequest(t *testing.T, mux *http.ServeMux, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + var r io.Reader + if body != "" { + r = strings.NewReader(body) + } + req := httptest.NewRequest(method, path, r) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + return rec +} + +func TestCreateDashboard(t *testing.T) { + mux := newTestMux(newFakeStore()) + rec := doRequest(t, mux, http.MethodPost, "/dashboards", `{"name": "Overview"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } + var got Dashboard + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decoding response: %v", err) + } + if got.ID == "" { + t.Fatalf("expected an assigned ID, got empty") + } +} + +func TestCreateDashboardRejectsEmptyName(t *testing.T) { + mux := newTestMux(newFakeStore()) + rec := doRequest(t, mux, http.MethodPost, "/dashboards", `{"name": ""}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +func TestGetDashboardNotFound(t *testing.T) { + mux := newTestMux(newFakeStore()) + rec := doRequest(t, mux, http.MethodGet, "/dashboards/nope", "") + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", rec.Code) + } +} + +func TestAddPanelRejectsRawSQL(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", Name: "Overview"} + mux := newTestMux(fs) + + rec := doRequest(t, mux, http.MethodPost, "/dashboards/dash-1/panels", + `{"query": "SELECT 1", "query_language": "sql", "viz_type": "table"}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestAddPanelRejectsInvalidVizType(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", Name: "Overview"} + mux := newTestMux(fs) + + rec := doRequest(t, mux, http.MethodPost, "/dashboards/dash-1/panels", + `{"query": "service=api", "viz_type": "pie"}`) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400; body=%s", rec.Code, rec.Body.String()) + } +} + +func TestAddPanelSuccess(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", Name: "Overview"} + mux := newTestMux(fs) + + rec := doRequest(t, mux, http.MethodPost, "/dashboards/dash-1/panels", + `{"title": "Errors", "query": "service=api | stats count by host", "viz_type": "line", "width": 6, "height": 4}`) + if rec.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201; body=%s", rec.Code, rec.Body.String()) + } + if len(fs.dashboards["dash-1"].Panels) != 1 { + t.Fatalf("expected 1 panel stored, got %d", len(fs.dashboards["dash-1"].Panels)) + } +} + +func TestUpdateDashboardChangesTimeRange(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", Name: "Overview", DefaultEarliest: "-1h", DefaultLatest: "now"} + mux := newTestMux(fs) + + rec := doRequest(t, mux, http.MethodPut, "/dashboards/dash-1", + `{"name": "Overview", "default_earliest": "-24h", "default_latest": "now"}`) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + if fs.dashboards["dash-1"].DefaultEarliest != "-24h" { + t.Fatalf("expected default_earliest to be updated, got %q", fs.dashboards["dash-1"].DefaultEarliest) + } +} + +func TestUpdateDashboardNotFound(t *testing.T) { + mux := newTestMux(newFakeStore()) + rec := doRequest(t, mux, http.MethodPut, "/dashboards/nope", `{"name": "Overview"}`) + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", rec.Code) + } +} + +func TestDeleteDashboard(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ID: "dash-1", Name: "Overview"} + mux := newTestMux(fs) + + rec := doRequest(t, mux, http.MethodDelete, "/dashboards/dash-1", "") + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204", rec.Code) + } + if _, ok := fs.dashboards["dash-1"]; ok { + t.Fatalf("expected dashboard to be deleted") + } +} + +func TestExportThenImportRoundTrips(t *testing.T) { + fs := newFakeStore() + fs.dashboards["dash-1"] = &Dashboard{ + ID: "dash-1", Name: "Overview", + Panels: []Panel{{ID: "panel-1", DashboardID: "dash-1", Query: "service=api", VizType: VizTable}}, + } + mux := newTestMux(fs) + + exportRec := doRequest(t, mux, http.MethodGet, "/dashboards/dash-1/export", "") + if exportRec.Code != http.StatusOK { + t.Fatalf("export status = %d, want 200", exportRec.Code) + } + + importRec := doRequest(t, mux, http.MethodPost, "/dashboards/import", exportRec.Body.String()) + if importRec.Code != http.StatusCreated { + t.Fatalf("import status = %d, want 201; body=%s", importRec.Code, importRec.Body.String()) + } + var imported Dashboard + if err := json.Unmarshal(importRec.Body.Bytes(), &imported); err != nil { + t.Fatalf("decoding import response: %v", err) + } + if imported.ID == "dash-1" { + t.Fatalf("expected import to assign a fresh ID, got the source ID back") + } +} + +func TestCreateDashboardStoreErrorReturns500(t *testing.T) { + fs := newFakeStore() + fs.createErr = errors.New("boom") + mux := newTestMux(fs) + + rec := doRequest(t, mux, http.MethodPost, "/dashboards", `{"name": "Overview"}`) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } +} diff --git a/api/internal/dashboards/store.go b/api/internal/dashboards/store.go new file mode 100644 index 0000000..e232b8c --- /dev/null +++ b/api/internal/dashboards/store.go @@ -0,0 +1,256 @@ +package dashboards + +import ( + "context" + "errors" + "fmt" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// ErrNotFound is returned by Get/Delete when the id doesn't exist. +var ErrNotFound = errors.New("not found") + +// Store is the pgx-backed CRUD implementation. IDs are assigned +// server-side (google/uuid), matching how /ingest assigns record_id -- +// one place (Go) generates IDs, not split between the app and the +// database via a Postgres extension. +type Store struct { + pool *pgxpool.Pool +} + +func NewStore(pool *pgxpool.Pool) *Store { + return &Store{pool: pool} +} + +func (s *Store) CreateDashboard(ctx context.Context, d *Dashboard) error { + d.ID = uuid.NewString() + if d.TenantID == "" { + d.TenantID = "default" + } + if d.CreatedBy == "" { + d.CreatedBy = "anonymous" + } + if d.DefaultEarliest == "" { + d.DefaultEarliest = "-1h" + } + if d.DefaultLatest == "" { + d.DefaultLatest = "now" + } + row := s.pool.QueryRow(ctx, ` + INSERT INTO dashboards (id, tenant_id, name, description, default_earliest, default_latest, created_by) + VALUES ($1, $2, $3, $4, $5, $6, $7) + RETURNING created_at, updated_at`, + d.ID, d.TenantID, d.Name, d.Description, d.DefaultEarliest, d.DefaultLatest, d.CreatedBy) + return row.Scan(&d.CreatedAt, &d.UpdatedAt) +} + +func (s *Store) ListDashboards(ctx context.Context) ([]Dashboard, error) { + rows, err := s.pool.Query(ctx, ` + SELECT id, tenant_id, name, description, default_earliest, default_latest, created_by, created_at, updated_at + FROM dashboards ORDER BY created_at DESC`) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []Dashboard + for rows.Next() { + var d Dashboard + if err := rows.Scan(&d.ID, &d.TenantID, &d.Name, &d.Description, &d.DefaultEarliest, &d.DefaultLatest, &d.CreatedBy, &d.CreatedAt, &d.UpdatedAt); err != nil { + return nil, err + } + out = append(out, d) + } + return out, rows.Err() +} + +func (s *Store) GetDashboard(ctx context.Context, id string) (*Dashboard, error) { + var d Dashboard + row := s.pool.QueryRow(ctx, ` + SELECT id, tenant_id, name, description, default_earliest, default_latest, created_by, created_at, updated_at + FROM dashboards WHERE id = $1`, id) + if err := row.Scan(&d.ID, &d.TenantID, &d.Name, &d.Description, &d.DefaultEarliest, &d.DefaultLatest, &d.CreatedBy, &d.CreatedAt, &d.UpdatedAt); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return nil, ErrNotFound + } + return nil, err + } + + panels, err := s.listPanels(ctx, id) + if err != nil { + return nil, err + } + d.Panels = panels + return &d, nil +} + +func (s *Store) listPanels(ctx context.Context, dashboardID string) ([]Panel, error) { + rows, err := s.pool.Query(ctx, ` + SELECT id, dashboard_id, title, query, query_language, viz_type, viz_config, + position_x, position_y, width, height, earliest_override, latest_override, + sort_order, created_at, updated_at + FROM dashboard_panels WHERE dashboard_id = $1 ORDER BY sort_order, created_at`, dashboardID) + if err != nil { + return nil, err + } + defer rows.Close() + + var out []Panel + for rows.Next() { + var p Panel + if err := rows.Scan(&p.ID, &p.DashboardID, &p.Title, &p.Query, &p.QueryLanguage, &p.VizType, &p.VizConfig, + &p.PositionX, &p.PositionY, &p.Width, &p.Height, &p.EarliestOverride, &p.LatestOverride, + &p.SortOrder, &p.CreatedAt, &p.UpdatedAt); err != nil { + return nil, err + } + out = append(out, p) + } + return out, rows.Err() +} + +func (s *Store) UpdateDashboard(ctx context.Context, d *Dashboard) error { + if d.DefaultEarliest == "" { + d.DefaultEarliest = "-1h" + } + if d.DefaultLatest == "" { + d.DefaultLatest = "now" + } + row := s.pool.QueryRow(ctx, ` + UPDATE dashboards SET name = $1, description = $2, default_earliest = $3, default_latest = $4, updated_at = now() + WHERE id = $5 + RETURNING tenant_id, created_by, created_at, updated_at`, + d.Name, d.Description, d.DefaultEarliest, d.DefaultLatest, d.ID) + if err := row.Scan(&d.TenantID, &d.CreatedBy, &d.CreatedAt, &d.UpdatedAt); err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrNotFound + } + return err + } + return nil +} + +func (s *Store) DeleteDashboard(ctx context.Context, id string) error { + tag, err := s.pool.Exec(ctx, `DELETE FROM dashboards WHERE id = $1`, id) + if err != nil { + return err + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} + +func (s *Store) AddPanel(ctx context.Context, dashboardID string, p *Panel) error { + p.ID = uuid.NewString() + p.DashboardID = dashboardID + row := s.pool.QueryRow(ctx, ` + INSERT INTO dashboard_panels (id, dashboard_id, title, query, query_language, viz_type, viz_config, + position_x, position_y, width, height, earliest_override, latest_override, sort_order) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) + RETURNING created_at, updated_at`, + p.ID, p.DashboardID, p.Title, p.Query, p.QueryLanguage, p.VizType, p.VizConfig, + p.PositionX, p.PositionY, p.Width, p.Height, p.EarliestOverride, p.LatestOverride, p.SortOrder) + return row.Scan(&p.CreatedAt, &p.UpdatedAt) +} + +func (s *Store) UpdatePanel(ctx context.Context, p *Panel) error { + tag, err := s.pool.Exec(ctx, ` + UPDATE dashboard_panels SET + title = $1, query = $2, query_language = $3, viz_type = $4, viz_config = $5, + position_x = $6, position_y = $7, width = $8, height = $9, + earliest_override = $10, latest_override = $11, sort_order = $12, updated_at = now() + WHERE id = $13 AND dashboard_id = $14`, + p.Title, p.Query, p.QueryLanguage, p.VizType, p.VizConfig, + p.PositionX, p.PositionY, p.Width, p.Height, + p.EarliestOverride, p.LatestOverride, p.SortOrder, p.ID, p.DashboardID) + if err != nil { + return err + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} + +func (s *Store) DeletePanel(ctx context.Context, dashboardID, panelID string) error { + tag, err := s.pool.Exec(ctx, `DELETE FROM dashboard_panels WHERE id = $1 AND dashboard_id = $2`, panelID, dashboardID) + if err != nil { + return err + } + if tag.RowsAffected() == 0 { + return ErrNotFound + } + return nil +} + +// ImportDashboard creates a new dashboard and all its panels from an +// exported Dashboard document, assigning fresh IDs throughout -- so +// importing an exported dashboard into a different environment (or +// re-importing into the same one) never collides with the source IDs. +// Runs in one transaction: either the whole dashboard lands, or none of +// it does. +func (s *Store) ImportDashboard(ctx context.Context, d *Dashboard) (*Dashboard, error) { + tx, err := s.pool.Begin(ctx) + if err != nil { + return nil, err + } + defer tx.Rollback(ctx) + + id := uuid.NewString() + tenantID := d.TenantID + if tenantID == "" { + tenantID = "default" + } + createdBy := d.CreatedBy + if createdBy == "" { + createdBy = "anonymous" + } + earliest := d.DefaultEarliest + if earliest == "" { + earliest = "-1h" + } + latest := d.DefaultLatest + if latest == "" { + latest = "now" + } + + var out Dashboard + out.ID, out.TenantID, out.Name, out.Description = id, tenantID, d.Name, d.Description + out.DefaultEarliest, out.DefaultLatest, out.CreatedBy = earliest, latest, createdBy + + row := tx.QueryRow(ctx, ` + INSERT INTO dashboards (id, tenant_id, name, description, default_earliest, default_latest, created_by) + VALUES ($1, $2, $3, $4, $5, $6, $7) + RETURNING created_at, updated_at`, + out.ID, out.TenantID, out.Name, out.Description, out.DefaultEarliest, out.DefaultLatest, out.CreatedBy) + if err := row.Scan(&out.CreatedAt, &out.UpdatedAt); err != nil { + return nil, err + } + + for _, p := range d.Panels { + if err := validatePanel(&p); err != nil { + return nil, fmt.Errorf("panel %q: %w", p.Title, err) + } + p.ID = uuid.NewString() + p.DashboardID = out.ID + prow := tx.QueryRow(ctx, ` + INSERT INTO dashboard_panels (id, dashboard_id, title, query, query_language, viz_type, viz_config, + position_x, position_y, width, height, earliest_override, latest_override, sort_order) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) + RETURNING created_at, updated_at`, + p.ID, p.DashboardID, p.Title, p.Query, p.QueryLanguage, p.VizType, p.VizConfig, + p.PositionX, p.PositionY, p.Width, p.Height, p.EarliestOverride, p.LatestOverride, p.SortOrder) + if err := prow.Scan(&p.CreatedAt, &p.UpdatedAt); err != nil { + return nil, err + } + out.Panels = append(out.Panels, p) + } + + if err := tx.Commit(ctx); err != nil { + return nil, err + } + return &out, nil +} diff --git a/api/internal/dashboards/types.go b/api/internal/dashboards/types.go new file mode 100644 index 0000000..6c38466 --- /dev/null +++ b/api/internal/dashboards/types.go @@ -0,0 +1,86 @@ +// Package dashboards implements CRUD for saved, multi-panel dashboards +// -- see /docs/phase-3-dashboard-design.md. Deliberately pure CRUD: panel +// *query execution* happens client-side (the web UI calls the existing +// POST /query per panel), so this package never touches querylang. +package dashboards + +import ( + "encoding/json" + "fmt" + "time" +) + +// VizType is one of the panel visualization kinds. "top_n" renders +// through the same path as "table" -- the query itself already did the +// sort/limit -- so there's no execution-side difference, only UI framing. +type VizType string + +const ( + VizTable VizType = "table" + VizLine VizType = "line" + VizBar VizType = "bar" + VizSingleStat VizType = "single_stat" + VizTopN VizType = "top_n" +) + +func validVizType(v VizType) bool { + switch v { + case VizTable, VizLine, VizBar, VizSingleStat, VizTopN: + return true + default: + return false + } +} + +type Dashboard struct { + ID string `json:"id"` + TenantID string `json:"tenant_id"` + Name string `json:"name"` + Description string `json:"description"` + DefaultEarliest string `json:"default_earliest"` + DefaultLatest string `json:"default_latest"` + CreatedBy string `json:"created_by"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` + Panels []Panel `json:"panels,omitempty"` +} + +type Panel struct { + ID string `json:"id"` + DashboardID string `json:"dashboard_id"` + Title string `json:"title"` + Query string `json:"query"` + QueryLanguage string `json:"query_language"` + VizType VizType `json:"viz_type"` + VizConfig json.RawMessage `json:"viz_config,omitempty"` + PositionX int `json:"position_x"` + PositionY int `json:"position_y"` + Width int `json:"width"` + Height int `json:"height"` + EarliestOverride *string `json:"earliest_override,omitempty"` + LatestOverride *string `json:"latest_override,omitempty"` + SortOrder int `json:"sort_order"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` +} + +// validatePanel enforces the two rules /docs/phase-3-dashboard-design.md +// states as disclosed non-goals rather than silent gaps: raw-SQL panels +// aren't supported (time-range injection has no reliable splice point +// into arbitrary SQL), and viz_type must be one this API knows how to +// store/render. +func validatePanel(p *Panel) error { + if p.Query == "" { + return fmt.Errorf("query must not be empty") + } + if p.QueryLanguage == "sql" { + return fmt.Errorf("raw-SQL panels are not supported -- dashboards only support pipe-syntax queries, since the dashboard time-range picker is injected as leading query terms") + } + if !validVizType(p.VizType) { + return fmt.Errorf("viz_type must be one of table, line, bar, single_stat, top_n, got %q", p.VizType) + } + if len(p.VizConfig) == 0 { + p.VizConfig = json.RawMessage(`{}`) + } + return nil +} diff --git a/api/internal/httpserver/cors.go b/api/internal/httpserver/cors.go new file mode 100644 index 0000000..ac553f1 --- /dev/null +++ b/api/internal/httpserver/cors.go @@ -0,0 +1,24 @@ +// Package httpserver holds cross-handler HTTP concerns for /api. Phase 3 +// introduced a second handler package (internal/dashboards) alongside +// internal/queryapi, so CORS moved out of individual handlers into one +// wrap applied around the fully-assembled mux in cmd/api/main.go, rather +// than each handler package wrapping itself. +package httpserver + +import "net/http" + +// WithCORS is deliberately permissive by default (see CORSAllowedOrigin +// in internal/config) since there's no auth yet and the SvelteKit dev +// server runs on a different origin. Tighten alongside adding real auth. +func WithCORS(next http.Handler, allowedOrigin string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Access-Control-Allow-Origin", allowedOrigin) + w.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type") + if r.Method == http.MethodOptions { + w.WriteHeader(http.StatusNoContent) + return + } + next.ServeHTTP(w, r) + }) +} diff --git a/api/internal/httpserver/cors_test.go b/api/internal/httpserver/cors_test.go new file mode 100644 index 0000000..509eb51 --- /dev/null +++ b/api/internal/httpserver/cors_test.go @@ -0,0 +1,42 @@ +package httpserver + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestWithCORSPreflight(t *testing.T) { + inner := http.NewServeMux() + inner.HandleFunc("POST /query", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + h := WithCORS(inner, "*") + + req := httptest.NewRequest(http.MethodOptions, "/query", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204", rec.Code) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) + } +} + +func TestWithCORSPassesThroughNonPreflight(t *testing.T) { + inner := http.NewServeMux() + inner.HandleFunc("GET /healthz", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + h := WithCORS(inner, "*") + + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } +} diff --git a/api/internal/queryapi/handler.go b/api/internal/queryapi/handler.go index 462b7f6..a1dfba2 100644 --- a/api/internal/queryapi/handler.go +++ b/api/internal/queryapi/handler.go @@ -24,38 +24,24 @@ import ( ) type Handler struct { - logger *slog.Logger - sqlRunner executor.SQLRunner - search executor.SearchClient - queryTimeout time.Duration - allowedOrigin string + logger *slog.Logger + sqlRunner executor.SQLRunner + search executor.SearchClient + queryTimeout time.Duration } -func NewHandler(logger *slog.Logger, sqlRunner executor.SQLRunner, search executor.SearchClient, queryTimeout time.Duration, allowedOrigin string) *Handler { - return &Handler{logger: logger, sqlRunner: sqlRunner, search: search, queryTimeout: queryTimeout, allowedOrigin: allowedOrigin} +func NewHandler(logger *slog.Logger, sqlRunner executor.SQLRunner, search executor.SearchClient, queryTimeout time.Duration) *Handler { + return &Handler{logger: logger, sqlRunner: sqlRunner, search: search, queryTimeout: queryTimeout} } -func (h *Handler) Routes() http.Handler { - mux := http.NewServeMux() +// RegisterRoutes adds this handler's routes onto a shared mux. Phase 3 +// introduced a second handler package (internal/dashboards), so CORS is +// now applied once, by main.go, around the fully-assembled mux rather +// than by each handler wrapping itself individually -- see +// httpserver.WithCORS. +func (h *Handler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /query", h.handleQuery) mux.HandleFunc("GET /healthz", h.handleHealthz) - return h.withCORS(mux) -} - -// withCORS is deliberately permissive by default (see CORSAllowedOrigin in -// internal/config) since there's no auth yet and the SvelteKit dev server -// runs on a different origin. Tighten alongside adding real auth. -func (h *Handler) withCORS(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Access-Control-Allow-Origin", h.allowedOrigin) - w.Header().Set("Access-Control-Allow-Methods", "POST, OPTIONS") - w.Header().Set("Access-Control-Allow-Headers", "Content-Type") - if r.Method == http.MethodOptions { - w.WriteHeader(http.StatusNoContent) - return - } - next.ServeHTTP(w, r) - }) } func (h *Handler) handleHealthz(w http.ResponseWriter, _ *http.Request) { diff --git a/api/internal/queryapi/handler_test.go b/api/internal/queryapi/handler_test.go index b84e23b..f4434a5 100644 --- a/api/internal/queryapi/handler_test.go +++ b/api/internal/queryapi/handler_test.go @@ -50,14 +50,20 @@ func newTestHandler(sqlRunner *fakeSQLRunner, search *fakeSearchClient) *Handler if search == nil { search = &fakeSearchClient{} } - return NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), sqlRunner, search, time.Second, "*") + return NewHandler(slog.New(slog.NewTextHandler(io.Discard, nil)), sqlRunner, search, time.Second) +} + +func newTestMux(h *Handler) *http.ServeMux { + mux := http.NewServeMux() + h.RegisterRoutes(mux) + return mux } func postQuery(t *testing.T, h *Handler, body string) *httptest.ResponseRecorder { t.Helper() req := httptest.NewRequest(http.MethodPost, "/query", strings.NewReader(body)) rec := httptest.NewRecorder() - h.Routes().ServeHTTP(rec, req) + newTestMux(h).ServeHTTP(rec, req) return rec } @@ -195,24 +201,9 @@ func TestHandleHealthz(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/healthz", nil) rec := httptest.NewRecorder() - h.Routes().ServeHTTP(rec, req) + newTestMux(h).ServeHTTP(rec, req) if rec.Code != http.StatusOK { t.Fatalf("status = %d, want 200", rec.Code) } } - -func TestCORSPreflight(t *testing.T) { - h := newTestHandler(&fakeSQLRunner{}, nil) - req := httptest.NewRequest(http.MethodOptions, "/query", nil) - rec := httptest.NewRecorder() - - h.Routes().ServeHTTP(rec, req) - - if rec.Code != http.StatusNoContent { - t.Fatalf("status = %d, want 204", rec.Code) - } - if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { - t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) - } -} diff --git a/api/internal/querylang/executor/executor_test.go b/api/internal/querylang/executor/executor_test.go index f054585..452ed7a 100644 --- a/api/internal/querylang/executor/executor_test.go +++ b/api/internal/querylang/executor/executor_test.go @@ -300,10 +300,34 @@ func TestBuildSQLTimeRange(t *testing.T) { to := mustParseTime(t, "2026-08-14T01:00:00Z") plan := &ir.Plan{TimeRange: &ir.TimeRange{From: from, To: to}} sql := buildSQL(plan, nil) - if !strings.Contains(sql, "`timestamp` >= '2026-08-14T00:00:00Z'") { + // Space-separated, no 'T'/'Z' -- ClickHouse's implicit string->DateTime64 + // cast for a column-vs-literal comparison is strict and rejects + // RFC3339/ISO-8601 shaped literals ("code: 53, Cannot convert string... + // to type DateTime64(9, 'UTC')"), confirmed by actually running a + // dashboard panel with earliest= against live ClickHouse -- this test + // previously asserted the RFC3339 shape that ClickHouse rejects, which + // is exactly how the bug went unnoticed: nothing here ever executed the + // SQL against a real database. + if !strings.Contains(sql, "`timestamp` >= '2026-08-14 00:00:00'") { t.Fatalf("missing From bound: %s", sql) } - if !strings.Contains(sql, "`timestamp` <= '2026-08-14T01:00:00Z'") { + if !strings.Contains(sql, "`timestamp` <= '2026-08-14 01:00:00'") { t.Fatalf("missing To bound: %s", sql) } } + +func TestFormatClickHouseDateTime64OmitsTrailingZeroFraction(t *testing.T) { + // time.Time's default zero-value fractional seconds must not leave a + // stray "." with nothing after it -- Format's `.999999999` verb + // already handles this (trims to nothing when the fraction is zero), + // but it's worth pinning down given how easy the RFC3339Nano mistake + // was to miss in the first place. + got := formatClickHouseDateTime64(mustParseTime(t, "2026-08-14T00:00:00Z")) + if got != "2026-08-14 00:00:00" { + t.Fatalf("got %q, want no trailing fractional-seconds dot", got) + } + got = formatClickHouseDateTime64(mustParseTime(t, "2026-08-14T00:00:00.223505479Z")) + if got != "2026-08-14 00:00:00.223505479" { + t.Fatalf("got %q", got) + } +} diff --git a/api/internal/querylang/executor/sql.go b/api/internal/querylang/executor/sql.go index d2f06ae..4363818 100644 --- a/api/internal/querylang/executor/sql.go +++ b/api/internal/querylang/executor/sql.go @@ -164,22 +164,41 @@ func buildWhereClause(plan *ir.Plan, recordIDFilter []string) string { if plan.TimeRange != nil { if !plan.TimeRange.From.IsZero() { - conds = append(conds, "`timestamp` >= "+quoteLiteral(plan.TimeRange.From.UTC().Format(time.RFC3339Nano))) + conds = append(conds, "`timestamp` >= "+quoteLiteral(formatClickHouseDateTime64(plan.TimeRange.From))) } if !plan.TimeRange.To.IsZero() { - conds = append(conds, "`timestamp` <= "+quoteLiteral(plan.TimeRange.To.UTC().Format(time.RFC3339Nano))) + conds = append(conds, "`timestamp` <= "+quoteLiteral(formatClickHouseDateTime64(plan.TimeRange.To))) } } return strings.Join(conds, " AND ") } +// formatClickHouseDateTime64 formats t the way ClickHouse's implicit +// string->DateTime64 CAST expects for a WHERE-clause comparison: +// "YYYY-MM-DD HH:MM:SS[.fractional]", space-separated, no 'T'/'Z'. This +// is a real, measured requirement, not a guess: an ISO-8601/RFC3339Nano +// literal (e.g. "2026-08-12T20:17:40.223505479Z", what time.RFC3339Nano +// produces) fails at query time with "code: 53, Cannot convert string +// ... to type DateTime64(9, 'UTC')" -- ClickHouse's *implicit* cast used +// for column-vs-literal comparisons is strict, unlike the lenient +// parseDateTimeBestEffort used elsewhere in ClickHouse. Found by +// actually running a dashboard panel with a relative earliest= against +// live ClickHouse (Phase 2's own unit tests never caught this: they +// assert against a fake SQLRunner that checks the generated SQL string, +// not that ClickHouse accepts it, and none of Phase 2's own live-stack +// runbook queries happened to use earliest=/latest= at all). +func formatClickHouseDateTime64(t time.Time) string { + return t.UTC().Format("2006-01-02 15:04:05.999999999") +} + // buildComparisonSQL numeric-casts a non-top-level field only when the // compared value itself looks numeric -- `status>=500` casts (numeric // comparison intent), `status="unknown"` doesn't (string comparison // intent). Top-level fields are never cast; ClickHouse compares them -// against a string literal natively (DateTime64 columns parse an -// RFC3339-shaped literal, LowCardinality(String)/String compare as-is). +// against a string literal natively (LowCardinality(String)/String +// compare as-is; DateTime64 columns need formatClickHouseDateTime64's +// exact literal shape, handled in buildWhereClause above, not here). func buildComparisonSQL(f ir.FilterPredicate) string { if !topLevelFields[f.Field] && isNumericLiteral(f.Value) { return "toFloat64OrZero(" + columnExpr(f.Field) + ") " + f.Op + " " + f.Value diff --git a/cli/README.md b/cli/README.md index 4aed991..f2b5e3d 100644 --- a/cli/README.md +++ b/cli/README.md @@ -26,9 +26,35 @@ auto-detection, same optional override the HTTP API itself exposes. Prints a table by default (stdlib `text/tabwriter`, no new dependency); `--json` prints the raw `{columns, rows}` response instead. -No CLI framework (cobra/urfave-cli/etc.) — two commands don't need one, -and stdlib `os.Args` handling is boring enough not to need a dependency. -Revisit once there's a real command tree to justify one. +```sh +sentryctl dashboards list +sentryctl dashboards get +sentryctl dashboards apply dashboard.json # imports a dashboard exported via the web UI's "Export JSON" button + +sentryctl alerts list +sentryctl alerts get +sentryctl alerts apply rule.json # creates a rule from a JSON file shaped like POST /rules's body +``` + +`dashboards` talks to `/api` (`--api`, same override as `query`/`ping`). +`alerts` talks to `/alerting`, a separate service with its own base URL +(`--alerting-api`, or `$SENTRYCTL_ALERTING_API_URL`, default +`http://localhost:8081`) — see `/docs/phase-3-alerting-design.md`'s +component boundary for why alerting isn't just another `/api` route. +`apply` in both cases sends the file's JSON as-is to the corresponding +create/import endpoint — no reshaping, since the file's shape already +matches what the endpoint expects (the same JSON the web UI's export +button downloads, or the same shape `GET /rules/{id}` returns). This is +deliberately the seed of a future Terraform provider: one JSON contract, +multiple callers (web export, CLI apply, eventually a provider), not +three different formats to keep in sync. + +No CLI framework (cobra/urfave-cli/etc.) — six commands split across a +few files (`cmd_ping.go`, `cmd_query.go`, `cmd_dashboards.go`, +`cmd_alerts.go`) is still boring enough not to need one; stdlib +`os.Args` handling plus a hand-rolled `switch` in `main.go` covers it. +Revisit once there's a real command tree (nested subcommands, nontrivial +flag parsing) to justify a dependency. ## Building & testing diff --git a/cli/cmd/sentryctl/cmd_alerts.go b/cli/cmd/sentryctl/cmd_alerts.go new file mode 100644 index 0000000..c7600b5 --- /dev/null +++ b/cli/cmd/sentryctl/cmd_alerts.go @@ -0,0 +1,51 @@ +package main + +import ( + "fmt" + "io" + "os" +) + +func cmdAlerts(args []string, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprintln(stderr, "sentryctl alerts: expected a subcommand (list, get, apply)") + return 1 + } + alertingURL, rest := extractAlertingAPIFlag(args[1:], os.Getenv) + + switch args[0] { + case "list": + return httpGetJSON(alertingURL, "/rules", stdout, stderr) + case "get": + if len(rest) == 0 { + fmt.Fprintln(stderr, "sentryctl alerts get: missing rule id") + return 1 + } + return httpGetJSON(alertingURL, "/rules/"+rest[0], stdout, stderr) + case "apply": + if len(rest) == 0 { + fmt.Fprintln(stderr, "sentryctl alerts apply: missing file path") + return 1 + } + // POST /rules accepts the same shape it returns -- a rule + // definition file (query, condition, interval, notification + // target ID) applies directly with no reshaping. + return httpPostFileJSON(alertingURL, "/rules", rest[0], stdout, stderr) + default: + fmt.Fprintf(stderr, "sentryctl alerts: unknown subcommand %q (want list, get, apply)\n", args[0]) + return 1 + } +} + +func extractAlertingAPIFlag(args []string, env func(string) string) (alertingURL string, rest []string) { + alertingURL = resolveAlertingURL(env) + for i := 0; i < len(args); i++ { + if args[i] == "--alerting-api" && i+1 < len(args) { + alertingURL = args[i+1] + i++ + continue + } + rest = append(rest, args[i]) + } + return alertingURL, rest +} diff --git a/cli/cmd/sentryctl/cmd_alerts_test.go b/cli/cmd/sentryctl/cmd_alerts_test.go new file mode 100644 index 0000000..52023fd --- /dev/null +++ b/cli/cmd/sentryctl/cmd_alerts_test.go @@ -0,0 +1,63 @@ +package main + +import ( + "bytes" + "reflect" + "testing" +) + +func TestExtractAlertingAPIFlagDefault(t *testing.T) { + alertingURL, rest := extractAlertingAPIFlag([]string{"rule-1"}, func(string) string { return "" }) + if alertingURL != defaultAlertingURL { + t.Fatalf("alertingURL = %q, want default %q", alertingURL, defaultAlertingURL) + } + if !reflect.DeepEqual(rest, []string{"rule-1"}) { + t.Fatalf("rest = %v", rest) + } +} + +func TestExtractAlertingAPIFlagOverride(t *testing.T) { + alertingURL, rest := extractAlertingAPIFlag([]string{"--alerting-api", "http://custom:9091", "rule-1"}, func(string) string { return "" }) + if alertingURL != "http://custom:9091" { + t.Fatalf("alertingURL = %q", alertingURL) + } + if !reflect.DeepEqual(rest, []string{"rule-1"}) { + t.Fatalf("rest = %v", rest) + } +} + +func TestExtractAlertingAPIFlagFromEnv(t *testing.T) { + alertingURL, _ := extractAlertingAPIFlag(nil, func(k string) string { + if k == "SENTRYCTL_ALERTING_API_URL" { + return "http://env-alerting:8081" + } + return "" + }) + if alertingURL != "http://env-alerting:8081" { + t.Fatalf("alertingURL = %q", alertingURL) + } +} + +func TestCmdAlertsMissingSubcommand(t *testing.T) { + var stdout, stderr bytes.Buffer + code := cmdAlerts(nil, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1", code) + } +} + +func TestCmdAlertsApplyMissingFile(t *testing.T) { + var stdout, stderr bytes.Buffer + code := cmdAlerts([]string{"apply"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1", code) + } +} + +func TestCmdAlertsUnknownSubcommand(t *testing.T) { + var stdout, stderr bytes.Buffer + code := cmdAlerts([]string{"bogus"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1", code) + } +} diff --git a/cli/cmd/sentryctl/cmd_dashboards.go b/cli/cmd/sentryctl/cmd_dashboards.go new file mode 100644 index 0000000..5f2fa21 --- /dev/null +++ b/cli/cmd/sentryctl/cmd_dashboards.go @@ -0,0 +1,55 @@ +package main + +import ( + "fmt" + "io" + "os" +) + +func cmdDashboards(args []string, stdout, stderr io.Writer) int { + if len(args) == 0 { + fmt.Fprintln(stderr, "sentryctl dashboards: expected a subcommand (list, get, apply)") + return 1 + } + apiURL, rest := extractAPIFlag(args[1:], os.Getenv) + + switch args[0] { + case "list": + return httpGetJSON(apiURL, "/dashboards", stdout, stderr) + case "get": + if len(rest) == 0 { + fmt.Fprintln(stderr, "sentryctl dashboards get: missing dashboard id") + return 1 + } + return httpGetJSON(apiURL, "/dashboards/"+rest[0], stdout, stderr) + case "apply": + if len(rest) == 0 { + fmt.Fprintln(stderr, "sentryctl dashboards apply: missing file path") + return 1 + } + // The import endpoint consumes exactly the shape GET + // /dashboards/{id}/export produces and the web UI's Export JSON + // button downloads -- one JSON contract, three call sites. + return httpPostFileJSON(apiURL, "/dashboards/import", rest[0], stdout, stderr) + default: + fmt.Fprintf(stderr, "sentryctl dashboards: unknown subcommand %q (want list, get, apply)\n", args[0]) + return 1 + } +} + +// extractAPIFlag pulls an optional --api out of args, resolving +// the default the same way parsePingArgs/parseQueryArgs do, and returns +// the remaining positional args. Shared by dashboards and alerts since +// both take an optional --api/--alerting-api override the same way. +func extractAPIFlag(args []string, env func(string) string) (apiURL string, rest []string) { + apiURL = resolveAPIURL(env) + for i := 0; i < len(args); i++ { + if args[i] == "--api" && i+1 < len(args) { + apiURL = args[i+1] + i++ + continue + } + rest = append(rest, args[i]) + } + return apiURL, rest +} diff --git a/cli/cmd/sentryctl/cmd_dashboards_test.go b/cli/cmd/sentryctl/cmd_dashboards_test.go new file mode 100644 index 0000000..f09c77c --- /dev/null +++ b/cli/cmd/sentryctl/cmd_dashboards_test.go @@ -0,0 +1,43 @@ +package main + +import ( + "bytes" + "reflect" + "testing" +) + +func TestExtractAPIFlagDefault(t *testing.T) { + apiURL, rest := extractAPIFlag([]string{"abc123"}, func(string) string { return "" }) + if apiURL != defaultAPIURL { + t.Fatalf("apiURL = %q, want default %q", apiURL, defaultAPIURL) + } + if !reflect.DeepEqual(rest, []string{"abc123"}) { + t.Fatalf("rest = %v", rest) + } +} + +func TestExtractAPIFlagOverride(t *testing.T) { + apiURL, rest := extractAPIFlag([]string{"--api", "http://custom:9090", "abc123"}, func(string) string { return "" }) + if apiURL != "http://custom:9090" { + t.Fatalf("apiURL = %q", apiURL) + } + if !reflect.DeepEqual(rest, []string{"abc123"}) { + t.Fatalf("rest = %v, want [abc123] (flag pair stripped)", rest) + } +} + +func TestCmdDashboardsMissingSubcommand(t *testing.T) { + var stdout, stderr bytes.Buffer + code := cmdDashboards(nil, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1", code) + } +} + +func TestCmdDashboardsGetMissingID(t *testing.T) { + var stdout, stderr bytes.Buffer + code := cmdDashboards([]string{"get"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("code = %d, want 1", code) + } +} diff --git a/cli/cmd/sentryctl/cmd_ping.go b/cli/cmd/sentryctl/cmd_ping.go new file mode 100644 index 0000000..2e2de49 --- /dev/null +++ b/cli/cmd/sentryctl/cmd_ping.go @@ -0,0 +1,44 @@ +package main + +import ( + "fmt" + "io" + "net/http" + "os" + "time" +) + +// parsePingArgs resolves the api base URL for ping: --api flag wins, then +// $SENTRYCTL_API_URL, then the hardcoded default. Kept pure (env passed in +// as a function) and separate from the HTTP call so it's unit-testable +// without a real environment or server. +func parsePingArgs(args []string, env func(string) string) string { + apiURL := resolveAPIURL(env) + for i := 0; i < len(args); i++ { + if args[i] == "--api" && i+1 < len(args) { + apiURL = args[i+1] + i++ + } + } + return apiURL +} + +func cmdPing(args []string, stdout, stderr io.Writer) int { + apiURL := parsePingArgs(args, os.Getenv) + + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Get(apiURL + "/healthz") + if err != nil { + fmt.Fprintf(stderr, "ping failed: %v\n", err) + return 1 + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + fmt.Fprintf(stderr, "ping failed: api returned status %d\n", resp.StatusCode) + return 1 + } + + fmt.Fprintln(stdout, "ok") + return 0 +} diff --git a/cli/cmd/sentryctl/cmd_query.go b/cli/cmd/sentryctl/cmd_query.go new file mode 100644 index 0000000..79beae4 --- /dev/null +++ b/cli/cmd/sentryctl/cmd_query.go @@ -0,0 +1,112 @@ +package main + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strings" + "time" +) + +type queryArgs struct { + apiURL string + jsonOut bool + language string + query string +} + +// parseQueryArgs is pure (env passed in, no I/O), same testability +// reasoning as parsePingArgs. Non-flag arguments are joined with spaces +// to form the query, so `sentryctl query service=api status=500` (no +// quotes, no shell-special characters) works without requiring users to +// quote every query -- though anything using "|" still needs shell +// quoting regardless, since that's a real shell pipe character otherwise. +func parseQueryArgs(args []string, env func(string) string) queryArgs { + qa := queryArgs{apiURL: resolveAPIURL(env)} + var rest []string + for i := 0; i < len(args); i++ { + switch args[i] { + case "--api": + if i+1 < len(args) { + qa.apiURL = args[i+1] + i++ + } + case "--json": + qa.jsonOut = true + case "--language": + if i+1 < len(args) { + qa.language = args[i+1] + i++ + } + default: + rest = append(rest, args[i]) + } + } + qa.query = strings.Join(rest, " ") + return qa +} + +type queryRequestBody struct { + Query string `json:"query"` + Language string `json:"language"` +} + +type queryResponseBody struct { + Columns []string `json:"columns"` + Rows [][]any `json:"rows"` +} + +func cmdQuery(args []string, stdout, stderr io.Writer) int { + qa := parseQueryArgs(args, os.Getenv) + if strings.TrimSpace(qa.query) == "" { + fmt.Fprintln(stderr, "sentryctl query: missing query string") + return 1 + } + + reqBody, err := json.Marshal(queryRequestBody{Query: qa.query, Language: qa.language}) + if err != nil { + fmt.Fprintf(stderr, "encoding request: %v\n", err) + return 1 + } + + client := &http.Client{Timeout: 30 * time.Second} + resp, err := client.Post(qa.apiURL+"/query", "application/json", bytes.NewReader(reqBody)) + if err != nil { + fmt.Fprintf(stderr, "query failed: %v\n", err) + return 1 + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + fmt.Fprintf(stderr, "reading response: %v\n", err) + return 1 + } + + if resp.StatusCode != http.StatusOK { + var errResp errorResponseBody + if json.Unmarshal(respBody, &errResp) == nil && errResp.Error != "" { + fmt.Fprintf(stderr, "query failed: %s\n", errResp.Error) + } else { + fmt.Fprintf(stderr, "query failed: api returned status %d\n", resp.StatusCode) + } + return 1 + } + + if qa.jsonOut { + _, _ = stdout.Write(respBody) + fmt.Fprintln(stdout) + return 0 + } + + var result queryResponseBody + if err := json.Unmarshal(respBody, &result); err != nil { + fmt.Fprintf(stderr, "decoding response: %v\n", err) + return 1 + } + printTable(stdout, result.Columns, result.Rows) + return 0 +} diff --git a/cli/cmd/sentryctl/httpclient.go b/cli/cmd/sentryctl/httpclient.go new file mode 100644 index 0000000..863b80d --- /dev/null +++ b/cli/cmd/sentryctl/httpclient.go @@ -0,0 +1,72 @@ +package main + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "time" +) + +var httpClient = &http.Client{Timeout: 30 * time.Second} + +// httpGetJSON GETs path and prints the pretty-printed JSON response to +// stdout, or the error body/status to stderr. Shared by dashboards/alerts +// list and get, which otherwise differ only in path and resource name. +func httpGetJSON(baseURL, path string, stdout, stderr io.Writer) int { + resp, err := httpClient.Get(baseURL + path) + if err != nil { + fmt.Fprintf(stderr, "request failed: %v\n", err) + return 1 + } + defer resp.Body.Close() + return printJSONResponse(resp, stdout, stderr) +} + +// httpPostFileJSON reads file (a JSON document, e.g. an exported +// dashboard or a rule definition) and POSTs it to path as-is -- no +// reshaping, since the file's shape already matches what the endpoint +// expects (the same JSON the web UI's export button and POST /rules +// produce/accept respectively). This is what makes "apply" the seed of a +// future Terraform provider: one JSON contract, multiple callers. +func httpPostFileJSON(baseURL, path, file string, stdout, stderr io.Writer) int { + body, err := os.ReadFile(file) + if err != nil { + fmt.Fprintf(stderr, "reading %s: %v\n", file, err) + return 1 + } + resp, err := httpClient.Post(baseURL+path, "application/json", bytes.NewReader(body)) + if err != nil { + fmt.Fprintf(stderr, "request failed: %v\n", err) + return 1 + } + defer resp.Body.Close() + return printJSONResponse(resp, stdout, stderr) +} + +func printJSONResponse(resp *http.Response, stdout, stderr io.Writer) int { + body, err := io.ReadAll(resp.Body) + if err != nil { + fmt.Fprintf(stderr, "reading response: %v\n", err) + return 1 + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + var errResp errorResponseBody + if json.Unmarshal(body, &errResp) == nil && errResp.Error != "" { + fmt.Fprintf(stderr, "request failed: %s\n", errResp.Error) + } else { + fmt.Fprintf(stderr, "request failed: status %d\n", resp.StatusCode) + } + return 1 + } + var pretty bytes.Buffer + if json.Indent(&pretty, body, "", " ") == nil { + stdout.Write(pretty.Bytes()) + } else { + stdout.Write(body) + } + fmt.Fprintln(stdout) + return 0 +} diff --git a/cli/cmd/sentryctl/main.go b/cli/cmd/sentryctl/main.go index 6b1ad1b..9875e20 100644 --- a/cli/cmd/sentryctl/main.go +++ b/cli/cmd/sentryctl/main.go @@ -1,22 +1,23 @@ -// Command sentryctl is Sentry's control CLI: "ping" (Phase 0) and -// "query" (Phase 2), which accepts either query syntax and hits the same -// POST /query endpoint the web UI does -- no separate query logic here, -// per the Phase 2 task list's explicit instruction. +// Command sentryctl is Sentry's control CLI. Six subcommands now +// (ping, query, dashboards, alerts) clearly justify splitting dispatch +// across files -- see cli/README.md's "revisit once there's a real +// command tree" note -- while keeping the same hand-rolled switch on +// os.Args, no CLI framework, per that same README. package main import ( - "bytes" "encoding/json" "fmt" "io" - "net/http" "os" "strings" "text/tabwriter" - "time" ) -const defaultAPIURL = "http://localhost:8080" +const ( + defaultAPIURL = "http://localhost:8080" + defaultAlertingURL = "http://localhost:8081" +) func main() { os.Exit(run(os.Args[1:], os.Stdout, os.Stderr)) @@ -33,6 +34,10 @@ func run(args []string, stdout, stderr io.Writer) int { return cmdPing(args[1:], stdout, stderr) case "query": return cmdQuery(args[1:], stdout, stderr) + case "dashboards": + return cmdDashboards(args[1:], stdout, stderr) + case "alerts": + return cmdAlerts(args[1:], stdout, stderr) case "-h", "--help", "help": usage(stdout) return 0 @@ -49,15 +54,25 @@ func usage(w io.Writer) { Usage: sentryctl ping [--api ] sentryctl query "" [--api ] [--language sql|spl] [--json] + sentryctl dashboards list|get |apply [--api ] + sentryctl alerts list|get |apply [--alerting-api ] Commands: - ping Checks that the api service is reachable via GET /healthz. - query Runs a query (pipe syntax or SQL) against POST /query and - prints the result as a table, or as JSON with --json. Quote - the query in your shell -- pipe syntax uses "|", which your - shell will otherwise interpret itself. + ping Checks that the api service is reachable via GET /healthz. + query Runs a query (pipe syntax or SQL) against POST /query and + prints the result as a table, or as JSON with --json. Quote + the query in your shell -- pipe syntax uses "|", which your + shell will otherwise interpret itself. + dashboards list/get/apply against api's dashboard CRUD endpoints. + "apply " imports a dashboard exported via the web + UI's Export JSON button or GET /dashboards/{id}/export -- + the same JSON shape both places, Terraform-friendly. + alerts list/get/apply against alerting's rule CRUD endpoints. + "apply " creates a rule from a JSON file with the + same shape POST /rules accepts. --api defaults to $SENTRYCTL_API_URL, or `+defaultAPIURL+` if unset. +--alerting-api defaults to $SENTRYCTL_ALERTING_API_URL, or `+defaultAlertingURL+` if unset. --language overrides auto-detection; omit it for the common case.`) } @@ -68,145 +83,17 @@ func resolveAPIURL(env func(string) string) string { return defaultAPIURL } -// parsePingArgs resolves the api base URL for ping: --api flag wins, then -// $SENTRYCTL_API_URL, then the hardcoded default. Kept pure (env passed in -// as a function) and separate from the HTTP call so it's unit-testable -// without a real environment or server. -func parsePingArgs(args []string, env func(string) string) string { - apiURL := resolveAPIURL(env) - for i := 0; i < len(args); i++ { - if args[i] == "--api" && i+1 < len(args) { - apiURL = args[i+1] - i++ - } +func resolveAlertingURL(env func(string) string) string { + if v := env("SENTRYCTL_ALERTING_API_URL"); v != "" { + return v } - return apiURL -} - -func cmdPing(args []string, stdout, stderr io.Writer) int { - apiURL := parsePingArgs(args, os.Getenv) - - client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(apiURL + "/healthz") - if err != nil { - fmt.Fprintf(stderr, "ping failed: %v\n", err) - return 1 - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - fmt.Fprintf(stderr, "ping failed: api returned status %d\n", resp.StatusCode) - return 1 - } - - fmt.Fprintln(stdout, "ok") - return 0 -} - -type queryArgs struct { - apiURL string - jsonOut bool - language string - query string -} - -// parseQueryArgs is pure (env passed in, no I/O), same testability -// reasoning as parsePingArgs. Non-flag arguments are joined with spaces -// to form the query, so `sentryctl query service=api status=500` (no -// quotes, no shell-special characters) works without requiring users to -// quote every query -- though anything using "|" still needs shell -// quoting regardless, since that's a real shell pipe character otherwise. -func parseQueryArgs(args []string, env func(string) string) queryArgs { - qa := queryArgs{apiURL: resolveAPIURL(env)} - var rest []string - for i := 0; i < len(args); i++ { - switch args[i] { - case "--api": - if i+1 < len(args) { - qa.apiURL = args[i+1] - i++ - } - case "--json": - qa.jsonOut = true - case "--language": - if i+1 < len(args) { - qa.language = args[i+1] - i++ - } - default: - rest = append(rest, args[i]) - } - } - qa.query = strings.Join(rest, " ") - return qa -} - -type queryRequestBody struct { - Query string `json:"query"` - Language string `json:"language"` -} - -type queryResponseBody struct { - Columns []string `json:"columns"` - Rows [][]any `json:"rows"` + return defaultAlertingURL } type errorResponseBody struct { Error string `json:"error"` } -func cmdQuery(args []string, stdout, stderr io.Writer) int { - qa := parseQueryArgs(args, os.Getenv) - if strings.TrimSpace(qa.query) == "" { - fmt.Fprintln(stderr, "sentryctl query: missing query string") - return 1 - } - - reqBody, err := json.Marshal(queryRequestBody{Query: qa.query, Language: qa.language}) - if err != nil { - fmt.Fprintf(stderr, "encoding request: %v\n", err) - return 1 - } - - client := &http.Client{Timeout: 30 * time.Second} - resp, err := client.Post(qa.apiURL+"/query", "application/json", bytes.NewReader(reqBody)) - if err != nil { - fmt.Fprintf(stderr, "query failed: %v\n", err) - return 1 - } - defer resp.Body.Close() - - respBody, err := io.ReadAll(resp.Body) - if err != nil { - fmt.Fprintf(stderr, "reading response: %v\n", err) - return 1 - } - - if resp.StatusCode != http.StatusOK { - var errResp errorResponseBody - if json.Unmarshal(respBody, &errResp) == nil && errResp.Error != "" { - fmt.Fprintf(stderr, "query failed: %s\n", errResp.Error) - } else { - fmt.Fprintf(stderr, "query failed: api returned status %d\n", resp.StatusCode) - } - return 1 - } - - if qa.jsonOut { - _, _ = stdout.Write(respBody) - fmt.Fprintln(stdout) - return 0 - } - - var result queryResponseBody - if err := json.Unmarshal(respBody, &result); err != nil { - fmt.Fprintf(stderr, "decoding response: %v\n", err) - return 1 - } - printTable(stdout, result.Columns, result.Rows) - return 0 -} - func printTable(w io.Writer, columns []string, rows [][]any) { tw := tabwriter.NewWriter(w, 0, 4, 2, ' ', 0) fmt.Fprintln(tw, strings.Join(columns, "\t")) diff --git a/docker-compose.yml b/docker-compose.yml index 24f1f65..5e4e475 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -97,6 +97,41 @@ services: CLICKHOUSE_HTTP: "http://clickhouse:8123" CLICKHOUSE_PASSWORD: "sentry-dev-only" + # Control-plane metadata store (dashboards, alert rules -- see + # /docs/phase-3-dashboard-design.md for why this is Postgres rather + # than new ClickHouse tables). Log data stays on ClickHouse/Tantivy + # only, unaffected. + metadata-postgres: + image: postgres:16-alpine + container_name: sentry-metadata-postgres + environment: + POSTGRES_DB: sentry_metadata + POSTGRES_USER: sentry + POSTGRES_PASSWORD: "sentry-dev-only" # not a real secret, same framing as CLICKHOUSE_PASSWORD above + volumes: + - metadata-postgres-data:/var/lib/postgresql/data + healthcheck: + test: ["CMD-SHELL", "pg_isready -U sentry -d sentry_metadata"] + interval: 5s + timeout: 5s + retries: 30 + + # One-shot: applies /metadata/migrations/*.sql, then exits 0. api waits + # on this completing successfully, same shape as clickhouse-migrate. + metadata-migrate: + build: + context: ./metadata + container_name: sentry-metadata-migrate + depends_on: + metadata-postgres: + condition: service_healthy + environment: + POSTGRES_HOST: "metadata-postgres" + POSTGRES_PORT: "5432" + POSTGRES_USER: "sentry" + POSTGRES_PASSWORD: "sentry-dev-only" + POSTGRES_DATABASE: "sentry_metadata" + ingest: build: context: . # needs both ingest/ and proto/ @@ -148,25 +183,68 @@ services: depends_on: clickhouse-migrate: condition: service_completed_successfully + metadata-migrate: + condition: service_completed_successfully ports: - "8080:8080" environment: CLICKHOUSE_ADDR: "clickhouse:9000" CLICKHOUSE_PASSWORD: "sentry-dev-only" SEARCH_GRPC_ADDR: "search:50052" + POSTGRES_ADDR: "metadata-postgres:5432" + POSTGRES_DATABASE: "sentry_metadata" + POSTGRES_USERNAME: "sentry" + POSTGRES_PASSWORD: "sentry-dev-only" + healthcheck: + # alerting (Phase 3 task 5) depends_on api -- without this, that + # dependency can only mean "container started," not "actually + # listening," and would hammer a not-yet-ready api with errors on + # every evaluator tick during stack startup. api's image is + # distroless (no shell, no wget) so this execs the api binary's own + # -healthcheck self-check mode instead of an external tool. + test: ["CMD", "/api", "-healthcheck"] + interval: 5s + timeout: 5s + retries: 30 + + alerting: + build: + context: alerting # self-contained, no /proto needed -- see alerting/Dockerfile + dockerfile: Dockerfile + container_name: sentry-alerting + depends_on: + metadata-migrate: + condition: service_completed_successfully + api: + condition: service_healthy + ports: + - "8081:8081" + environment: + POSTGRES_ADDR: "metadata-postgres:5432" + POSTGRES_DATABASE: "sentry_metadata" + POSTGRES_USERNAME: "sentry" + POSTGRES_PASSWORD: "sentry-dev-only" + API_QUERY_URL: "http://api:8080" + healthcheck: + test: ["CMD", "/alerting", "-healthcheck"] + interval: 5s + timeout: 5s + retries: 30 web: build: context: web args: # Baked in at build time (static site, not a server) as - # localhost:8080 -- this is fetched from the *browser*, which - # resolves against the host's mapped port, not the compose - # network's service DNS name. + # localhost:8080/8081 -- fetched from the *browser*, which + # resolves against the host's mapped ports, not the compose + # network's service DNS names. VITE_API_BASE_URL: "http://localhost:8080" + VITE_ALERTING_API_BASE_URL: "http://localhost:8081" container_name: sentry-web depends_on: - api + - alerting ports: - "3000:3000" @@ -174,3 +252,4 @@ volumes: redpanda-data: clickhouse-data: search-index-data: + metadata-postgres-data: diff --git a/docs/phase-3-alerting-design.md b/docs/phase-3-alerting-design.md new file mode 100644 index 0000000..481e0c1 --- /dev/null +++ b/docs/phase-3-alerting-design.md @@ -0,0 +1,358 @@ +# Alerting design + +> **Status:** Design only — no code written against this yet. Task 4 of +> Phase 3. Per explicit instruction, execution stops here for review +> before task 5 (building `/alerting`) begins: the firing/resolved state +> machine and debounce behavior are easy to get subtly wrong, and this is +> the artifact to sign off on before anything is built against it. If +> implementation later reveals this design is wrong somewhere, fix this +> doc in the same change — same discipline as +> `/docs/query-language-design.md` and +> `/docs/phase-3-dashboard-design.md`. + +## Why this design, in one paragraph + +A rule is a saved Phase 2 query plus a condition (threshold or absence) +plus an evaluation interval plus a notification target. The genuinely +hard part isn't the data model — it's the firing/resolved state machine +under concurrent evaluation and its interaction with notification +delivery, where a naive implementation can plausibly double-fire, lose a +resolve, or silently misreport an infrastructure outage as "all clear." +This doc's state machine mirrors Prometheus Alertmanager's well-understood +`pending`/`firing` `for:` model, then adds four specific correctness +properties on top of it — not because the base model is wrong, but +because the *concurrent, at-least-once* environment a Go ticker-based +evaluator actually runs in exposes gaps a single-threaded description of +the model glosses over. Each fix below is stated as: the failure it +prevents, and the mechanism, so it's reviewable as a claim rather than +just an assertion. + +## Data model + +A rule is a saved query, evaluated on an interval, checked against a +condition, with a debounce before it's allowed to notify, and one +notification target it notifies. Two condition types: + +- **`threshold`**: the query's result must resolve to exactly one row; + the value in that row's first column is compared against + `threshold_value` via `comparator`. +- **`absence`**: the query returned zero rows. The evaluation *window* + is not a separate rule field — it's whatever `earliest=`/`latest=` the + rule's own saved query already expresses (e.g. `service=payments + severity=ERROR earliest=-5m`), reusing Phase 2's time-range syntax + rather than inventing a second one. + +```sql +CREATE TABLE notification_targets ( + id UUID PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + kind TEXT NOT NULL CHECK (kind IN ('webhook', 'slack', 'pagerduty')), + webhook_url TEXT NOT NULL, + payload_template TEXT, -- generic ("webhook") targets only; NULL for slack/pagerduty + headers JSONB NOT NULL DEFAULT '{}', + secret TEXT, -- PagerDuty routing key / generic-webhook HMAC secret -- plaintext, see "Known gaps" below + created_by TEXT NOT NULL DEFAULT 'anonymous', + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +CREATE TABLE alert_rules ( + id UUID PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + query TEXT NOT NULL, + query_language TEXT NOT NULL DEFAULT '', + condition_type TEXT NOT NULL CHECK (condition_type IN ('threshold', 'absence')), + comparator TEXT CHECK (comparator IN ('gt', 'gte', 'lt', 'lte', 'eq', 'ne')), -- NULL for absence + threshold_value DOUBLE PRECISION, -- NULL for absence + eval_interval_seconds INT NOT NULL CHECK (eval_interval_seconds >= 30), + for_minutes INT NOT NULL DEFAULT 0, -- 0 = fire on first true evaluation + renotify_interval_minutes INT, -- NULL = notify once per firing episode + notification_target_id UUID NOT NULL REFERENCES notification_targets(id), + enabled BOOLEAN NOT NULL DEFAULT true, + created_by TEXT NOT NULL DEFAULT 'anonymous', + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +CREATE TABLE alert_state ( + rule_id UUID PRIMARY KEY REFERENCES alert_rules(id) ON DELETE CASCADE, + state TEXT NOT NULL DEFAULT 'ok' CHECK (state IN ('ok', 'pending', 'firing')), + condition_true_since TIMESTAMPTZ, + fired_at TIMESTAMPTZ, + last_notified_at TIMESTAMPTZ, + last_evaluated_at TIMESTAMPTZ, + last_eval_status TEXT NOT NULL DEFAULT 'ok' CHECK (last_eval_status IN ('ok', 'error')), + last_error TEXT, + last_value DOUBLE PRECISION, + consecutive_errors INT NOT NULL DEFAULT 0, + next_eval_at TIMESTAMPTZ NOT NULL, -- the claim column, see "Concurrency" below + claimed_at TIMESTAMPTZ +); + +CREATE TABLE delivery_log ( + id BIGSERIAL PRIMARY KEY, + rule_id UUID NOT NULL REFERENCES alert_rules(id) ON DELETE CASCADE, + notification_target_id UUID NOT NULL REFERENCES notification_targets(id), + event_type TEXT NOT NULL CHECK (event_type IN ('firing', 'resolved')), + status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending', 'sent', 'failed', 'retrying')), + attempt_count INT NOT NULL DEFAULT 0, + max_attempts INT NOT NULL DEFAULT 5, + next_attempt_at TIMESTAMPTZ, -- the delivery worker's own claim key + last_attempt_at TIMESTAMPTZ, + last_error TEXT, + response_status INT, + payload JSONB NOT NULL, -- the actual rendered payload -- needed to debug template issues + created_at TIMESTAMPTZ NOT NULL DEFAULT now() +); +CREATE INDEX ON delivery_log (rule_id, created_at DESC); -- per-rule delivery log UI +CREATE INDEX ON delivery_log (status, next_attempt_at) WHERE status IN ('pending', 'retrying'); -- delivery worker's claim query +``` + +**`alert_state` must be inserted in the same transaction as its owning +`alert_rules` row**, `state = 'ok'`, `next_eval_at = now()` (evaluate +immediately on creation). A rule with no `alert_state` row is silently +never picked up by the claim query below — worth stating loudly since +it's the kind of thing that "just works" in every test that remembers to +seed both rows and then silently doesn't in production the one time it's +forgotten. + +Single notification target per rule for MVP. Multi-target (e.g. page AND +Slack) is a disclosed, straightforward future extension — a join table, +not a redesign. + +## Notification delivery: generic webhook as the base primitive + +All three `notification_targets.kind` values ultimately do the same +thing — an HTTP POST to `webhook_url` — with kind-specific *payload +formatting only*, per the explicit requirement that this stays pluggable +rather than accumulating per-vendor delivery logic: + +- **`webhook`**: `payload_template` is a Go `text/template` string, + rendered against the firing/resolved event (rule name, condition, + current value, timestamp). No template = a sane default JSON shape. +- **`slack`**: fixed formatter producing Slack's incoming-webhook shape + (`{"text": "..."}`), no user template. `payload_template` is ignored + for this kind (kept nullable in the schema rather than removed, so a + target's `kind` can be changed later without a payload column migration). +- **`pagerduty`**: fixed formatter producing PagerDuty's Events API v2 + shape (`{"routing_key": secret, "event_action": "trigger"|"resolve", + "payload": {...}}`) — `secret` here is the PagerDuty integration + routing key, not a delivery credential in the auth sense. + +All three go through the exact same HTTP POST + retry/backoff mechanism +in `internal/delivery/webhook.go`; `slack.go`/`pagerduty.go` are payload +builders only, never their own delivery path. + +## The state machine + +States per rule: `ok` → `pending` → `firing`, tracked in `alert_state`. +Every evaluation produces one of three outcomes — `condition_true`, +`condition_false`, or `error` — and error is handled entirely separately +from the other two (see fix 3). + +**On `condition_true`:** +- `ok` → `pending`: set `condition_true_since = now()`. No notification. +- `pending` → `firing`, once `now() - condition_true_since >= + for_minutes`: send a **firing** notification, set `fired_at = + last_notified_at = now()`. `for_minutes = 0` means this transition + happens on the very first true evaluation. +- `pending`, not yet past `for_minutes`: no transition, no notification. +- `firing` → stays `firing`: silent, unless `renotify_interval_minutes` + is set and `now() - last_notified_at >= renotify_interval_minutes`, in + which case re-send **firing** and update `last_notified_at`. Default + (`NULL`) is "notify once per firing episode, stay silent until + resolved" — stated explicitly since it's exactly the kind of default an + implementer would otherwise have to guess. + +**On `condition_false`:** +- `ok` → no-op. +- `pending` → `ok`: clear `condition_true_since`. **No notification** — + this was a blip inside the debounce window, not a real alert. This is + deliberate, not an oversight: it's the entire reason `for_minutes` + exists. +- `firing` → `ok`: clear `condition_true_since`/`fired_at`, send a + **resolved** notification. + +`condition_true_since` is a **wall-clock timestamp**, not a +consecutive-evaluation counter. This is what makes the debounce survive +evaluator restarts/downtime correctly: if the evaluator is down for part +of a rule's `for_minutes` window and comes back, wall-clock math +correctly resumes toward firing where it left off, while a counter would +have silently lost that progress and restarted the count. Worth +defending explicitly here since a counter looks like the "simpler" +choice and a future contributor might "simplify" it into one. + +**Disclosed non-goal: no debounce on the way down.** `firing` → `ok` +happens on a single false evaluation — there's no symmetric "stay firing +for N more minutes" hold (Grafana calls this "keep firing for"). A +condition that flickers right at the threshold produces a firing/resolved +notification pair per flicker. Future work, not solved in Phase 3. + +### Four correctness properties, and the concrete failure each one prevents + +**1. Concurrent evaluation of the same rule (claim-then-evaluate).** +A worker-pool evaluator is required at the scale task 8 targets (~500 +rules @ 60s ≈ 8+ evaluations/sec sustained). If a single evaluation's +round-trip to `api`'s `/query` ever takes longer than the rule's own +interval, the next scheduler tick can pick the *same* rule again while +the first evaluation is still in flight — two goroutines then read- +modify-write the same `alert_state` row concurrently. Depending on +timing, that produces either a duplicated firing notification (both see +`pending`, both compute "elapsed ≥ for_minutes", both fire) or a lost +resolve (a slow evaluation's stale write clobbers a faster one). + +Fix: atomically claim due rules **before** the slow network call starts: + +```sql +UPDATE alert_state +SET next_eval_at = now() + (eval_interval_seconds || ' seconds')::interval, + claimed_at = now() +FROM alert_rules +WHERE alert_state.rule_id = alert_rules.id + AND alert_state.next_eval_at <= now() + AND alert_rules.enabled +LIMIT $batch_size +RETURNING alert_state.rule_id, alert_state.state, alert_state.condition_true_since, ... +``` + +`next_eval_at` is bumped *before* the `/query` HTTP call ever starts, so +a second scheduler tick can't re-select the same rule while the first is +still running. The `/query` call itself happens **outside** any database +transaction — never hold a Postgres connection open across a network +call to another service. A second, short transaction applies the +resulting state transition once the query result is known. + +This same claim pattern is what makes horizontal evaluator replicas safe +to add later (a named Phase 4+ path, see task 8) without redesigning the +state model — each replica's claim query naturally excludes rows another +replica already claimed. Worth stating positively: this is the one place +in Phase 3 that's actively built *for* that future need, not just +avoiding a trap. + +**2. Notification loss/duplication on crash (transactional outbox).** +If the state transition and the webhook POST happen as separate, +sequentially-ordered steps, a crash between them is unrecoverable in one +direction or the other: crash after a successful POST but before the DB +commit → next evaluation replays the transition and double-fires; crash +after commit but before the POST → the notification is silently owed +forever with no record that it was ever decided. + +Fix: the state transition and `INSERT INTO delivery_log (..., status = +'pending')` happen in the **same database transaction** — "we decided to +notify" becomes durable exactly once, atomically with the state change +itself, before any network call to a notification target is attempted. A +**separate** delivery worker polls `delivery_log WHERE status IN +('pending', 'retrying') AND next_attempt_at <= now()` (same claim +pattern as fix 1), performs the actual HTTP POST, and updates +`status`/`attempt_count`/`last_error`. This decouples "did we decide to +notify" (transactionally certain) from "did the HTTP call succeed" +(best-effort with retries) — which is exactly what task 5's retry-with- +backoff requirement needs anyway, so this isn't extra machinery bolted +on for correctness's sake, it's the same piece of work. + +**3. Query errors must never be treated as `condition_false`.** +The state machine above only describes `condition_true`/ +`condition_false` — what happens when the `/query` call to `api` itself +fails (timeout, ClickHouse down, a 5xx)? Coercing an error to "false" is +the tempting default and the worst possible one: a `firing` alert would +silently auto-resolve and go quiet at precisely the moment something is +broken enough that the query can't even run. Coercing to "true" is +equally wrong the other way (spurious pages on transient infra hiccups). + +Fix: evaluation outcome is modeled as a three-way result, and an `error` +outcome **never transitions `state`** at all. It only updates +`alert_state.last_evaluated_at`, `last_eval_status = 'error'`, +`last_error`, and increments `consecutive_errors`. This must be explicit +in the implementation, not left to the "obvious" path — the obvious +path (an error propagating into whatever boolean the rest of the +function expects) is also the wrong one. + +**4. Zero rows on a `threshold` rule is an error, not a `0`.** +`stats count by host` can legitimately return zero rows for a threshold +rule (nothing matched in the window) — that's different in kind from an +`absence` rule, where zero rows *is* the signal being checked for. If a +threshold evaluation coerces "no rows" to a scalar `0` for comparison, +`count > 100` silently reports "definitely fine" in exactly the case +where the honest answer is "the query returned nothing, which might mean +nothing happened, or might mean something upstream is broken" — often +the more alarming possibility, not the safe one. + +Fix, stated as an explicit rule rather than left implicit: `threshold` +evaluation requires **exactly one** result row (first row, first numeric +column, per the dashboard/query design's single-row precedent). Zero +rows — or more than one — is treated the same as fix 3's evaluation +error, not coerced to a value. Named non-goal alongside "no per-group +alerting": a threshold rule's query must resolve to a scalar. + +## Evaluator architecture + +A single Go process (`/alerting`), ticker-driven — **not** a workflow +engine, per the explicit instruction not to reach for one at this stage. +Loop shape: + +1. Every few seconds, run the claim query (fix 1) for due, enabled rules + up to a bounded batch size. +2. Dispatch claimed rules to a bounded worker pool (goroutines). +3. Each worker: `POST /query` against `api` (reusing the existing + endpoint — the same precedent `sentryctl query` already set, never a + second query-execution path), evaluate the condition, run the state + transition (fix 3/4 aware) and, if applicable, the fix-2 transactional + outbox insert, in one short DB transaction. +4. A separate delivery-worker loop claims and sends `delivery_log` rows + independently of the evaluation loop. + +`alerting` is therefore hard-dependent on `api` being reachable — if +`api`/ClickHouse is down, every due rule's evaluation records an error +(fix 3), not a false resolve. This is documented behavior, not an +accident: you can't trust a condition you can't evaluate. `alerting`'s +docker-compose entry depends on `api`'s healthcheck (added in task 3) +for this reason. + +## Component boundary + +`/alerting` (new top-level Go service, own `go.mod`) owns rule CRUD, +notification-target CRUD, the delivery-log read endpoint, the evaluator +loop, and the delivery worker — all four pieces of "alerting" in one +service, since they share the same Postgres tables and the same +claim-based concurrency pattern. It does **not** import `api`'s +`querylang` package or talk to ClickHouse/Tantivy directly; it only +calls `api`'s `POST /query` over HTTP, exactly like `sentryctl query` +and the web UI's dashboard panels already do. `web` gets a second +backend base URL (`alerting`'s) alongside the existing `api` one. + +## Known gaps (named, not hidden) + +- **`notification_targets.secret` is stored plaintext** in Postgres — + the same posture `dashboard-design.md` already named for that domain. + Becomes both an enterprise-tier (secrets/KMS) and a multi-tenancy + (per-tenant secret isolation) concern later; naming it now avoids it + being "discovered" as a surprise security-review finding during a + future push. +- **CORS on `alerting`'s HTTP API is wide open**, matching `api`'s + existing no-auth-system posture from Phases 0–2. Not a new problem + Phase 3 introduces, just a second surface that inherits the same one. +- **No per-group/multi-row threshold alerting** (fix 4's scope decision) + and **no resolved-side debounce** (state-machine section) are both + named future work. +- **Single shared Postgres role** for `api` and `alerting`, same as + `dashboard-design.md`'s note — fine for Phase 3, a named Phase 4 item + once real auth exists. + +## Load-testing plan (task 8, not run yet) + +Seed ~500 rules via `alerting`'s own create API (not a direct DB insert +— exercise the real code path) at 60-second intervals, against a real +ClickHouse dataset (reusing `hack/benchmark-fixture`'s generator). +Measure: drift between `alert_state.next_eval_at` and the actual claim +timestamp under sustained load, `consecutive_errors`/`last_eval_status` +distribution (did the evaluator start erroring under load, as opposed to +just running slow), and `delivery_log.attempt_count`/`status` +distribution. Document real numbers, not projections — matching every +prior phase's benchmark discipline — and name what would need to change +for materially larger rule counts (horizontal evaluator replicas +partitioning by rule-id hash, which fix 1's claim design already makes +safe to add without a state-model change; moving off a single-process +ticker) as explicit Phase 4+ scope, not solved here. diff --git a/docs/phase-3-dashboard-design.md b/docs/phase-3-dashboard-design.md new file mode 100644 index 0000000..0c6c64a --- /dev/null +++ b/docs/phase-3-dashboard-design.md @@ -0,0 +1,288 @@ +# Dashboard design + +> **Status:** Approved, in progress. Task 2 of Phase 3 — see `/CLAUDE.md`'s +> "What done looks like for Phase 3" section for the exit criteria this is +> built against. Task 3 (dashboard CRUD API + web UI) implements this +> doc; if implementation reveals this design is wrong somewhere, fix this +> doc in the same change, don't let them drift apart — same discipline as +> `/docs/query-language-design.md`. + +## Why this design, in one paragraph + +A dashboard is a named collection of panels, each wrapping one saved +Phase 2 query plus a visualization type and a grid position. That's a +straightforward data model; the one real design decision here is *where +it's stored*. Dashboards and their panels need real create/update/delete +semantics with immediate read-your-writes consistency for a UI — +ClickHouse's MergeTree family isn't built for that access pattern (see +"Why not ClickHouse" below). This doc also covers the mechanism that +makes one query language work for both ad hoc search and reusable +dashboard panels: injecting the dashboard's time range into a stored +query at execution time, rather than baking a fixed time range into the +saved query itself. + +## Why not ClickHouse: PostgreSQL for control-plane config + +This phase adds PostgreSQL as a new pinned-stack component — flagged and +confirmed with the project owner before implementation, per CLAUDE.md's +"ask before... making an architectural decision not already specified in +`/docs/architecture.md`." Scope is strictly control-plane config +(dashboards, panels, and — per `/docs/phase-3-alerting-design.md` — +notification targets, alert rules, alert state, delivery log). Log data +itself is untouched: ClickHouse and Tantivy remain the only stores for +`logs`. + +Two concrete reasons ClickHouse doesn't fit this job, not just "it feels +risky": + +1. **No row-level locking primitive.** The alerting evaluator (see the + alerting design doc) needs to atomically claim a due rule so a second + evaluation tick can't touch it concurrently — a `SELECT ... FOR UPDATE + SKIP LOCKED` operation. `ReplacingMergeTree` + `FINAL` gives + eventually-consistent "latest write wins," not concurrency control. + That's a missing primitive, not a tuning problem. +2. **Read-your-writes consistency for a UI.** A user editing a dashboard + expects to immediately see their own edit reflected back. ClickHouse's + MergeTree engines don't guarantee this the way a transactional + database does without extra machinery (`FINAL`, careful ordering) that + amounts to reimplementing what Postgres already provides natively. + +Scope of the addition: new top-level `/metadata` component (naming +mirrors existing precedent — transport=Redpanda, storage=ClickHouse, +search=Tantivy → metadata=Postgres), `postgres:16-alpine` in +docker-compose, `jackc/pgx/v5` as the Go driver in `api` and the new +`alerting` service. No ORM, no `sqlc` — hand-written SQL per store +package, matching the project's existing avoidance of query-generation +machinery (no cobra, no golang-migrate, nothing code-generated from SQL +anywhere in the repo today). + +### Migrations: mirror `/storage/migrate.sh`, not a framework + +`storage/README.md`'s objection to `golang-migrate` ("premature machinery" +for what's being built) is general, not specific to ClickHouse's +HTTP-interface constraint — six new tables in one change doesn't meet the +bar that would justify revisiting that call. `/metadata/migrate.sh` +mirrors it: bash + `psql -v ON_ERROR_STOP=1 -f ` per migration, a +`schema_migrations` tracking table, one DDL object per file (kept for +repo-wide consistency of what a migration "version" means, even though +Postgres itself supports multi-statement transactions unlike ClickHouse's +HTTP interface), applied by a one-shot init container +(`metadata-migrate`) that other services `depends_on: condition: +service_completed_successfully` — same shape as `clickhouse-migrate`. + +``` +metadata/ + README.md Dockerfile migrate.sh docker-compose.yml + migrations/ + 0001_create_dashboards.sql + 0002_create_dashboard_panels.sql + 0003_create_notification_targets.sql (alerting design doc) + 0004_create_alert_rules.sql (alerting design doc) + 0005_create_alert_state.sql (alerting design doc) + 0006_create_delivery_log.sql (alerting design doc) +``` + +All six tables live in one migrations directory / one Postgres database, +even though `api` (dashboards) and `alerting` (rules/targets/state/log) +are the two services that own different tables within it — mirrors how +ClickHouse already hosts tables conceptually "owned" by different +components (`logs` via ingest's consumer, `schema_migrations` via the +migration runner itself) inside one physical database. No shared Go +store code between `api` and `alerting` — each owns hand-written SQL for +the tables it's responsible for; nothing is shared across service +`internal/` trees in this repo except `/proto`, and that precedent holds +here too. + +## Schema + +All tables carry `tenant_id TEXT NOT NULL DEFAULT 'default'` (populated, +unenforced — see "Not painting Phase 4 into a corner" below) and +`created_by TEXT NOT NULL DEFAULT 'anonymous'` for the same reason. +`updated_at` is app-managed on every UPDATE, no PL/pgSQL trigger — +consistent with the project's avoidance of database-side procedural code. + +```sql +CREATE TABLE dashboards ( + id UUID PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + default_earliest TEXT NOT NULL DEFAULT '-1h', + default_latest TEXT NOT NULL DEFAULT 'now', + created_by TEXT NOT NULL DEFAULT 'anonymous', + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); + +CREATE TABLE dashboard_panels ( + id UUID PRIMARY KEY, + dashboard_id UUID NOT NULL REFERENCES dashboards(id) ON DELETE CASCADE, + title TEXT NOT NULL DEFAULT '', + query TEXT NOT NULL, -- pipe syntax only, no earliest=/latest= + query_language TEXT NOT NULL DEFAULT '', -- mirrors queryRequest.Language: ''|sql|spl + viz_type TEXT NOT NULL CHECK (viz_type IN ('table','line','bar','single_stat','top_n')), + viz_config JSONB NOT NULL DEFAULT '{}', -- e.g. {"x_column":"timestamp","series_column":"host"} + position_x INT NOT NULL, + position_y INT NOT NULL, + width INT NOT NULL, + height INT NOT NULL, + earliest_override TEXT NULL, -- NULL = inherit dashboard default + latest_override TEXT NULL, + sort_order INT NOT NULL DEFAULT 0, -- deterministic export ordering + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); +``` + +`query_language` deliberately excludes `'sql'` from being usable in +practice for panels (enforced at the API layer, not the DB CHECK +constraint — see "Raw-SQL panels out of scope" below): the column exists +because it mirrors the existing `/query` request shape exactly, not +because raw SQL panels are supported yet. + +`position_x/y/width/height` map 1:1 to the web UI's grid layout library's +own `x/y/w/h` units — no translation layer between stored position and +rendered position. + +IDs are app-generated UUIDs (`google/uuid`, already an `api` dependency), +assigned server-side once, matching how `ingest` assigns `record_id` — +not `gen_random_uuid()` via a Postgres extension, to keep ID generation +in one place (Go) rather than split between the app and the database. + +## Time-range mechanics + +A dashboard has a default time range (`default_earliest`/ +`default_latest`, in the query language's own relative syntax, e.g. +`-1h`/`now`). Each panel can override it. Critically, **panel query +strings never contain `earliest=`/`latest=` themselves** — the query a +user writes for a panel is time-range-agnostic (e.g. `service=api | +where status>=500 | stats count by host`), and the effective time range +is resolved and injected at execution time: + +``` +effective_earliest = panel.earliest_override ?? dashboard.default_earliest +effective_latest = panel.latest_override ?? dashboard.default_latest +executed_query = "earliest={effective_earliest} latest={effective_latest} " + panel.query +``` + +This works because `earliest=`/`latest=` are ordinary `base_search` terms +in Phase 2's grammar (`term := ... | "earliest" "=" time_expr | "latest" +"=" time_expr`), implicitly AND'd with whatever else is in the query, +and — like every term in `bool_expr` — order-independent. Prepending them +is syntactically identical to a user having typed them first. + +**`"now"` is a UI-only sentinel, never injected literally.** The default +`default_latest` value shown in the picker is the human-readable string +`"now"`, but `time_expr` only accepts a quoted absolute timestamp or a +`"-N unit"` relative offset — there is no `now` token in the grammar. +Injecting `latest=now` produces a real compile error (`expected a quoted +absolute timestamp or a relative offset`), found by actually running a +dashboard panel against the live stack, not a hypothetical. The fix: +when the effective latest value is `"now"` (or empty), the `latest=` +clause is omitted from the injected query entirely — which is exactly +what the query language already does to mean "no upper bound" (see +`planner.go`'s handling of an absent `latest` term). `earliest=` has no +equivalent sentinel; a dashboard's `default_earliest` is always a real +`time_expr` value. + +**This injection only works for pipe-syntax queries.** A raw-SQL query +(Phase 2's `SELECT ...` escape hatch) has no equivalent injection point — +there's no reliable way to splice a time bound into arbitrary SQL without +parsing it, which is exactly the work the raw-SQL escape hatch was +designed to avoid (see `/docs/query-language-design.md`'s "not parsed, +wrapped as opaque IR"). **Raw-SQL dashboard panels are out of scope for +Phase 3** — a disclosed non-goal, not a silent gap. `dashboard_panels. +query_language` exists in the schema for symmetry with the `/query` +request shape, but the dashboard API layer (task 3) rejects `'sql'` on +create/update with a clear error rather than accepting it and having the +time-range picker silently do nothing. + +## Panel execution: client-side, not server-side + +`GET /dashboards/{id}` returns panel *definitions* only (query, viz_type, +position, overrides) — it does not execute any queries. The web UI +resolves each panel's effective time range and calls `POST /query` +per panel directly, exactly the same endpoint the root query page already +uses. This keeps `api/internal/dashboards` pure CRUD with zero +query-execution code of its own, consistent with the project's standing +rule that nothing duplicates `querylang`'s execution path (the same +reason `sentryctl query` calls `/query` over HTTP instead of importing +`querylang` internals). It also means panels load and error +independently in the UI — one broken panel query doesn't fail the whole +dashboard. + +## Visualization types + +- **`table`**: renders `{columns, rows}` directly via (an extended) + `ResultsTable.svelte`. No `viz_config` needed. +- **`line`** / **`bar`**: `viz_config` names which result column is the + x-axis/category (`x_column`, typically `timestamp` or a `stats ... by` + grouping column) and which is plotted (`series_column` for + multi-series, `value_column` for the plotted value). Rendered via + uPlot (see below). +- **`single_stat`**: takes the first row's first numeric column, no + `viz_config` needed for MVP (a labeled big number). +- **`top_n`**: a table sorted/limited view — for MVP this is really + "table, but the query already did `sort`/`head`," so it renders + through the same table path as `table` with different framing in the + UI; no separate execution logic. + +### New frontend dependencies + +`web/package.json` currently has zero runtime dependencies. Two are +added this phase, both confirmed with the project owner before landing: + +- **gridstack.js** — panel grid layout with drag-and-drop/resize. + Vanilla JS, framework-agnostic (wrapped in a thin Svelte component), + well-established, used by real dashboard products. Pre-approved + ("a lightweight grid library is fine, don't build drag-and-drop from + scratch") — listed here so the concrete choice is visible in the + design record, not just in a `package.json` diff. +- **uPlot** — line/bar chart rendering. ~45KB, canvas-based, fast, no + heavy transitive dependency tree. Chosen over Chart.js (heavier) and a + hand-rolled D3/SVG approach (correct axes/scales/tooltips is a lot of + code to own well for something a library already does correctly). + +Table, single-stat, and top-N panels need neither dependency. + +## Export / import + +`GET /dashboards/{id}/export` returns the dashboard and its panels +marshaled as one JSON document (the same `Dashboard`/`Panel` Go structs +used internally, not a separate export-specific shape). `POST +/dashboards/import` accepts that same shape and creates a new dashboard +from it (new IDs assigned, not a literal replay of the source IDs, so +importing into a different environment doesn't collide). This is +deliberately the *same* JSON contract `sentryctl dashboards apply` (task +7) consumes — "exportable/importable, Terraform-friendly" isn't a +separate design, it's one JSON shape used from two call sites (web +export button, CLI apply). + +## Not painting Phase 4 into a corner + +- **`tenant_id`** on every table, populated but unenforced — Phase 4's + multi-tenancy retrofit needs a partitioning/enforcement layer, not a + schema migration + backfill on every table. +- **`created_by`** on every table, defaulted to `'anonymous'` since no + auth exists yet — same reasoning, cheaper to add the column now than + during Phase 4 when real identity is also landing. +- **"Shareable" currently means "shareable within the single trusted + deployment."** There is no access-control model at all yet — not + tenant isolation, not even within-tenant per-dashboard permissions. + Stating this plainly here so it doesn't read as more solved than it is + once multi-tenancy is on the table. +- CORS stays wide open (`Access-Control-Allow-Origin: *`) on `api`, + matching the existing Phase 0–2 posture. Adding dashboards doesn't + change this tradeoff, just widens the same already-accepted surface + area — noted so it's not mistaken for a new gap introduced this phase. + +## Where this lives + +`api/internal/dashboards/` (`handler.go`, `store.go`, `types.go`, plus +tests) — mirrors `api/internal/queryapi`'s existing `Handler`/`Store` +shape. Requires one small prerequisite refactor to existing code: +`queryapi.Handler.Routes()` currently builds its own `http.NewServeMux()` +and applies CORS in one shot; with a second handler package, both need to +register onto one shared mux in `main.go`, with CORS applied once at the +top. `queryapi.Handler` gains a `RegisterRoutes(mux *http.ServeMux)` +method in place of `Routes()`. diff --git a/docs/phase-3-runbook.md b/docs/phase-3-runbook.md new file mode 100644 index 0000000..daf198f --- /dev/null +++ b/docs/phase-3-runbook.md @@ -0,0 +1,260 @@ +# Phase 3 runbook + +Extends `/docs/phase-0-runbook.md` through `/docs/phase-2-runbook.md` +with dashboards and alerting. Read those first — this assumes the stack +already works. Phase 3 adds a new Postgres-backed control-plane +(`/metadata`), a new dashboard CRUD surface in `/api`, and a new +top-level service (`/alerting`) plus its web UI and CLI subcommands. + +## What's actually been verified + +Every claim below was checked against the live stack, not asserted — +same discipline as every prior phase's runbook. This phase's real +findings (five of them, not hypothetical) are called out inline where +they were caught, matching the project's standing "actually run it" +rule: a passing test suite and a working feature are not the same claim. + +## 1. Bring up the stack + +```sh +docker compose up -d --build +``` + +New services beyond Phase 0–2: `metadata-postgres`, `metadata-migrate` +(one-shot, applies `/metadata/migrations/*.sql`), `alerting` (port 8081). +`api` now depends on `metadata-migrate` and exposes a `-healthcheck` +self-check mode (distroless, no shell/wget — see `api/cmd/api/main.go`) +so `alerting`'s `depends_on: api: condition: service_healthy` means +something real. Confirm everything is healthy: + +```sh +docker compose ps +# api, alerting, metadata-postgres, clickhouse, redpanda should all show (healthy) +``` + +## 2. Dashboards + +Generate a fixture dataset (reuses Phase 2's `hack/benchmark-fixture`): + +```sh +cd hack/benchmark-fixture +go run . --count 500000 +``` + +Create a dashboard and a couple of panels, either through the web UI +(`http://localhost:3000/dashboards` → "+ Create" → "+ Add panel") or via +`sentryctl`: + +```sh +sentryctl dashboards apply my-dashboard.json # shape = GET /dashboards/{id}/export +``` + +**Verified live**: a table panel (`severity=INFO | head 10`) and a bar +chart panel (`| stats count by host`, viz_type `bar`) both render +correctly against real data, including drag/resize via the gridstack +grid. + +### Real bugs this caught + +Building and actually loading the dashboard UI against a live stack +(not just unit tests) found four real bugs, all now fixed: + +1. **A Phase 2 bug, only surfaced now**: `earliest=`/`latest=` time-range + queries never actually worked against live ClickHouse. The executor + formatted `TimeRange` bounds with `time.RFC3339Nano` + (`2026-08-12T20:17:40.223505479Z`), but ClickHouse's *implicit* + string→`DateTime64` cast for a column comparison is strict and wants + `'YYYY-MM-DD HH:MM:SS[.fractional]'` — no `T`/`Z`. Failed with `code: + 53, Cannot convert string ... to type DateTime64(9, 'UTC')`. Nothing + in Phase 2's own runbook queries or unit tests happened to exercise a + relative time filter live; the unit test asserted the broken format + without ever asking real ClickHouse whether it was valid. Fixed in + `api/internal/querylang/executor/sql.go` (`formatClickHouseDateTime64`). +2. **`"now"` as a literal query token**: the dashboard time-range picker + injected `latest=now` verbatim; the query language has no `now` + keyword (only quoted absolute timestamps or `-N unit` relative + offsets). Fixed by treating `"now"` as a UI-only sentinel that omits + the `latest=` clause entirely (`web/src/lib/api.ts`'s + `injectTimeRange`). +3. **A layout-timing race**: GridStack/uPlot measured a panel's width + before the grid had finished laying out, producing a 74px-wide chart + canvas in a 540px container. Fixed with a `ResizeObserver` in + `PanelViz.svelte` that re-renders whenever the container's actual + size changes (also fixes chart sizing after a manual panel resize). +4. **`Date.parse` is too lenient to use as a "is this a timestamp" + check**: `Date.parse("host-06")` returns a real (bogus) timestamp + rather than `NaN`, which silently misrouted a categorical `stats + count by host` column onto a numeric time axis and rendered + unreadable giant tick labels instead of host names. Fixed by requiring + a strict ISO-8601 prefix match before attempting `Date.parse` + (`PanelViz.svelte`'s `isoTimestampPrefix`). + +## 3. Alerting + +Bring up a local webhook receiver for testing (no real Slack/PagerDuty +needed): + +```sh +docker run -d --name sentry-webhook-sink --network sentry_default \ + -p 9099:9099 -v $(pwd)/hack/webhook-sink:/src -w /src golang:1.25-alpine go run . +``` + +Create a notification target and a rule, either via the web UI +(`http://localhost:3000/alerts/new`) or directly: + +```sh +curl -X POST http://localhost:8081/targets -H 'Content-Type: application/json' -d '{ + "name": "local sink", "kind": "webhook", "webhook_url": "http://sentry-webhook-sink:9099/" +}' + +curl -X POST http://localhost:8081/rules -H 'Content-Type: application/json' -d '{ + "name": "always fires", "query": "SELECT 1", "query_language": "sql", + "condition_type": "threshold", "comparator": "gte", "threshold_value": 1, + "eval_interval_seconds": 30, "for_minutes": 0, + "notification_target_id": "" +}' +``` + +**Verified live, through both curl and the actual web UI** (created a +rule via the `/alerts/new` form, watched it transition in the browser): +the rule transitions `ok` → `firing` on its first evaluation +(`for_minutes: 0`), the delivery log shows `firing / sent / 200`, and +`docker logs sentry-webhook-sink` shows the real received payload. + +Also verified live: a threshold rule whose query returns **zero rows** +records `last_eval_status: "error"` with the exact expected message +(`"threshold rule query returned 0 rows, want exactly 1"`) and leaves +`state` untouched at `"ok"` — confirming fix 3/4 from +`/docs/phase-3-alerting-design.md` hold in practice, not just in the unit +tests that were written against them. + +### Real bugs this caught + +5. **`enabled` silently defaulted to `false`.** `rulestore.Rule.Enabled` + is a plain `bool`; a create request that simply didn't mention + `"enabled"` decoded it to Go's zero value (`false`) rather than the + intended "enabled by default." A rule created this way was silently + dead on arrival — never picked up by the evaluator's claim query, no + error anywhere. Fixed in `alerting/internal/httpapi/handler.go` with a + `createRuleRequest` wrapper using `Enabled *bool`: `nil` (omitted) + means enabled; only an explicit `"enabled": false` creates a disabled + rule. Caught by actually creating a rule through the endpoint and + checking the response, not by inspection. + +## 4. Load test (task 8) + +```sh +cd hack/alert-load-test +go run . --rule-count 500 --eval-interval-seconds 60 --duration 3m30s +``` + +Seeds 500 rules via `alerting`'s real create API (not a direct DB +insert), each querying a different host's count over the last minute +against the 500,000-row fixture dataset from §2, with an unreachably +high threshold so rules stay `ok` — isolating evaluator/ClickHouse +scheduling throughput from delivery-worker load. + +**First run, before a fix**: every one of 500 rules showed +`consecutive_errors > 0`. Root cause: the load-test tool's own query +(`host=host-01`, unquoted) hit a real, pre-existing Phase 2 lexer quirk — +an unquoted comparison value containing a literal `-` fails to parse +(`unexpected MINUS after query`). Fixed by quoting the value +(`host="host-01"`) in the load-test tool itself; not a bug worth chasing +in the query language for this runbook. + +**Second run, after that fix — a real, significant finding**: zero +errors, but the observed inter-evaluation interval was a suspiciously +exact **125.0s** for every one of 320 observed intervals, against a +configured `eval_interval_seconds: 60`. Root cause: `evaluator.tick()` +passed `workerPoolSize` (default 20) as *both* the claim batch size and +the concurrency limit — two genuinely different concerns conflated into +one number. With 500 rules all due at nearly the same instant (created +within ~65ms of each other), each 5-second tick could claim only 20 of +them regardless of how many more were already due, so draining the full +backlog took 500 ÷ 20 = 25 ticks × 5s = **125s** — more than double the +configured interval. Fixed by separating `EVALUATOR_CLAIM_BATCH_SIZE` +(default 1000 — how many due rules one tick can pull off the queue) from +`EVALUATOR_WORKER_POOL_SIZE` (default 20 — bounded concurrent `/query` +calls within that batch); see `alerting/internal/config/config.go`. + +**Third run, after the fix**: + +| Metric | Value | +|---|---| +| Rules seeded | 500 | +| Rules with `consecutive_errors > 0` | 0 | +| Configured `eval_interval_seconds` | 60 | +| Observed interval — min | 59.9s | +| Observed interval — mean | 63.3s | +| Observed interval — p95 | 65.0s | +| Observed interval — max | 65.2s | +| Drift vs. configured (p95 − configured) | 5.0s | + +The remaining ~5s of "drift" is attributable to the load test's own +5-second poll granularity (an evaluation can only be observed to within +one poll interval), not to the evaluator falling behind — 500 rules at +60s intervals is comfortably within the evaluator's capacity once the +claim-batch/worker-pool conflation was fixed. + +**Phase 4+ scaling paths, named but not solved here**: the claim query's +`SELECT ... FOR UPDATE SKIP LOCKED` design (see +`/docs/phase-3-alerting-design.md`'s fix 1) is specifically what makes +horizontal evaluator replicas safe to add later without a state-model +redesign — each replica's claim naturally excludes rows another replica +already claimed. Moving off a single-process ticker to a distributed +scheduler, and materially larger rule counts (10,000+), are both +explicitly out of scope for this phase. + +## 5. Confirm `sentryctl` + +```sh +sentryctl dashboards list +sentryctl dashboards apply exported-dashboard.json +sentryctl alerts list +sentryctl alerts apply rule.json +``` + +Both `dashboards` and `alerts` hit the exact same REST endpoints the web +UI does — `dashboards` against `/api` (`--api`), `alerts` against +`/alerting` (`--alerting-api`) — no separate CRUD logic to drift out of +sync. + +## Tearing down + +```sh +docker compose down -v # wipes the fixture dataset and all dashboards/rules +``` + +## Troubleshooting + +**A relative `earliest=`/`latest=` query fails with `code: 53, Cannot +convert string ... to type DateTime64`.** +You've reverted or bypassed `formatClickHouseDateTime64` in +`api/internal/querylang/executor/sql.go` — ClickHouse's implicit +string→`DateTime64` cast needs `'YYYY-MM-DD HH:MM:SS[.fractional]'`, not +an ISO-8601/RFC3339 literal. See §2, bug 1 above. + +**A dashboard chart panel renders with a tiny or zero-width canvas.** +Check that `PanelViz.svelte`'s `ResizeObserver` is still wired up — this +is the exact symptom of the gridstack/uPlot layout-timing race from §2, +bug 3. + +**A rule created via the API never evaluates, but no error appears +anywhere.** +Check the response's `"enabled"` field — if the create request omitted +`"enabled"` entirely and the response shows `false`, the +`createRuleRequest` fix in `alerting/internal/httpapi/handler.go` has +regressed. See §3, bug 5. + +**500+ rules at a short `eval_interval_seconds` fall behind the +configured interval.** +Check `EVALUATOR_CLAIM_BATCH_SIZE` hasn't been set equal to (or below) +`EVALUATOR_WORKER_POOL_SIZE` — that's the exact regression `hack/alert- +load-test` caught in §4. Claim batch size should stay well above the +worker pool size; the worker pool is what bounds concurrent `/query` +load, not the claim. + +**`hack/alert-load-test` reports errors on every rule.** +Check the tool's generated query quotes any comparison value that might +contain a `-` (e.g. `host="host-01"`, not `host=host-01`) — an unquoted +value with a literal hyphen fails to parse. See §4's first-run finding. diff --git a/docs/query-language-design.md b/docs/query-language-design.md index 91b38d0..57be6ee 100644 --- a/docs/query-language-design.md +++ b/docs/query-language-design.md @@ -209,6 +209,27 @@ ClickHouse-side text indexing, a different join strategy, or simply raising `max_query_size` server-side with matching memory sizing) is explicitly future work, out of scope for Phase 2. +### Post-Phase-2 fix: `earliest=`/`latest=` never actually worked against live ClickHouse + +Found during Phase 3's dashboard time-range picker work (the first thing +to run a relative `earliest=`/`latest=` query against real ClickHouse +end-to-end — none of Phase 2's own runbook queries or unit tests +happened to exercise it): `executor/sql.go` formatted `TimeRange` bounds +with `time.RFC3339Nano` (e.g. `2026-08-12T20:17:40.223505479Z`), which +ClickHouse's *implicit* string→`DateTime64` cast for a column-vs-literal +comparison rejects outright — `code: 53, Cannot convert string ... to +type DateTime64(9, 'UTC')`. ClickHouse's implicit cast is strict and +wants `'YYYY-MM-DD HH:MM:SS[.fractional]'` (space-separated, no `T`/`Z`); +the lenient ISO-8601-accepting `parseDateTimeBestEffort` is a different, +explicitly-invoked function, not what a plain `WHERE timestamp >= '...'` +comparison uses. Fixed by `formatClickHouseDateTime64` in `sql.go`. The +Phase 2 unit test that covered this (`TestBuildSQLTimeRange`) only +asserted the generated SQL *string*, against a fake `SQLRunner` — it +never caught this because nothing in that test actually asked real +ClickHouse whether the SQL was valid. Left here as a pointed reminder of +why this project's "actually run it" discipline exists: a passing test +suite and a working feature are not the same claim. + ## Where this lives: `api/internal/querylang/` Not a new top-level component. This subsystem always executes in-process diff --git a/hack/alert-load-test/README.md b/hack/alert-load-test/README.md new file mode 100644 index 0000000..a709914 --- /dev/null +++ b/hack/alert-load-test/README.md @@ -0,0 +1,42 @@ +# alert-load-test + +Seeds a realistic number of concurrent alert rules via `/alerting`'s real +create API (not a direct DB insert) and measures whether the evaluator's +claim scheduling keeps up under load. See +`/docs/phase-3-alerting-design.md`'s "Load-testing plan" and +`/docs/phase-3-runbook.md` for the methodology and real measured results. + +```sh +# 1. Push real data so rule queries have real work to do (reuses +# hack/benchmark-fixture): +cd ../benchmark-fixture +go run . --count 500000 + +# 2. Run a webhook-sink so the (never-firing, by design) rules have a +# valid notification target to point at: +docker run -d --name sentry-webhook-sink --network sentry_default \ + -p 9099:9099 -v $(pwd)/../webhook-sink:/src -w /src golang:1.25-alpine go run . + +# 3. Run the load test: +cd ../alert-load-test +go run . --rule-count 500 --eval-interval-seconds 60 --duration 3m30s +``` + +Each rule queries a different host's count over the last minute +(`earliest=-1m host="host-01" | stats count`) against real ClickHouse +data, with `threshold_value` set unreachably high so rules stay `ok` -- +this isolates evaluator/ClickHouse scheduling throughput from +delivery-worker load (a query that never fires still exercises the exact +same claim → `/query` → evaluate → `ApplyTransition` path every tick). + +The report shows, per rule, the observed intervals between consecutive +`last_evaluated_at` changes (polled at `--poll-interval`), compared +against the configured `eval_interval_seconds`. A real, significant +finding from actually running this: the evaluator's claim batch size and +worker-pool concurrency limit defaulted to the same number (20), so 500 +rules all due at once took 125s to cycle through instead of the +configured 60s. Fixed by separating `EVALUATOR_CLAIM_BATCH_SIZE` from +`EVALUATOR_WORKER_POOL_SIZE` (see `alerting/internal/config/config.go`). + +Cleans up the seeded rules and notification target on exit unless +`--no-cleanup` is passed. diff --git a/hack/alert-load-test/go.mod b/hack/alert-load-test/go.mod new file mode 100644 index 0000000..3a5b8f6 --- /dev/null +++ b/hack/alert-load-test/go.mod @@ -0,0 +1,3 @@ +module github.com/sentry/sentry/hack/alert-load-test + +go 1.25.0 diff --git a/hack/alert-load-test/main.go b/hack/alert-load-test/main.go new file mode 100644 index 0000000..5b847c0 --- /dev/null +++ b/hack/alert-load-test/main.go @@ -0,0 +1,286 @@ +// Command alert-load-test seeds a realistic number of concurrent alert +// rules via alerting's real create API (not a direct DB insert -- the +// same discipline hack/benchmark-fixture uses, exercising the actual +// code path under test) and measures whether the evaluator's claim +// scheduling keeps up under load. See /docs/phase-3-alerting-design.md's +// "Load-testing plan" and /docs/phase-3-runbook.md for the methodology +// and real measured results from running this. +package main + +import ( + "bytes" + "encoding/json" + "flag" + "fmt" + "net/http" + "os" + "sort" + "sync" + "time" +) + +type target struct { + ID string `json:"id"` +} + +type ruleState struct { + State string `json:"state"` + LastEvaluatedAt *string `json:"last_evaluated_at,omitempty"` + LastEvalStatus string `json:"last_eval_status"` + ConsecutiveErrors int `json:"consecutive_errors"` +} + +type rule struct { + ID string `json:"id"` + State ruleState `json:"state"` +} + +var ( + alertingURL = flag.String("alerting-url", "http://localhost:8081", "alerting service base URL") + ruleCount = flag.Int("rule-count", 500, "number of rules to seed") + evalInterval = flag.Int("eval-interval-seconds", 60, "each rule's eval_interval_seconds") + duration = flag.Duration("duration", 5*time.Minute, "how long to observe after seeding") + pollInterval = flag.Duration("poll-interval", 5*time.Second, "how often to poll GET /rules while observing") + concurrency = flag.Int("concurrency", 20, "concurrent rule-creation requests") + skipCleanup = flag.Bool("no-cleanup", false, "leave the seeded rules/target in place after the run") + webhookURL = flag.String("webhook-url", "http://sentry-webhook-sink:9099/", "notification target URL -- default assumes a webhook-sink container reachable on the compose network") +) + +func main() { + flag.Parse() + client := &http.Client{Timeout: 30 * time.Second} + + fmt.Printf("creating notification target...\n") + targetID, err := createTarget(client) + if err != nil { + fmt.Fprintln(os.Stderr, "creating notification target:", err) + os.Exit(1) + } + fmt.Printf("target_id=%s\n", targetID) + + fmt.Printf("seeding %d rules at %d concurrent requests...\n", *ruleCount, *concurrency) + start := time.Now() + ruleIDs, err := seedRules(client, targetID) + if err != nil { + fmt.Fprintln(os.Stderr, "seeding rules:", err) + os.Exit(1) + } + fmt.Printf("seeded %d rules in %s\n", len(ruleIDs), time.Since(start)) + + fmt.Printf("observing for %s (polling every %s)...\n", *duration, *pollInterval) + observations := observe(client) + + report(observations, ruleIDs) + + if !*skipCleanup { + fmt.Println("cleaning up...") + cleanup(client, ruleIDs, targetID) + } +} + +func createTarget(client *http.Client) (string, error) { + body, _ := json.Marshal(map[string]string{ + "name": "alert-load-test", "kind": "webhook", "webhook_url": *webhookURL, + }) + resp, err := client.Post(*alertingURL+"/targets", "application/json", bytes.NewReader(body)) + if err != nil { + return "", err + } + defer resp.Body.Close() + var t target + if err := json.NewDecoder(resp.Body).Decode(&t); err != nil { + return "", err + } + return t.ID, nil +} + +// seedRules creates *ruleCount rules, each querying a different host's +// count over the last minute against real ClickHouse data (reuse +// hack/benchmark-fixture to populate that data first). threshold_value +// is deliberately unreachably high (real fixture volumes stay well +// under it) so rules stay in "ok" and the measurement here isolates +// evaluator/ClickHouse scheduling throughput, not delivery-worker load +// -- a query that never fires still exercises the exact same claim -> +// /query -> evaluateCondition -> ApplyTransition path every tick. +func seedRules(client *http.Client, targetID string) ([]string, error) { + hosts := []string{"host-01", "host-02", "host-03", "host-04", "host-05", "host-06", "host-07", "host-08"} + + type result struct { + id string + err error + } + work := make(chan int, *ruleCount) + for i := 0; i < *ruleCount; i++ { + work <- i + } + close(work) + + results := make(chan result, *ruleCount) + var wg sync.WaitGroup + for w := 0; w < *concurrency; w++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := range work { + host := hosts[i%len(hosts)] + body, _ := json.Marshal(map[string]any{ + "name": fmt.Sprintf("load-test-rule-%d", i), + // host=host-01 (unquoted) fails to parse -- Phase 2's query + // language lexer treats the "-" in an unquoted comparison + // value as a token boundary (confirmed by actually running + // this query: "unexpected MINUS after query"). Quoting the + // value is the fix, not a language bug worth chasing here. + "query": fmt.Sprintf(`earliest=-1m host="%s" | stats count`, host), + "condition_type": "threshold", + "comparator": "gt", + "threshold_value": 1_000_000_000, + "eval_interval_seconds": *evalInterval, + "notification_target_id": targetID, + }) + resp, err := client.Post(*alertingURL+"/rules", "application/json", bytes.NewReader(body)) + if err != nil { + results <- result{err: err} + continue + } + var r rule + decodeErr := json.NewDecoder(resp.Body).Decode(&r) + resp.Body.Close() + if decodeErr != nil || r.ID == "" { + results <- result{err: fmt.Errorf("rule %d: unexpected response (status %d)", i, resp.StatusCode)} + continue + } + results <- result{id: r.ID} + } + }() + } + wg.Wait() + close(results) + + var ids []string + var errs int + for r := range results { + if r.err != nil { + errs++ + if errs <= 5 { + fmt.Fprintln(os.Stderr, "seed error:", r.err) + } + continue + } + ids = append(ids, r.id) + } + if errs > 0 { + fmt.Fprintf(os.Stderr, "%d rule-creation errors (showing up to 5 above)\n", errs) + } + return ids, nil +} + +func observe(client *http.Client) [][]rule { + deadline := time.Now().Add(*duration) + var polls [][]rule + for time.Now().Before(deadline) { + resp, err := client.Get(*alertingURL + "/rules") + if err == nil { + var rules []rule + if json.NewDecoder(resp.Body).Decode(&rules) == nil { + polls = append(polls, rules) + } + resp.Body.Close() + } + time.Sleep(*pollInterval) + } + return polls +} + +func report(observations [][]rule, seededIDs []string) { + if len(observations) < 2 { + fmt.Println("not enough observations to compute drift (need at least 2 polls)") + return + } + + // Per rule, collect the distinct last_evaluated_at timestamps seen + // across polls, in order -- consecutive differences are the observed + // inter-evaluation intervals. + seen := map[string][]string{} + latestErrors := map[string]int{} + for _, poll := range observations { + for _, r := range poll { + if r.State.LastEvaluatedAt != nil { + ts := *r.State.LastEvaluatedAt + vals := seen[r.ID] + if len(vals) == 0 || vals[len(vals)-1] != ts { + seen[r.ID] = append(vals, ts) + } + } + latestErrors[r.ID] = r.State.ConsecutiveErrors + } + } + + var intervals []float64 + for _, timestamps := range seen { + var parsed []time.Time + for _, ts := range timestamps { + t, err := time.Parse(time.RFC3339Nano, ts) + if err == nil { + parsed = append(parsed, t) + } + } + sort.Slice(parsed, func(i, j int) bool { return parsed[i].Before(parsed[j]) }) + for i := 1; i < len(parsed); i++ { + intervals = append(intervals, parsed[i].Sub(parsed[i-1]).Seconds()) + } + } + + erroring := 0 + for _, n := range latestErrors { + if n > 0 { + erroring++ + } + } + + fmt.Println() + fmt.Println("=== alert-load-test results ===") + fmt.Printf("rules seeded: %d\n", len(seededIDs)) + fmt.Printf("rules observed with at least one evaluation: %d\n", len(seen)) + fmt.Printf("rules with consecutive_errors > 0 at last poll: %d\n", erroring) + fmt.Printf("configured eval_interval_seconds: %d\n", *evalInterval) + if len(intervals) == 0 { + fmt.Println("no inter-evaluation intervals observed (duration may be too short relative to eval_interval_seconds)") + return + } + sort.Float64s(intervals) + fmt.Printf("observed evaluation intervals (n=%d): min=%.1fs mean=%.1fs p50=%.1fs p95=%.1fs max=%.1fs\n", + len(intervals), intervals[0], mean(intervals), percentile(intervals, 0.5), percentile(intervals, 0.95), intervals[len(intervals)-1]) + fmt.Printf("drift vs configured interval (p95 - configured): %.1fs\n", percentile(intervals, 0.95)-float64(*evalInterval)) +} + +func mean(xs []float64) float64 { + var sum float64 + for _, x := range xs { + sum += x + } + return sum / float64(len(xs)) +} + +func percentile(sorted []float64, p float64) float64 { + if len(sorted) == 0 { + return 0 + } + idx := int(p * float64(len(sorted)-1)) + return sorted[idx] +} + +func cleanup(client *http.Client, ruleIDs []string, targetID string) { + for _, id := range ruleIDs { + req, _ := http.NewRequest(http.MethodDelete, *alertingURL+"/rules/"+id, nil) + resp, err := client.Do(req) + if err == nil { + resp.Body.Close() + } + } + req, _ := http.NewRequest(http.MethodDelete, *alertingURL+"/targets/"+targetID, nil) + resp, err := client.Do(req) + if err == nil { + resp.Body.Close() + } + fmt.Printf("deleted %d rules and 1 target\n", len(ruleIDs)) +} diff --git a/hack/webhook-sink/go.mod b/hack/webhook-sink/go.mod new file mode 100644 index 0000000..356772d --- /dev/null +++ b/hack/webhook-sink/go.mod @@ -0,0 +1,3 @@ +module github.com/sentry/sentry/hack/webhook-sink + +go 1.25.0 diff --git a/hack/webhook-sink/main.go b/hack/webhook-sink/main.go new file mode 100644 index 0000000..ad75839 --- /dev/null +++ b/hack/webhook-sink/main.go @@ -0,0 +1,33 @@ +// Command webhook-sink is a minimal HTTP receiver for manually verifying +// alert delivery against a live stack (see /docs/phase-3-runbook.md) when +// a real Slack workspace/PagerDuty account isn't available -- logs every +// received POST body to stdout and returns 200. Not part of any +// docker-compose service by default; run it standalone when you need it. +package main + +import ( + "flag" + "fmt" + "io" + "log" + "net/http" + "time" +) + +func main() { + addr := flag.String("addr", ":9099", "listen address") + flag.Parse() + + http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, "reading body: "+err.Error(), http.StatusBadRequest) + return + } + fmt.Printf("[%s] %s %s\n%s\n\n", time.Now().UTC().Format(time.RFC3339), r.Method, r.URL.Path, body) + w.WriteHeader(http.StatusOK) + }) + + log.Printf("webhook-sink listening on %s", *addr) + log.Fatal(http.ListenAndServe(*addr, nil)) +} diff --git a/metadata/Dockerfile b/metadata/Dockerfile new file mode 100644 index 0000000..d6be65a --- /dev/null +++ b/metadata/Dockerfile @@ -0,0 +1,9 @@ +# One-shot migration runner: bash + psql client baked in, migrations/*.sql +# copied in at build time. Mirrors /storage/Dockerfile's shape. +# docker build -f metadata/Dockerfile -t sentry-metadata-migrate metadata/ +FROM alpine:3.20 +RUN apk add --no-cache bash postgresql16-client +WORKDIR /metadata +COPY migrate.sh ./ +COPY migrations ./migrations +ENTRYPOINT ["bash", "migrate.sh"] diff --git a/metadata/README.md b/metadata/README.md new file mode 100644 index 0000000..773dbe3 --- /dev/null +++ b/metadata/README.md @@ -0,0 +1,69 @@ +# metadata + +PostgreSQL schema and migration tooling for Sentry's control-plane +config: dashboards, alert rules, and everything else that isn't log data. +See `/docs/phase-3-dashboard-design.md` and +`/docs/phase-3-alerting-design.md` for why this is a separate database +from `/storage` (ClickHouse) rather than new ClickHouse tables — short +version: dashboards and alert state need real row-level locking and +transactional read-modify-write, which ClickHouse's MergeTree family +doesn't provide. + +## Schema + +Six tables across two features, one shared database (`sentry_metadata`): + +- `dashboards`, `dashboard_panels` — owned by `/api` (`api/internal/dashboards`) +- `notification_targets`, `alert_rules`, `alert_state`, `delivery_log` — + owned by `/alerting` + +"Owned" here is a documentation convention, not a technical boundary — +both services connect to the same Postgres instance/database, each with +its own hand-written SQL for the tables it's responsible for. Nothing is +shared across service `internal/` trees for this, matching the existing +repo convention that only `/proto` is shared code (and even that isn't +shared logic, just generated bindings). + +## Migration tooling: mirrors `/storage/migrate.sh`, not a framework + +Same reasoning as `/storage/README.md`: pulling in `golang-migrate` for +what's currently six `CREATE TABLE` statements is premature machinery. +`migrate.sh` applies `migrations/*.sql` in filename order over `psql`, +tracking what's applied in a `schema_migrations` table, one DDL object +per file (kept for repo-wide consistency of what a migration "version" +means, even though Postgres itself supports multi-statement transactions +unlike ClickHouse's HTTP interface). + +## Running + +```sh +docker compose up -d # starts a standalone Postgres for local work +POSTGRES_PASSWORD=sentry-dev-only ./migrate.sh # applies migrations/*.sql +``` + +Environment variables `migrate.sh` reads (all optional except +`POSTGRES_PASSWORD`, matching the root docker-compose.yml's +`metadata-postgres` service): + +| Var | Default | +|---|---| +| `POSTGRES_HOST` | `localhost` | +| `POSTGRES_PORT` | `5432` | +| `POSTGRES_USER` | `sentry` | +| `POSTGRES_PASSWORD` | (empty — must be set) | +| `POSTGRES_DATABASE` | `sentry_metadata` | + +The database itself isn't created by `migrate.sh` — the `postgres:16-alpine` +image auto-creates `POSTGRES_DB` on first startup, unlike ClickHouse where +`migrate.sh` has to issue `CREATE DATABASE IF NOT EXISTS` itself. + +There's also a `Dockerfile` (bash + the `postgresql16-client` package +baked in, `migrations/` copied in at build time) used by the root-level +`docker-compose.yml` as a one-shot init service (`metadata-migrate`) — +no runtime package install, no host volume mount needed. + +## Adding a migration + +Add `migrations/000N_description.sql` with the next sequential number and +a single DDL statement. `migrate.sh` picks it up automatically — no +registration step. diff --git a/metadata/docker-compose.yml b/metadata/docker-compose.yml new file mode 100644 index 0000000..912d9e9 --- /dev/null +++ b/metadata/docker-compose.yml @@ -0,0 +1,19 @@ +# Standalone Postgres for local development against /metadata in +# isolation (e.g. iterating on migrations). The root-level docker-compose.yml +# runs the full stack and defines its own metadata-postgres service +# separately — this file is not included by it. +services: + postgres: + image: postgres:16-alpine + container_name: sentry-metadata-postgres + ports: + - "5432:5432" + environment: + POSTGRES_DB: sentry_metadata + POSTGRES_USER: sentry + POSTGRES_PASSWORD: "sentry-dev-only" # not a real secret, see root docker-compose.yml + volumes: + - metadata-postgres-data:/var/lib/postgresql/data + +volumes: + metadata-postgres-data: diff --git a/metadata/migrate.sh b/metadata/migrate.sh new file mode 100644 index 0000000..493e30e --- /dev/null +++ b/metadata/migrate.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# Applies migrations/*.sql to Postgres in filename order, tracking what's +# already been applied in a schema_migrations table -- same shape as +# /storage/migrate.sh, adapted for psql instead of curl since Postgres +# supports real transactions per file (kept to one DDL object per file +# anyway, for repo-wide consistency of what a migration "version" means). +set -euo pipefail + +POSTGRES_HOST="${POSTGRES_HOST:-localhost}" +POSTGRES_PORT="${POSTGRES_PORT:-5432}" +POSTGRES_USER="${POSTGRES_USER:-sentry}" +POSTGRES_PASSWORD="${POSTGRES_PASSWORD:-}" +POSTGRES_DATABASE="${POSTGRES_DATABASE:-sentry_metadata}" + +export PGPASSWORD="$POSTGRES_PASSWORD" + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +MIGRATIONS_DIR="${SCRIPT_DIR}/migrations" + +psql_exec() { + psql -v ON_ERROR_STOP=1 -X -q -h "$POSTGRES_HOST" -p "$POSTGRES_PORT" -U "$POSTGRES_USER" -d "$POSTGRES_DATABASE" "$@" +} + +echo "Ensuring schema_migrations table exists..." +psql_exec -c "CREATE TABLE IF NOT EXISTS schema_migrations (version TEXT PRIMARY KEY, applied_at TIMESTAMPTZ NOT NULL DEFAULT now())" + +applied="$(psql_exec -t -A -c "SELECT version FROM schema_migrations")" + +shopt -s nullglob +for file in "${MIGRATIONS_DIR}"/*.sql; do + version="$(basename "$file")" + if grep -qx "$version" <<< "$applied"; then + echo "skip ${version} (already applied)" + continue + fi + echo "apply ${version}" + psql_exec -f "$file" + psql_exec -c "INSERT INTO schema_migrations (version) VALUES ('${version}')" +done + +echo "Migrations complete." diff --git a/metadata/migrations/0001_create_dashboards.sql b/metadata/migrations/0001_create_dashboards.sql new file mode 100644 index 0000000..6a0d687 --- /dev/null +++ b/metadata/migrations/0001_create_dashboards.sql @@ -0,0 +1,12 @@ +CREATE TABLE IF NOT EXISTS dashboards +( + id UUID PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + default_earliest TEXT NOT NULL DEFAULT '-1h', + default_latest TEXT NOT NULL DEFAULT 'now', + created_by TEXT NOT NULL DEFAULT 'anonymous', + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +) diff --git a/metadata/migrations/0002_create_dashboard_panels.sql b/metadata/migrations/0002_create_dashboard_panels.sql new file mode 100644 index 0000000..9e22a73 --- /dev/null +++ b/metadata/migrations/0002_create_dashboard_panels.sql @@ -0,0 +1,19 @@ +CREATE TABLE IF NOT EXISTS dashboard_panels +( + id UUID PRIMARY KEY, + dashboard_id UUID NOT NULL REFERENCES dashboards(id) ON DELETE CASCADE, + title TEXT NOT NULL DEFAULT '', + query TEXT NOT NULL, + query_language TEXT NOT NULL DEFAULT '', + viz_type TEXT NOT NULL CHECK (viz_type IN ('table', 'line', 'bar', 'single_stat', 'top_n')), + viz_config JSONB NOT NULL DEFAULT '{}', + position_x INT NOT NULL, + position_y INT NOT NULL, + width INT NOT NULL, + height INT NOT NULL, + earliest_override TEXT, + latest_override TEXT, + sort_order INT NOT NULL DEFAULT 0, + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +) diff --git a/metadata/migrations/0003_create_notification_targets.sql b/metadata/migrations/0003_create_notification_targets.sql new file mode 100644 index 0000000..21bd365 --- /dev/null +++ b/metadata/migrations/0003_create_notification_targets.sql @@ -0,0 +1,14 @@ +CREATE TABLE IF NOT EXISTS notification_targets +( + id UUID PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + kind TEXT NOT NULL CHECK (kind IN ('webhook', 'slack', 'pagerduty')), + webhook_url TEXT NOT NULL, + payload_template TEXT, + headers JSONB NOT NULL DEFAULT '{}', + secret TEXT, + created_by TEXT NOT NULL DEFAULT 'anonymous', + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +) diff --git a/metadata/migrations/0004_create_alert_rules.sql b/metadata/migrations/0004_create_alert_rules.sql new file mode 100644 index 0000000..7c29a10 --- /dev/null +++ b/metadata/migrations/0004_create_alert_rules.sql @@ -0,0 +1,20 @@ +CREATE TABLE IF NOT EXISTS alert_rules +( + id UUID PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT 'default', + name TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + query TEXT NOT NULL, + query_language TEXT NOT NULL DEFAULT '', + condition_type TEXT NOT NULL CHECK (condition_type IN ('threshold', 'absence')), + comparator TEXT CHECK (comparator IN ('gt', 'gte', 'lt', 'lte', 'eq', 'ne')), + threshold_value DOUBLE PRECISION, + eval_interval_seconds INT NOT NULL CHECK (eval_interval_seconds >= 30), + for_minutes INT NOT NULL DEFAULT 0, + renotify_interval_minutes INT, + notification_target_id UUID NOT NULL REFERENCES notification_targets(id), + enabled BOOLEAN NOT NULL DEFAULT true, + created_by TEXT NOT NULL DEFAULT 'anonymous', + created_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +) diff --git a/metadata/migrations/0005_create_alert_state.sql b/metadata/migrations/0005_create_alert_state.sql new file mode 100644 index 0000000..55d3692 --- /dev/null +++ b/metadata/migrations/0005_create_alert_state.sql @@ -0,0 +1,15 @@ +CREATE TABLE IF NOT EXISTS alert_state +( + rule_id UUID PRIMARY KEY REFERENCES alert_rules(id) ON DELETE CASCADE, + state TEXT NOT NULL DEFAULT 'ok' CHECK (state IN ('ok', 'pending', 'firing')), + condition_true_since TIMESTAMPTZ, + fired_at TIMESTAMPTZ, + last_notified_at TIMESTAMPTZ, + last_evaluated_at TIMESTAMPTZ, + last_eval_status TEXT NOT NULL DEFAULT 'ok' CHECK (last_eval_status IN ('ok', 'error')), + last_error TEXT, + last_value DOUBLE PRECISION, + consecutive_errors INT NOT NULL DEFAULT 0, + next_eval_at TIMESTAMPTZ NOT NULL, + claimed_at TIMESTAMPTZ +) diff --git a/metadata/migrations/0006_add_alert_state_next_eval_at_index.sql b/metadata/migrations/0006_add_alert_state_next_eval_at_index.sql new file mode 100644 index 0000000..c1370ed --- /dev/null +++ b/metadata/migrations/0006_add_alert_state_next_eval_at_index.sql @@ -0,0 +1 @@ +CREATE INDEX IF NOT EXISTS alert_state_next_eval_at_idx ON alert_state (next_eval_at) diff --git a/metadata/migrations/0007_create_delivery_log.sql b/metadata/migrations/0007_create_delivery_log.sql new file mode 100644 index 0000000..767c2f0 --- /dev/null +++ b/metadata/migrations/0007_create_delivery_log.sql @@ -0,0 +1,16 @@ +CREATE TABLE IF NOT EXISTS delivery_log +( + id BIGSERIAL PRIMARY KEY, + rule_id UUID NOT NULL REFERENCES alert_rules(id) ON DELETE CASCADE, + notification_target_id UUID NOT NULL REFERENCES notification_targets(id), + event_type TEXT NOT NULL CHECK (event_type IN ('firing', 'resolved')), + status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending', 'sent', 'failed', 'retrying')), + attempt_count INT NOT NULL DEFAULT 0, + max_attempts INT NOT NULL DEFAULT 5, + next_attempt_at TIMESTAMPTZ, + last_attempt_at TIMESTAMPTZ, + last_error TEXT, + response_status INT, + payload JSONB NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT now() +) diff --git a/metadata/migrations/0008_add_delivery_log_rule_id_index.sql b/metadata/migrations/0008_add_delivery_log_rule_id_index.sql new file mode 100644 index 0000000..6dc922b --- /dev/null +++ b/metadata/migrations/0008_add_delivery_log_rule_id_index.sql @@ -0,0 +1 @@ +CREATE INDEX IF NOT EXISTS delivery_log_rule_id_created_at_idx ON delivery_log (rule_id, created_at DESC) diff --git a/metadata/migrations/0009_add_delivery_log_claim_index.sql b/metadata/migrations/0009_add_delivery_log_claim_index.sql new file mode 100644 index 0000000..e1347e2 --- /dev/null +++ b/metadata/migrations/0009_add_delivery_log_claim_index.sql @@ -0,0 +1,2 @@ +CREATE INDEX IF NOT EXISTS delivery_log_claim_idx ON delivery_log (status, next_attempt_at) + WHERE status IN ('pending', 'retrying') diff --git a/web/nginx.conf b/web/nginx.conf index b0c7699..b72f170 100644 --- a/web/nginx.conf +++ b/web/nginx.conf @@ -4,6 +4,15 @@ server { index index.html; location / { - try_files $uri $uri/ /index.html; + # adapter-static writes prerendered routes as flat .html + # files (e.g. /dashboards -> dashboards.html, confirmed by + # actually inspecting the build output), not /index.html -- + # $uri.html has to be in this chain or a request for exactly + # "/dashboards" falls straight through to the SPA fallback and + # skips the prerendered page it should be serving. 200.html (not + # index.html) is the fallback shell for genuinely dynamic routes + # (dashboards/[id] etc.) -- named differently so it doesn't + # collide with "/" -> index.html, which really is prerendered. + try_files $uri $uri.html $uri/ /200.html; } } diff --git a/web/package-lock.json b/web/package-lock.json index 1015e62..5f87202 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -7,6 +7,10 @@ "": { "name": "web", "version": "0.0.1", + "dependencies": { + "gridstack": "^11.5.0", + "uplot": "^1.6.32" + }, "devDependencies": { "@sveltejs/adapter-static": "^3.0.10", "@sveltejs/kit": "^2.63.0", @@ -631,6 +635,22 @@ "node": "^8.16.0 || ^10.6.0 || >=11.0.0" } }, + "node_modules/gridstack": { + "version": "11.5.1", + "resolved": "https://registry.npmjs.org/gridstack/-/gridstack-11.5.1.tgz", + "integrity": "sha512-qgbH65F6TtyKyi9t6fCkrxLhiobgYR3RBjnK0AzZl+YO7hreMVlsZ1MFbkPV0+7ZhjXdvcaRSZp3UA1yAJqYBQ==", + "funding": [ + { + "type": "paypal", + "url": "https://www.paypal.me/alaind831" + }, + { + "type": "venmo", + "url": "https://www.venmo.com/adumesny" + } + ], + "license": "MIT" + }, "node_modules/is-reference": { "version": "3.0.3", "resolved": "https://registry.npmjs.org/is-reference/-/is-reference-3.0.3.tgz", @@ -1229,6 +1249,12 @@ "node": ">=14.17" } }, + "node_modules/uplot": { + "version": "1.6.32", + "resolved": "https://registry.npmjs.org/uplot/-/uplot-1.6.32.tgz", + "integrity": "sha512-KIMVnG68zvu5XXUbC4LQEPnhwOxBuLyW1AHtpm6IKTXImkbLgkMy+jabjLgSLMasNuGGzQm/ep3tOkyTxpiQIw==", + "license": "MIT" + }, "node_modules/vite": { "version": "8.2.1", "resolved": "https://registry.npmjs.org/vite/-/vite-8.2.1.tgz", diff --git a/web/package.json b/web/package.json index 24dac41..7ffea7c 100644 --- a/web/package.json +++ b/web/package.json @@ -19,5 +19,9 @@ "svelte-check": "^4.6.0", "typescript": "^6.0.3", "vite": "^8.0.16" + }, + "dependencies": { + "gridstack": "^11.5.0", + "uplot": "^1.6.32" } } diff --git a/web/src/lib/PanelViz.svelte b/web/src/lib/PanelViz.svelte new file mode 100644 index 0000000..ee33ef4 --- /dev/null +++ b/web/src/lib/PanelViz.svelte @@ -0,0 +1,147 @@ + + +{#if vizType === 'table' || vizType === 'top_n'} + +{:else if vizType === 'single_stat'} +
{result.rows[0]?.[0] ?? '—'}
+{:else} +
+{/if} + + diff --git a/web/src/lib/QueryBar.svelte b/web/src/lib/QueryBar.svelte new file mode 100644 index 0000000..6bb140f --- /dev/null +++ b/web/src/lib/QueryBar.svelte @@ -0,0 +1,93 @@ + + +
+ +
+ + + {effectiveLanguage === 'sql' ? 'SQL' : 'pipe syntax'} + + + ⌘/Ctrl+Enter to run +
+
+ + diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts new file mode 100644 index 0000000..f84ad5f --- /dev/null +++ b/web/src/lib/api.ts @@ -0,0 +1,242 @@ +// First real API-client module -- previously each route did its own +// inline fetch(). Introduced in Phase 3 because the surface triples +// (query + dashboards + panels + export/import); still zero-dependency, +// a thin fetch wrapper, not a generated client. + +export const apiBase = import.meta.env.VITE_API_BASE_URL ?? 'http://localhost:8080'; +export const alertingBase = import.meta.env.VITE_ALERTING_API_BASE_URL ?? 'http://localhost:8081'; + +export type Language = '' | 'sql' | 'spl'; + +export type QueryResult = { columns: string[]; rows: unknown[][] }; + +export type VizType = 'table' | 'line' | 'bar' | 'single_stat' | 'top_n'; + +export type Panel = { + id: string; + dashboard_id: string; + title: string; + query: string; + query_language: Language; + viz_type: VizType; + viz_config: Record; + position_x: number; + position_y: number; + width: number; + height: number; + earliest_override: string | null; + latest_override: string | null; + sort_order: number; +}; + +export type Dashboard = { + id: string; + name: string; + description: string; + default_earliest: string; + default_latest: string; + created_at: string; + updated_at: string; + panels: Panel[] | null; +}; + +class ApiError extends Error {} + +async function requestFrom(base: string, path: string, init?: RequestInit): Promise { + const res = await fetch(`${base}${path}`, { + headers: { 'Content-Type': 'application/json' }, + ...init + }); + if (!res.ok) { + let message = `request failed with status ${res.status}`; + try { + const body = await res.json(); + if (body?.error) message = body.error; + } catch { + // non-JSON error body -- keep the generic message + } + throw new ApiError(message); + } + if (res.status === 204) return undefined as T; + return res.json(); +} + +function request(path: string, init?: RequestInit): Promise { + return requestFrom(apiBase, path, init); +} + +// alerting is a separate service (its own base URL) -- see +// /docs/phase-3-alerting-design.md's component boundary. +function alertingRequest(path: string, init?: RequestInit): Promise { + return requestFrom(alertingBase, path, init); +} + +export function runQuery(query: string, language: Language): Promise { + return request('/query', { method: 'POST', body: JSON.stringify({ query, language }) }); +} + +export function listDashboards(): Promise { + return request('/dashboards').then((d) => (d as Dashboard[]) ?? []); +} + +export function getDashboard(id: string): Promise { + return request(`/dashboards/${id}`); +} + +export function createDashboard(input: { + name: string; + description?: string; + default_earliest?: string; + default_latest?: string; +}): Promise { + return request('/dashboards', { method: 'POST', body: JSON.stringify(input) }); +} + +export function updateDashboard( + id: string, + input: { name: string; description?: string; default_earliest?: string; default_latest?: string } +): Promise { + return request(`/dashboards/${id}`, { method: 'PUT', body: JSON.stringify(input) }); +} + +export function deleteDashboard(id: string): Promise { + return request(`/dashboards/${id}`, { method: 'DELETE' }); +} + +export function addPanel(dashboardId: string, panel: Partial): Promise { + return request(`/dashboards/${dashboardId}/panels`, { method: 'POST', body: JSON.stringify(panel) }); +} + +export function updatePanel(dashboardId: string, panel: Partial): Promise { + return request(`/dashboards/${dashboardId}/panels/${panel.id}`, { + method: 'PUT', + body: JSON.stringify(panel) + }); +} + +export function deletePanel(dashboardId: string, panelId: string): Promise { + return request(`/dashboards/${dashboardId}/panels/${panelId}`, { method: 'DELETE' }); +} + +export function exportDashboard(id: string): Promise { + return request(`/dashboards/${id}/export`); +} + +export function importDashboard(dashboard: Dashboard): Promise { + return request('/dashboards/import', { method: 'POST', body: JSON.stringify(dashboard) }); +} + +// resolveTimeRange applies the override-or-default rule from +// /docs/phase-3-dashboard-design.md's "Time-range mechanics": a panel's +// own earliest/latest override wins if set, otherwise the dashboard's +// default applies. +export function resolveTimeRange( + dashboard: Pick, + panel: Pick +): { earliest: string; latest: string } { + return { + earliest: panel.earliest_override ?? dashboard.default_earliest, + latest: panel.latest_override ?? dashboard.default_latest + }; +} + +// injectTimeRange prepends earliest=/latest= as leading base_search +// terms -- works because they're ordinary implicit-AND terms in Phase +// 2's grammar, order-independent. Never used for raw-SQL panels (the +// dashboards API rejects query_language: "sql" on panels entirely, so +// this never has to handle that case). +// +// "now" is a UI-only sentinel (the default_latest value shown in the +// time-range picker), not a token the query language understands -- +// time_expr only accepts a quoted absolute timestamp or a "-N unit" +// relative offset (see /docs/query-language-design.md). Emitting a +// literal `latest=now` produces a real compile error ("expected a +// quoted absolute timestamp or a relative offset"), caught by actually +// running this against the live stack. Omitting the latest= clause +// entirely is the query language's own way of saying "no upper bound", +// which is exactly what "now" means here. +export function injectTimeRange(query: string, earliest: string, latest: string): string { + const clauses = [`earliest=${earliest}`]; + if (latest && latest !== 'now') clauses.push(`latest=${latest}`); + return `${clauses.join(' ')} ${query}`; +} + +// --- alerting --------------------------------------------------------- + +export type ConditionType = 'threshold' | 'absence'; +export type Comparator = 'gt' | 'gte' | 'lt' | 'lte' | 'eq' | 'ne'; +export type NotificationKind = 'webhook' | 'slack' | 'pagerduty'; +export type AlertRuleState = 'ok' | 'pending' | 'firing'; + +export type NotificationTarget = { + id: string; + name: string; + kind: NotificationKind; + webhook_url: string; +}; + +export type AlertRule = { + id: string; + name: string; + description: string; + query: string; + query_language: Language; + condition_type: ConditionType; + comparator?: Comparator; + threshold_value?: number; + eval_interval_seconds: number; + for_minutes: number; + renotify_interval_minutes?: number; + notification_target_id: string; + enabled: boolean; + state: { + state: AlertRuleState; + last_evaluated_at?: string; + last_eval_status: 'ok' | 'error'; + last_error?: string; + last_value?: number; + consecutive_errors: number; + }; +}; + +export type DeliveryLogEntry = { + id: number; + event_type: 'firing' | 'resolved'; + status: 'pending' | 'sent' | 'failed' | 'retrying'; + attempt_count: number; + last_error?: string; + response_status?: number; + created_at: string; +}; + +export function listRules(): Promise { + return alertingRequest('/rules').then((r) => r ?? []); +} + +export function getRule(id: string): Promise { + return alertingRequest(`/rules/${id}`); +} + +export function createRule(input: Partial): Promise { + return alertingRequest('/rules', { method: 'POST', body: JSON.stringify(input) }); +} + +export function deleteRule(id: string): Promise { + return alertingRequest(`/rules/${id}`, { method: 'DELETE' }); +} + +export function listDeliveries(ruleId: string): Promise { + return alertingRequest(`/rules/${ruleId}/deliveries`).then((d) => d ?? []); +} + +export function listNotificationTargets(): Promise { + return alertingRequest('/targets').then((t) => t ?? []); +} + +export function createNotificationTarget(input: { + name: string; + kind: NotificationKind; + webhook_url: string; +}): Promise { + return alertingRequest('/targets', { method: 'POST', body: JSON.stringify(input) }); +} diff --git a/web/src/routes/+layout.svelte b/web/src/routes/+layout.svelte index 8098bbb..685660a 100644 --- a/web/src/routes/+layout.svelte +++ b/web/src/routes/+layout.svelte @@ -1,6 +1,9 @@ @@ -9,4 +12,29 @@ + + {@render children()} + + diff --git a/web/src/routes/+page.svelte b/web/src/routes/+page.svelte index aa1488b..4f7188c 100644 --- a/web/src/routes/+page.svelte +++ b/web/src/routes/+page.svelte @@ -2,12 +2,14 @@ // Phase 2: single unified query page. Replaces Phase 0/1's two // separate pages (raw-SQL-only /query, free-text-only /search) -- // see /docs/query-language-design.md and /docs/query-language-reference.md. + // Phase 3: query bar extracted into $lib/QueryBar.svelte (reused by + // the dashboard panel editor and alert rule editor), fetch calls + // moved into $lib/api.ts. import ResultsTable from '$lib/ResultsTable.svelte'; + import QueryBar from '$lib/QueryBar.svelte'; + import { runQuery as apiRunQuery, type Language } from '$lib/api'; - const apiBase = import.meta.env.VITE_API_BASE_URL ?? 'http://localhost:8080'; - - type Language = '' | 'sql' | 'spl'; type HistoryEntry = { query: string; language: Language; at: number }; const HISTORY_KEY = 'sentry.queryHistory'; @@ -22,16 +24,6 @@ let hasRun = $state(false); let history = $state(loadHistory()); - // Client-side mirror of the backend's auto-detect heuristic - // (api/internal/querylang/planner.looksLikeSQL) -- purely a UI hint, - // the server does its own detection independently and is the - // authority on what actually runs. - function detectedLanguage(q: string): 'sql' | 'spl' { - return /^\s*select\b/i.test(q) ? 'sql' : 'spl'; - } - let detected = $derived(detectedLanguage(query)); - let effectiveLanguage = $derived(language === '' ? detected : language); - function loadHistory(): HistoryEntry[] { if (typeof sessionStorage === 'undefined') return []; try { @@ -61,20 +53,9 @@ loading = true; error = ''; try { - const res = await fetch(`${apiBase}/query`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ query, language }) - }); - const body = await res.json(); - if (!res.ok) { - error = body?.error ?? `request failed with status ${res.status}`; - columns = []; - rows = []; - return; - } - columns = body.columns ?? []; - rows = body.rows ?? []; + const result = await apiRunQuery(query, language); + columns = result.columns ?? []; + rows = result.rows ?? []; saveHistory({ query, language, at: Date.now() }); } catch (e) { error = e instanceof Error ? e.message : String(e); @@ -85,16 +66,6 @@ hasRun = true; } } - - function onKeydown(e: KeyboardEvent) { - // Cmd/Ctrl+Enter runs the query -- textarea's own Enter key needs - // to stay newline-for-pipe-stage-formatting, so this isn't a bare - // Enter binding. - if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { - e.preventDefault(); - runQuery(); - } - }
@@ -102,35 +73,10 @@

One query bar for both filter/stats queries and free-text search — see /docs/query-language-reference.md in the repo for the full syntax, or the cheat - sheet below. + sheet below. Build reusable queries into a dashboard.

- - -
- - - {effectiveLanguage === 'sql' ? 'SQL' : 'pipe syntax'} - - - ⌘/Ctrl+Enter to run -
+ {#if error}

Error: {error}

@@ -179,34 +125,6 @@ margin: 2rem auto; padding: 0 1rem; } - textarea { - width: 100%; - font-family: monospace; - font-size: 0.9rem; - box-sizing: border-box; - } - .controls { - margin-top: 0.5rem; - display: flex; - align-items: center; - gap: 0.75rem; - flex-wrap: wrap; - } - .detected-badge { - font-size: 0.75rem; - padding: 0.15rem 0.5rem; - border-radius: 1rem; - background: #eef; - color: #224; - } - .detected-badge.sql { - background: #fee; - color: #422; - } - .hint { - font-size: 0.8rem; - color: #777; - } .error { color: #b00020; } diff --git a/web/src/routes/alerts/+page.svelte b/web/src/routes/alerts/+page.svelte new file mode 100644 index 0000000..b1f5798 --- /dev/null +++ b/web/src/routes/alerts/+page.svelte @@ -0,0 +1,132 @@ + + +
+

Alerts

+ {#if error}

Error: {error}

{/if} + + + New rule + + {#if loading} +

Loading…

+ {:else if rules.length === 0} +

No alert rules yet.

+ {:else} + + + + + + + + + + + + {#each rules as r (r.id)} + + + + + + + + {/each} + +
NameConditionStateEnabled
{r.name}{conditionSummary(r)} + + {r.state.state} + + {#if r.state.last_eval_status === 'error'} + eval error + {/if} + {r.enabled ? 'yes' : 'no'}
+ {/if} +
+ + diff --git a/web/src/routes/alerts/+page.ts b/web/src/routes/alerts/+page.ts new file mode 100644 index 0000000..4476454 --- /dev/null +++ b/web/src/routes/alerts/+page.ts @@ -0,0 +1,3 @@ +// No route params, data comes from a client-side fetch -- same shape as +// the dashboards list page's +page.ts. +export const prerender = true; diff --git a/web/src/routes/alerts/[id]/+page.svelte b/web/src/routes/alerts/[id]/+page.svelte new file mode 100644 index 0000000..3b997b2 --- /dev/null +++ b/web/src/routes/alerts/[id]/+page.svelte @@ -0,0 +1,188 @@ + + +
+ {#if loading} +

Loading…

+ {:else if !rule} +

Error: {error}

+ {:else} +
+

{rule.name}

+ +
+ {#if rule.description}

{rule.description}

{/if} + {#if error}

Error: {error}

{/if} + +
+
+ State + + {rule.state.state} + +
+
Condition {rule.condition_type} — {conditionSummary(rule)}
+
Query {rule.query}
+
Evaluation interval {rule.eval_interval_seconds}s
+
Debounce (for) {rule.for_minutes}m
+
Enabled {rule.enabled ? 'yes' : 'no'}
+ {#if rule.state.last_eval_status === 'error'} +
+ Last evaluation error {rule.state.last_error} + ({rule.state.consecutive_errors} consecutive) +
+ {:else if rule.state.last_value !== undefined} +
Last observed value {rule.state.last_value}
+ {/if} +
+ +

Delivery log

+

Most recent first — this is "why didn't I get paged."

+ {#if deliveries.length === 0} +

No deliveries yet.

+ {:else} + + + + + + + + + + + + + {#each deliveries as d (d.id)} + + + + + + + + + {/each} + +
WhenEventStatusAttemptsResponseError
{new Date(d.created_at).toLocaleString()}{d.event_type}{d.status}{d.attempt_count}{d.response_status ?? '—'}{d.last_error ?? ''}
+ {/if} + {/if} +
+ + diff --git a/web/src/routes/alerts/[id]/+page.ts b/web/src/routes/alerts/[id]/+page.ts new file mode 100644 index 0000000..33bfef0 --- /dev/null +++ b/web/src/routes/alerts/[id]/+page.ts @@ -0,0 +1,4 @@ +// The rule ID param doesn't exist at build time -- same dynamic-route +// shape as dashboards/[id], served from adapter-static's fallback shell. +export const prerender = false; +export const ssr = false; diff --git a/web/src/routes/alerts/new/+page.svelte b/web/src/routes/alerts/new/+page.svelte new file mode 100644 index 0000000..e573ae5 --- /dev/null +++ b/web/src/routes/alerts/new/+page.svelte @@ -0,0 +1,241 @@ + + +
+

New alert rule

+ {#if error}

Error: {error}

{/if} + + + + + {}} placeholder="service=api | where status>=500 | stats count" /> +

+ For threshold rules, the query must resolve to exactly one row (e.g. + | stats count). For absence rules, the query's own earliest= + defines the window being checked for zero results. +

+ +
+ + {#if conditionType === 'threshold'} + + + {/if} +
+ +
+ + + +
+ +
+ + +
+ + {#if showNewTarget} +
+ + + + +
+ {/if} + + +
+ + diff --git a/web/src/routes/alerts/new/+page.ts b/web/src/routes/alerts/new/+page.ts new file mode 100644 index 0000000..57ef5b1 --- /dev/null +++ b/web/src/routes/alerts/new/+page.ts @@ -0,0 +1,4 @@ +// A static path segment ("new"), not a dynamic route -- SvelteKit +// resolves this before matching /alerts/[id], so "new" never collides +// with a rule ID lookup. No route params, prerenderable like the list page. +export const prerender = true; diff --git a/web/src/routes/dashboards/+page.svelte b/web/src/routes/dashboards/+page.svelte new file mode 100644 index 0000000..e89f6b0 --- /dev/null +++ b/web/src/routes/dashboards/+page.svelte @@ -0,0 +1,147 @@ + + +
+

Dashboards

+ {#if error}

Error: {error}

{/if} + +
+ e.key === 'Enter' && create()} + /> + + +
+ + {#if loading} +

Loading…

+ {:else if dashboards.length === 0} +

No dashboards yet.

+ {:else} +
    + {#each dashboards as d (d.id)} +
  • + {d.name} + {#if d.description}{d.description}{/if} + +
    • + {/each} +
+ {/if} +
+ + diff --git a/web/src/routes/dashboards/+page.ts b/web/src/routes/dashboards/+page.ts new file mode 100644 index 0000000..7e45e0b --- /dev/null +++ b/web/src/routes/dashboards/+page.ts @@ -0,0 +1,3 @@ +// No route params, data comes from a client-side fetch -- same shape as +// the root query page's +page.ts. +export const prerender = true; diff --git a/web/src/routes/dashboards/[id]/+page.svelte b/web/src/routes/dashboards/[id]/+page.svelte new file mode 100644 index 0000000..55a521c --- /dev/null +++ b/web/src/routes/dashboards/[id]/+page.svelte @@ -0,0 +1,362 @@ + + +
+ {#if loading} +

Loading…

+ {:else if !dashboard} +

Error: {error}

+ {:else} +
+

{dashboard.name}

+
+ + +
+
+ {#if dashboard.description}

{dashboard.description}

{/if} + {#if error}

Error: {error}

{/if} + +
+ + + + Per-panel overrides win over this default -- see the panel editor. +
+ + {#if dashboard.panels && dashboard.panels.length > 0} +
+ {#each dashboard.panels as panel (panel.id)} +
+
+
+ {panel.title || panel.query} + +
+ {#if panelErrors[panel.id]} +

Error: {panelErrors[panel.id]}

+ {:else if panelResults[panel.id]} + + {:else} +

Loading…

+ {/if} +
+
+ {/each} +
+ {:else} +

No panels yet. Add one below.

+ {/if} + +
+ + {#if showAddPanel} +
+ + + + +
+ {/if} +
+ {/if} +
+ + diff --git a/web/src/routes/dashboards/[id]/+page.ts b/web/src/routes/dashboards/[id]/+page.ts new file mode 100644 index 0000000..88414fe --- /dev/null +++ b/web/src/routes/dashboards/[id]/+page.ts @@ -0,0 +1,5 @@ +// The dashboard ID param doesn't exist at build time, so this route can't +// be prerendered like the list page -- served from adapter-static's +// fallback shell (see vite.config.ts) and rendered fully client-side. +export const prerender = false; +export const ssr = false; diff --git a/web/vite.config.ts b/web/vite.config.ts index 66f7358..c8867bb 100644 --- a/web/vite.config.ts +++ b/web/vite.config.ts @@ -9,7 +9,16 @@ export default defineConfig({ // Force runes mode for the project, except for libraries. Can be removed in svelte 6. runes: ({ filename }) => filename.split(/[/\\]/).includes('node_modules') ? undefined : true }, - adapter: adapter() + // fallback: dashboards/[id] and alerts/[id] are dynamic routes + // whose params (dashboard/rule IDs) don't exist at build time, so + // they can't be prerendered like the root/list pages. Named + // 200.html, not index.html -- the root route ("/") is itself + // prerendered to index.html, and a same-named fallback silently + // overwrites it (confirmed by actually running the build: "Using + // index.html" produced the warning "Overwriting build/index.html + // with fallback page"). nginx.conf's try_files points at + // /200.html to match. + adapter: adapter({ fallback: '200.html' }) }) ] });