mirror of
https://github.com/block/buzz.git
synced 2026-10-28 04:51:40 +00:00
## Why Database pressure currently collapses several distinct delays into one symptom. This adds the evidence layer needed to distinguish pool acquisition wait, logical database operation time, advisory-lock wait, and selected transaction duration before changing timeout or retry policy. This is the phase 2 Lane A observability bundle for [#26](https://github.com/TheSentinel454/buzz/issues/26), [#28](https://github.com/TheSentinel454/buzz/issues/28), and [#33](https://github.com/TheSentinel454/buzz/issues/33). It is stacked on #6668. ## What - Record explicit reader/writer checkout wait and acquisition outcomes with `buzz_db_pool_acquire_wait_seconds` and `buzz_db_pool_acquisitions_total`. - Extend the compile-time `#[datastore_span(name = "...")]` seam with `buzz_db_operation_duration_seconds`, so operation labels remain static source literals instead of request data. - Route correctness-critical replacement, membership, push-gate, deletion, and migration/schema-safety advisory locks through one observer without changing their SQL, order, scope, or blocking behavior. - Measure six internally owned transaction lifetimes with `buzz_db_transaction_duration_seconds`, starting after `BEGIN` succeeds and ending after explicit commit/rollback or scope exit. - Emit root slow-operation warnings at 500 ms, logging the first slow completion and then 1/100 per call site with only `operation`, `outcome`, and `elapsed_ms`. - Document names, units, fixed label vocabularies, measurement boundaries, and blind spots in this PR description. Fixed labels are deliberately small: - `pool_role`: `writer`, `reader` - `lock_type`: `replacement`, `membership`, `push_gate`, `deletion`, `migration_schema_safety` - `outcome`: `success`, `error`, `timeout` where SQLx/PostgreSQL can distinguish it accurately - `operation`: compile-time datastore names plus the six closed transaction operation names documented in the runbook No metric or slow warning contains community IDs, event IDs, event kinds, coordinates, d-tags, SQL/query text, query IDs, returned errors, or event content. ## Coverage boundaries - Operation duration is the complete annotated logical function body, not pure SQL execution; it may include implicit checkout, lock wait, nested operations, and application work. Cancelled futures do not reach its completion hook. - Pool timing covers explicit helper checkouts, including proved-reader routing and selected writer-owned transactions. Implicit SQLx checkout through `&PgPool` remains folded into operation duration. - Lock timing covers application-side blocking locks in the five named families. Trigger/stored-procedure locks, channel-TTL locking, the usage try-lock, and the audit service session lock remain outside this slice. - Transaction timing covers only the six wholly owned boundaries documented in the runbook. It excludes pool wait, `BEGIN`, asynchronous rollback cleanup after an early return, and caller-owned `Db::begin_transaction` lifetime. ## Relationship to #6229 #6229 is the incident-driven timeout precursor. This PR does not add or change `statement_timeout`, `lock_timeout`, `idle_in_transaction_session_timeout`, retries, audit durability, or client-visible conflicts. It provides the missing distributions needed to evaluate those policies later and intentionally leaves #6229's open audit retry/durability finding untouched. The branches overlap in `crates/buzz-db/src/lib.rs` and `crates/buzz-db/src/migration.rs`, so a later rebase may need textual conflict resolution, but the behavior is complementary rather than duplicated. ## Risk assessment Moderate-low. The primary risk is instrumentation overhead and added static series. Cardinality is source-bounded, slow logs are sampled/redacted root events, and the lock/transaction changes wrap existing awaits without changing policy or ordering. ## Verification Author workstation: `buzz-tornquist-db-pressure-observability` (`2010927`), exact head `d7cf833e26c528adfcde3917ded80daf6f4ddac9`, parent `6f50e6b2b2a996349149af61d35bdd6a355f77fd`. - `cargo fmt --all --check` — passed - `cargo clippy -p buzz-datastore-tracing -p buzz-db -p buzz-audit -p buzz-search -p buzz-relay --all-targets -- -D warnings` — passed - `cargo test -p buzz-datastore-tracing --quiet` — 4 passed - `cargo test -p buzz-db --quiet` — 109 passed, 200 ignored - `cargo test -p buzz-audit -p buzz-search --quiet` — 16 passed, 25 ignored - `cargo test -p buzz-relay --lib --quiet -- --test-threads=1` — 906 passed, 48 ignored - Native PostgreSQL focused tests for pool success/timeout/error, lock success/contention/timeout/error, replacement, membership serialization, push ordering, deletion fencing, migration/schema exclusion, and reader fallback — 8 passed The default-parallel relay run passed once; subsequent runs exposed the existing load-sensitive `api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo` 504 at the end of the suite. That test passes in isolation and the full relay suite passes serially. Independent exact-head review workstation: `buzz-tornquist-db-pressure-observability-review` (`2013067`). Formatting, the same all-target clippy command, datastore instrumentation tests, DB unit tests, source privacy guards, and diff/non-goal audits passed; no review findings. Generated with Codex --------- Signed-off-by: tornquist <tornquist@squareup.com>
210 lines
7.1 KiB
Rust
210 lines
7.1 KiB
Rust
use buzz_datastore_tracing::datastore_span;
|
|
use metrics_util::debugging::{DebugValue, DebuggingRecorder};
|
|
use opentelemetry::trace::{SpanKind, Status, TracerProvider as _};
|
|
use opentelemetry_sdk::trace::{InMemorySpanExporter, SdkTracerProvider};
|
|
use std::collections::BTreeMap;
|
|
use std::sync::{Arc, Mutex};
|
|
use tracing::field::{Field, Visit};
|
|
use tracing::{Event, Subscriber};
|
|
use tracing_subscriber::layer::{Context, Layer};
|
|
use tracing_subscriber::prelude::*;
|
|
|
|
const DIRECT_ERROR: &str = "raw-secret-direct-error";
|
|
const QUESTION_ERROR: &str = "raw-secret-question-error";
|
|
|
|
fn question_path(fail: bool) -> Result<(), &'static str> {
|
|
if fail {
|
|
Err(QUESTION_ERROR)
|
|
} else {
|
|
Ok(())
|
|
}
|
|
}
|
|
|
|
#[datastore_span(name = "test_operation", system = "postgresql", fields(limit = limit))]
|
|
async fn operation(
|
|
limit: usize,
|
|
direct_error: bool,
|
|
question_error: bool,
|
|
) -> Result<usize, &'static str> {
|
|
if direct_error {
|
|
return Err(DIRECT_ERROR);
|
|
}
|
|
question_path(question_error)?;
|
|
Ok(limit)
|
|
}
|
|
|
|
#[datastore_span(name = "slow_test_operation", system = "postgresql")]
|
|
async fn slow_operation(delay: std::time::Duration) -> Result<(), &'static str> {
|
|
tokio::time::sleep(delay).await;
|
|
Err(DIRECT_ERROR)
|
|
}
|
|
|
|
#[derive(Default)]
|
|
struct EventFields(BTreeMap<String, String>);
|
|
|
|
impl Visit for EventFields {
|
|
fn record_debug(&mut self, field: &Field, value: &dyn std::fmt::Debug) {
|
|
self.0.insert(field.name().to_owned(), format!("{value:?}"));
|
|
}
|
|
|
|
fn record_str(&mut self, field: &Field, value: &str) {
|
|
self.0.insert(field.name().to_owned(), value.to_owned());
|
|
}
|
|
|
|
fn record_u64(&mut self, field: &Field, value: u64) {
|
|
self.0.insert(field.name().to_owned(), value.to_string());
|
|
}
|
|
}
|
|
|
|
#[derive(Clone, Default)]
|
|
struct EventCapture(Arc<Mutex<Vec<EventFields>>>);
|
|
|
|
impl<S> Layer<S> for EventCapture
|
|
where
|
|
S: Subscriber,
|
|
{
|
|
fn on_event(&self, event: &Event<'_>, _context: Context<'_, S>) {
|
|
let mut fields = EventFields::default();
|
|
event.record(&mut fields);
|
|
self.0.lock().expect("capture lock").push(fields);
|
|
}
|
|
}
|
|
|
|
#[tokio::test(flavor = "current_thread")]
|
|
async fn exports_policy_fields_without_error_or_argument_data() {
|
|
let recorder = DebuggingRecorder::new();
|
|
let snapshotter = recorder.snapshotter();
|
|
let _metrics_guard = metrics::set_default_local_recorder(&recorder);
|
|
let exporter = InMemorySpanExporter::default();
|
|
let provider = SdkTracerProvider::builder()
|
|
.with_simple_exporter(exporter.clone())
|
|
.build();
|
|
let subscriber = tracing_subscriber::registry()
|
|
.with(tracing_opentelemetry::layer().with_tracer(provider.tracer("datastore-macro-test")));
|
|
let _subscriber_guard = tracing::subscriber::set_default(subscriber);
|
|
|
|
assert_eq!(operation(7, false, false).await, Ok(7));
|
|
assert_eq!(operation(8, true, false).await, Err(DIRECT_ERROR));
|
|
assert_eq!(operation(9, false, true).await, Err(QUESTION_ERROR));
|
|
|
|
let operation_samples = snapshotter
|
|
.snapshot()
|
|
.into_vec()
|
|
.into_iter()
|
|
.filter(|(key, ..)| key.key().name() == "buzz_db_operation_duration_seconds")
|
|
.map(|(key, _, _, value)| {
|
|
let DebugValue::Histogram(samples) = value else {
|
|
panic!("operation duration must be a histogram");
|
|
};
|
|
let labels = key
|
|
.key()
|
|
.labels()
|
|
.map(|label| (label.key().to_owned(), label.value().to_owned()))
|
|
.collect::<std::collections::BTreeMap<_, _>>();
|
|
(labels, samples)
|
|
})
|
|
.collect::<Vec<_>>();
|
|
assert_eq!(operation_samples.len(), 2);
|
|
for (labels, samples) in operation_samples {
|
|
assert_eq!(
|
|
labels.get("operation").map(String::as_str),
|
|
Some("test_operation")
|
|
);
|
|
assert!(matches!(
|
|
labels.get("outcome").map(String::as_str),
|
|
Some("success" | "error")
|
|
));
|
|
assert!(!samples.is_empty());
|
|
assert!(samples.iter().all(|sample| sample.into_inner() >= 0.0));
|
|
}
|
|
|
|
provider.force_flush().expect("spans flush");
|
|
let spans = exporter.get_finished_spans().expect("exported spans");
|
|
assert_eq!(spans.len(), 3);
|
|
|
|
for (span, (expected_limit, expected_status)) in spans.iter().zip([
|
|
(7_i64, Status::Unset),
|
|
(8_i64, Status::error("")),
|
|
(9_i64, Status::error("")),
|
|
]) {
|
|
assert_eq!(span.name, "test_operation");
|
|
assert_eq!(span.span_kind, SpanKind::Client);
|
|
assert_eq!(span.status, expected_status);
|
|
|
|
let attributes = span
|
|
.attributes
|
|
.iter()
|
|
.map(|attribute| (attribute.key.as_str(), attribute.value.to_string()))
|
|
.collect::<Vec<_>>();
|
|
assert!(attributes.contains(&("target", "buzz_datastore".to_owned())));
|
|
assert!(attributes.contains(&("db.system.name", "postgresql".to_owned())));
|
|
assert!(attributes.contains(&("limit", expected_limit.to_string())));
|
|
assert!(!attributes
|
|
.iter()
|
|
.any(|(key, _)| { matches!(*key, "direct_error" | "question_error") }));
|
|
|
|
let exported = format!("{span:?}");
|
|
assert!(!exported.contains(DIRECT_ERROR));
|
|
assert!(!exported.contains(QUESTION_ERROR));
|
|
assert!(span.events.iter().all(|event| {
|
|
!format!("{event:?}").contains(DIRECT_ERROR)
|
|
&& !format!("{event:?}").contains(QUESTION_ERROR)
|
|
}));
|
|
if let Status::Error { description } = &span.status {
|
|
assert!(description.is_empty());
|
|
}
|
|
}
|
|
}
|
|
|
|
#[tokio::test(flavor = "current_thread")]
|
|
async fn slow_operation_logging_is_guarded_sampled_and_redacted() {
|
|
let capture = EventCapture::default();
|
|
let subscriber = tracing_subscriber::registry().with(capture.clone());
|
|
let _subscriber_guard = tracing::subscriber::set_default(subscriber);
|
|
|
|
assert_eq!(
|
|
slow_operation(std::time::Duration::from_millis(1)).await,
|
|
Err(DIRECT_ERROR)
|
|
);
|
|
assert_eq!(
|
|
slow_operation(std::time::Duration::from_millis(510)).await,
|
|
Err(DIRECT_ERROR)
|
|
);
|
|
assert_eq!(
|
|
slow_operation(std::time::Duration::from_millis(510)).await,
|
|
Err(DIRECT_ERROR)
|
|
);
|
|
|
|
let events = capture.0.lock().expect("capture lock");
|
|
let slow = events
|
|
.iter()
|
|
.filter(|event| {
|
|
event
|
|
.0
|
|
.get("message")
|
|
.is_some_and(|message| message.contains("slow datastore operation"))
|
|
})
|
|
.collect::<Vec<_>>();
|
|
assert_eq!(
|
|
slow.len(),
|
|
1,
|
|
"first slow call is logged, next 99 are sampled out"
|
|
);
|
|
let fields = &slow[0].0;
|
|
assert_eq!(
|
|
fields.get("operation").map(String::as_str),
|
|
Some("slow_test_operation")
|
|
);
|
|
assert_eq!(fields.get("outcome").map(String::as_str), Some("error"));
|
|
assert!(fields
|
|
.get("elapsed_ms")
|
|
.and_then(|value| value.parse::<u64>().ok())
|
|
.is_some_and(|elapsed| elapsed >= 500));
|
|
assert_eq!(
|
|
fields.len(),
|
|
4,
|
|
"only message and fixed safe fields are logged"
|
|
);
|
|
assert!(!format!("{fields:?}").contains(DIRECT_ERROR));
|
|
}
|