diff --git a/agent/README.md b/agent/README.md index 0f1d189..6a7c980 100644 --- a/agent/README.md +++ b/agent/README.md @@ -135,6 +135,24 @@ See `config/agent.example.toml` for all fields. ./sentry-agent --config /path/to/agent.toml ``` +## Heartbeat and unavailability alerting + +Every agent sends a small, independent "still alive" record on its own +schedule (`[heartbeat]` in the config, default every 60s), separate from +whatever real log traffic is flowing — see `config/agent.example.toml`. +This isn't a new wire protocol: it's an ordinary record through the same +`PushBatch` RPC and mTLS identity every log line uses, tagged with a +`sentry.heartbeat=true` attribute so it's easy to filter for and doesn't +show up as noise in normal log views. Set `interval` to a plain number +plus `s`/`m`/`h` (matches the query language's own `earliest=`/`latest=` +units); `enabled = false` turns it off entirely. + +The platform has no separate "agent status" concept — an agent going +quiet is just the absence of its heartbeat records, which the existing +alerting engine already detects natively via an `absence`-condition +alert rule. See `/docs/agent-heartbeat-monitoring.md` for the exact rule +to create. + ## Running as a Windows service "A native Windows service, not a WSL wrapper" means implementing the Win32 diff --git a/agent/sentry-agent/config/agent.example.toml b/agent/sentry-agent/config/agent.example.toml index 6331ce5..66632aa 100644 --- a/agent/sentry-agent/config/agent.example.toml +++ b/agent/sentry-agent/config/agent.example.toml @@ -38,6 +38,19 @@ kind = "journald" max_size = 500 flush_interval_ms = 2000 +[heartbeat] +# How often this agent proves it's still alive to the platform, sent as +# its own record independent of whatever real log traffic is flowing -- +# pair with an "absence" alert rule on the sentry.heartbeat attribute to +# get paged when a host goes quiet. Accepts a plain number + unit: s +# (seconds), m (minutes), or h (hours) -- same vocabulary as +# earliest=/latest= in the query language. See +# /docs/agent-heartbeat-monitoring.md. +enabled = true +interval = "60s" +# interval = "5m" +# interval = "1h" + [ingest] endpoint = "https://ingest.internal:4317" diff --git a/agent/sentry-agent/src/config.rs b/agent/sentry-agent/src/config.rs index bd291ef..3ea7d76 100644 --- a/agent/sentry-agent/src/config.rs +++ b/agent/sentry-agent/src/config.rs @@ -1,6 +1,7 @@ use anyhow::{Context, Result}; -use serde::Deserialize; +use serde::{Deserialize, Deserializer}; use std::path::{Path, PathBuf}; +use std::time::Duration; #[cfg(not(windows))] const DEFAULT_CONFIG_PATH: &str = "/etc/sentry-agent/agent.toml"; @@ -13,6 +14,7 @@ pub struct Config { pub agent: AgentConfig, pub source: SourceConfig, pub batch: BatchConfig, + pub heartbeat: HeartbeatConfig, pub ingest: IngestConfig, pub tls: TlsConfig, } @@ -141,6 +143,100 @@ impl Default for BatchConfig { } } +/// Sent independently of `batch` -- a heartbeat is a punctual liveness +/// signal, not log data, so it bypasses `Batcher` entirely (see +/// main.rs's `send_heartbeat`) rather than waiting on `max_size`/ +/// `flush_interval_ms` like real records do. This is the operator-facing +/// "polling resolution" knob: how often this agent proves it's still +/// alive, which a `condition_type = "absence"` alert rule on the +/// `sentry.heartbeat` attribute (see /docs/agent-heartbeat-monitoring.md) +/// turns into "alert when this host goes quiet." +#[derive(Debug, Clone, Deserialize)] +#[serde(default)] +pub struct HeartbeatConfig { + pub enabled: bool, + #[serde(deserialize_with = "deserialize_duration")] + pub interval: Duration, +} + +impl Default for HeartbeatConfig { + fn default() -> Self { + Self { + enabled: true, + interval: Duration::from_secs(60), + } + } +} + +/// Parses a human-friendly duration string with an explicit unit suffix +/// -- "30s", "5m", "1h" -- deliberately the same s/m/h vocabulary +/// `earliest=`/`latest=` use in the query language +/// (/docs/query-language-reference.md), so the interval you set here and +/// the window you write in the matching alert rule's query read the same +/// way. Kept as a small hand-rolled parser rather than pulling in a +/// duration-parsing crate for this one field -- this is the +/// statically-linked edge agent every "no glibc runtime deps" constraint +/// in CLAUDE.md is about keeping lean, and the grammar needed here is a +/// handful of lines. +fn deserialize_duration<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + let s = String::deserialize(deserializer)?; + parse_duration(&s).map_err(serde::de::Error::custom) +} + +fn parse_duration(s: &str) -> Result { + let s = s.trim(); + let (num, unit) = s.split_at(s.len().saturating_sub(1)); + let n: u64 = num + .parse() + .map_err(|_| format!("expected a duration like \"30s\", \"5m\", or \"1h\", got {s:?}"))?; + match unit { + "s" => Ok(Duration::from_secs(n)), + "m" => Ok(Duration::from_secs(n * 60)), + "h" => Ok(Duration::from_secs(n * 3600)), + _ => Err(format!("expected a time unit of s, m, or h after {n}, got {s:?}")), + } +} + +#[cfg(test)] +mod heartbeat_config_tests { + use super::*; + + #[test] + fn parses_seconds_minutes_hours() { + assert_eq!(parse_duration("30s").unwrap(), Duration::from_secs(30)); + assert_eq!(parse_duration("5m").unwrap(), Duration::from_secs(300)); + assert_eq!(parse_duration("2h").unwrap(), Duration::from_secs(7200)); + } + + #[test] + fn rejects_missing_or_unknown_unit() { + assert!(parse_duration("30").is_err()); + assert!(parse_duration("30x").is_err()); + assert!(parse_duration("").is_err()); + } + + #[test] + fn default_is_60_seconds_and_enabled() { + let cfg = HeartbeatConfig::default(); + assert!(cfg.enabled); + assert_eq!(cfg.interval, Duration::from_secs(60)); + } + + #[test] + fn toml_field_parses_via_deserialize() { + #[derive(Deserialize)] + struct Wrapper { + #[serde(default)] + heartbeat: HeartbeatConfig, + } + let w: Wrapper = toml::from_str("[heartbeat]\nenabled = true\ninterval = \"90s\"\n").unwrap(); + assert_eq!(w.heartbeat.interval, Duration::from_secs(90)); + } +} + #[derive(Debug, Clone, Deserialize)] #[serde(default)] pub struct IngestConfig { diff --git a/agent/sentry-agent/src/main.rs b/agent/sentry-agent/src/main.rs index d933e42..b9dd287 100644 --- a/agent/sentry-agent/src/main.rs +++ b/agent/sentry-agent/src/main.rs @@ -97,8 +97,20 @@ pub async fn run_agent(config_path: Option) -> Result<()> { let mut batcher = Batcher::new(cfg.batch.max_size, flush_interval); let mut ticker = tokio::time::interval(flush_interval.max(Duration::from_millis(50))); + // Heartbeat's own ticker, independent of the batch flush ticker above + // -- it always fires on cfg.heartbeat.interval regardless of + // cfg.batch's settings or whether any real log traffic is flowing. + // Built unconditionally even when disabled (tokio::time::interval + // doesn't fail on construction); the `if cfg.heartbeat.enabled` + // select! guard is what actually turns it off, so a disabled + // heartbeat costs nothing beyond one idle timer. + let mut heartbeat_ticker = tokio::time::interval(cfg.heartbeat.interval.max(Duration::from_millis(50))); + loop { tokio::select! { + _ = heartbeat_ticker.tick(), if cfg.heartbeat.enabled => { + send_heartbeat(&mut client, &host, &service).await; + } maybe_line = rx.recv() => { let Some(raw) = maybe_line else { tracing::warn!("source exited, flushing remaining batch and shutting down"); @@ -191,6 +203,40 @@ async fn spawn_source(source: config::SourceConfig, tx: source::LineSender) { } } +/// Sends a single synthetic record through the same `PushBatch` RPC and +/// mTLS identity as real log data -- no new proto message, no new ingest +/// code, no new ClickHouse schema. Bypasses `Batcher` (see the heartbeat +/// ticker's own comment above): a heartbeat that got queued behind +/// `batch.max_size` or `batch.flush_interval_ms` would defeat the point +/// of a punctual "still alive" signal. Distinguished from a real log +/// record purely by the `sentry.heartbeat` attribute -- `service` stays +/// the agent's real configured service so it doesn't pollute +/// service-based dashboards/faceting with a fake value. See +/// /docs/agent-heartbeat-monitoring.md for how an absence alert rule +/// turns a run of missed heartbeats into a notification. +async fn send_heartbeat(client: &mut LogIngestClient, host: &str, service: &str) { + let record = LogRecord { + timestamp_unix_nano: now_unix_nanos(), + host: host.to_string(), + service: service.to_string(), + severity: Severity::Info as i32, + message: "agent heartbeat".to_string(), + attributes: std::collections::HashMap::from([("sentry.heartbeat".to_string(), "true".to_string())]), + record_id: String::new(), + }; + match grpc::send_batch(client, format!("heartbeat-{}", batch_id()), vec![record]).await { + Ok(_) => tracing::debug!(host, "heartbeat sent"), + Err(e) => tracing::warn!(error = %e, host, "heartbeat send failed"), + } +} + +fn now_unix_nanos() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos() as i64) + .unwrap_or(0) +} + async fn flush(client: &mut LogIngestClient, batch: Vec) { let n = batch.len(); let batch_id = batch_id(); diff --git a/api/internal/querylang/lexer/lexer.go b/api/internal/querylang/lexer/lexer.go index b9c9a73..b9db275 100644 --- a/api/internal/querylang/lexer/lexer.go +++ b/api/internal/querylang/lexer/lexer.go @@ -232,7 +232,13 @@ func isIdentStart(c rune) bool { } // Identifiers allow dots (e.g. winevt.event_id, a real attribute key -// shape from Phase 1) and digits after the first character. +// shape from Phase 1), digits, and internal hyphens (e.g. host-03, +// api-service -- real, common bare-word values with no need for +// quoting) after the first character. A leading hyphen is deliberately +// NOT part of isIdentStart -- Minus has to stay its own token there so +// `earliest=-1h` and `sort -count`'s leading sign still lex correctly; +// this only affects a hyphen once a token has already started with a +// real identifier character. func isIdentPart(c rune) bool { - return isIdentStart(c) || isDigit(c) || c == '.' || c == '_' + return isIdentStart(c) || isDigit(c) || c == '.' || c == '_' || c == '-' } diff --git a/api/internal/querylang/lexer/lexer_test.go b/api/internal/querylang/lexer/lexer_test.go index fde00ca..e4a30a6 100644 --- a/api/internal/querylang/lexer/lexer_test.go +++ b/api/internal/querylang/lexer/lexer_test.go @@ -76,6 +76,34 @@ func TestLexFieldWithDots(t *testing.T) { } } +// TestLexIdentWithInternalHyphens is a regression test for a real bug: +// isIdentPart didn't include '-', so a bare (unquoted) hyphenated value +// like host-03 -- the exact example /docs/query-language-reference.md +// itself uses (`host!=host-03`) -- lexed as IDENT("host") MINUS +// IDENT("03") and failed to parse at all. A leading hyphen must still +// lex as its own MINUS token (earliest=-1h, sort -count depend on it) -- +// only an internal hyphen, once a real identifier character has already +// started the token, should be absorbed. +func TestLexIdentWithInternalHyphens(t *testing.T) { + cases := []string{"host-03", "api-service", "heartbeat-test-host", "multi-hyphen-value"} + for _, c := range cases { + l := New(c) + tok := l.Next() + if tok.Kind != Ident || tok.Value != c { + t.Errorf("lexing %q: got %v, want Ident(%s)", c, tok, c) + } + if end := l.Next(); end.Kind != EOF { + t.Errorf("lexing %q: expected EOF after the identifier, got %v", c, end) + } + } +} + +func TestLexFilterWithHyphenatedValue(t *testing.T) { + got := collectKinds(`host!=host-03`) + want := []Kind{Ident, Neq, Ident, EOF} + assertKinds(t, got, want) +} + func TestLexNumber(t *testing.T) { cases := []string{"123", "1.5", "0"} for _, c := range cases { diff --git a/api/internal/querylang/parser/parser_test.go b/api/internal/querylang/parser/parser_test.go index 245343d..4f8dc3b 100644 --- a/api/internal/querylang/parser/parser_test.go +++ b/api/internal/querylang/parser/parser_test.go @@ -26,6 +26,39 @@ func TestParseSimpleFilter(t *testing.T) { } } +// TestParseFilterWithHyphenatedValue is a regression test for a real +// bug in the lexer (isIdentPart excluded '-'): this exact query is +// /docs/query-language-reference.md's own canonical unquoted example +// and used to fail with "unexpected MINUS after query". +func TestParseFilterWithHyphenatedValue(t *testing.T) { + q, err := Parse(`host!=host-03`) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + cmp, ok := q.Base.Terms[0].(ast.Comparison) + if !ok { + t.Fatalf("expected Comparison, got %T", q.Base.Terms[0]) + } + if cmp.Field != "host" || cmp.Op != "!=" || cmp.Value != "host-03" { + t.Fatalf("unexpected comparison: %+v", cmp) + } +} + +// TestParseNegativeTimeExprStillWorks guards against the hyphenated- +// identifier fix above accidentally swallowing the leading sign +// earliest=/latest= depend on -- a leading '-' must stay its own token. +func TestParseNegativeTimeExprStillWorks(t *testing.T) { + q, err := Parse(`earliest=-1h`) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + earliest := q.Base.Terms[0].(ast.TimeBound) + if earliest.Kind != "earliest" || !earliest.Expr.IsRelative || earliest.Expr.RelativeSign != -1 || + earliest.Expr.RelativeN != 1 || earliest.Expr.RelativeUnit != "h" { + t.Fatalf("unexpected earliest: %+v", earliest) + } +} + func TestParseFullPipeline(t *testing.T) { q, err := Parse(`service=api | where status>=500 | stats count(*) as errors by host | sort -errors`) if err != nil { diff --git a/docs/agent-heartbeat-monitoring.md b/docs/agent-heartbeat-monitoring.md new file mode 100644 index 0000000..7f6e77e --- /dev/null +++ b/docs/agent-heartbeat-monitoring.md @@ -0,0 +1,124 @@ +# Agent heartbeat and unavailability alerting + +Every Linux/Windows agent (`/agent`) sends a small, independent "still +alive" record on its own schedule, in addition to whatever real log +traffic is flowing. This is what "define polling resolution in seconds, +minutes, hours" means in practice: how often an agent proves it's still +reachable, and how quickly the platform notices when it stops. + +## Design: why this is a heartbeat, not a true pull + +The agent's transport has always been push-only, by design — it dials +*out* to `ingest` over mTLS; nothing in the platform ever dials into an +agent (see `agent/sentry-agent/src/grpc.rs`'s doc comment). Making +liveness detection a true pull (the platform reaching into every remote +host on a schedule) would mean every agent needs a reachable address and +an open inbound port — a real problem for hosts behind NAT or with +dynamic IPs, which is the common case for a "remote" fleet, and exactly +the class of problem push was chosen to avoid. + +A heartbeat gets the same outcome — the platform notices an agent going +quiet, on a configurable cadence — without any of that: the agent keeps +its one existing egress path, and "unavailable" is just the *absence* of +its heartbeat records, which the alerting engine (Phase 3) already +detects natively via `condition_type: "absence"`. No new RPC, no new +ingest code, no new ClickHouse schema, no new alert rule type — see +`/docs/phase-3-alerting-design.md` for the existing absence-condition +model this reuses unchanged. + +## Configuring the heartbeat + +`agent/sentry-agent/config/agent.example.toml`: + +```toml +[heartbeat] +enabled = true +interval = "60s" # or "5m", "1h" -- same s/m/h vocabulary as earliest=/latest= +``` + +The heartbeat record is sent through the exact same `PushBatch` RPC and +mTLS identity every log line uses, bypassing the batch buffer (`[batch]` +`max_size`/`flush_interval_ms`) so it's punctual rather than subject to +batching delay. It's distinguished from real log data purely by an +attribute — `sentry.heartbeat=true` — not by a fake `service` value, so +it never pollutes service-based dashboards or faceting. `message` is the +literal string `"agent heartbeat"`. + +## Building the alert rule + +There's no dedicated "agent monitor" rule type — you create an ordinary +absence alert rule, scoped to one host, with a query window a little +wider than the heartbeat interval (so an evaluation landing just after a +heartbeat doesn't look like a false absence): + +```sh +curl -X POST http://localhost:8081/rules -H 'Content-Type: application/json' -d '{ + "name": "web-01 unavailable", + "description": "fires when web-01 misses its heartbeat window", + "query": "earliest=-3m host=web-01 sentry.heartbeat=true", + "query_language": "spl", + "condition_type": "absence", + "eval_interval_seconds": 60, + "for_minutes": 0, + "notification_target_id": "", + "enabled": true +}' +``` + +- **`query`**: `earliest=-Ns/m/h` should comfortably exceed the agent's + configured `[heartbeat] interval` — 2-3x it is a reasonable default, + the same margin any liveness check needs against jitter. + `host=` scopes the rule to one specific agent — the + evaluator's absence check only asks "did any row come back," so a + query spanning multiple hosts would only fire when *every* host in it + goes quiet at once, not when one specific host does (this is the same + "no per-group/multi-row alerting" limitation `/docs/phase-3-alerting- + design.md` already documents for threshold rules — one rule per + resource, not a fleet-wide wildcard, is real future work, not an + oversight here). +- **`eval_interval_seconds`**: the alerting-side "polling resolution" — + how often this specific rule is re-checked. Already second-granular + (any multiple of 30, the engine's documented floor — see below); a + value in minutes or hours is just a larger number of seconds, no + separate unit field needed. +- **`for_minutes`**: `0` fires on the very first absent evaluation, no + debounce. A real fleet might prefer `1` or `2` to ride out a single + missed evaluation before paging anyone — the same tradeoff any other + absence rule makes. + +**`eval_interval_seconds` has a real floor of 30**, enforced by +`alerting`'s rule-creation validation (`eval_interval_seconds must be at +least 30`) — a rule can't be checked more often than every 30 seconds +regardless of how fast the agent's own heartbeat is. An agent heartbeat +interval faster than that (this doc's own live verification used 5s) is +still useful — it tightens how quickly *evidence* of an outage +accumulates in the query window — but the alert itself can't fire on a +tighter cadence than 30s. + +## Verified live + +This exact flow was run end-to-end against a live stack in this repo: a +real `sentry-agent` binary, heartbeat interval 5s, connected to a real +`ingest`; an absence rule (`earliest=-45s host=... sentry.heartbeat=true`, +`eval_interval_seconds=30`, `for_minutes=0`) created via the REST API +above; the agent process killed; the rule transitioned `ok` → `firing` +within one evaluation cycle (`condition_true_since`/`fired_at` both set +at the same timestamp the query window's silence became detectable); +and a real webhook delivery landed (`delivery_log` row: `status: sent, +response_status: 200`). + +**A real, independent bug was found and fixed during this +verification**: the query language's lexer never treated `-` as part of +an identifier, so any unquoted hyphenated filter value — +`host=heartbeat-test-host`, or even the reference doc's own canonical +example `host!=host-03` — failed to parse at all +(`unexpected MINUS after query`). This wasn't specific to heartbeat +monitoring; it affected any hyphenated host/service name filtered +unquoted, which is extremely common. Fixed in +`api/internal/querylang/lexer/lexer.go`'s `isIdentPart` (now includes +`-`, but only mid-identifier — a leading `-` still lexes as its own +`Minus` token, so `earliest=-1h` and `sort -count` are unaffected). +Regression tests: `TestLexIdentWithInternalHyphens`, +`TestLexFilterWithHyphenatedValue` (lexer), +`TestParseFilterWithHyphenatedValue`, +`TestParseNegativeTimeExprStillWorks` (parser).