From fe615520ddf3c024f6ef896cd2197017280b1dd5 Mon Sep 17 00:00:00 2001 From: Christian Schwarz Date: Tue, 21 Jan 2025 20:30:28 +0100 Subject: [PATCH] remove the timing histograms for traversal and walredo, since their meaning and utility is dubious with concurrent IO; https://github.com/neondatabase/neon/pull/9353#discussion_r1924181713 The issue is that get_vectored_reconstruct_data latency means something very different now with concurrent IO than what it did before, because all the time we spend on the data blocks is no longer part of the get_vectored_reconstruct_data().await wall clock time GET_RECONSTRUCT_DATA_TIME : all the 3 dashboards that use it are in my /personal/christian folder. I guess I'm free to break them :smile: https://github.com/search?q=repo%3Aneondatabase%2Fgrafana-dashboard-export%20pageserver_getpage_get_reconstruct_data_seconds&type=code RECONSTRUCT_TIME Used in a couple of dashboards I think nobody uses - Timeline Inspector - Sharding WAL streaming - Pageserver - walredo time throaway Vlad agrees with removing them for now. Maybe in the future we'll add some back pageserver_getpage_get_reconstruct_data_seconds -> pageserver_getpage_io_plan_seconds pageserver_getpage_reconstruct_data_seconds -> pageserver_getpage_io_execute_seconds --- pageserver/src/metrics.rs | 68 ------------------------------- pageserver/src/tenant/timeline.rs | 20 +-------- test_runner/fixtures/metrics.py | 4 -- 3 files changed, 1 insertion(+), 91 deletions(-) diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index 4758aaf230..7d383379db 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -126,73 +126,6 @@ pub(crate) static INITDB_RUN_TIME: Lazy = Lazy::new(|| { .expect("failed to define metric") }); -// Metrics collected on operations on the storage repository. -#[derive( - Clone, Copy, enum_map::Enum, strum_macros::EnumString, strum_macros::Display, IntoStaticStr, -)] -pub(crate) enum GetKind { - Singular, - Vectored, -} - -pub(crate) struct ReconstructTimeMetrics { - singular: Histogram, - vectored: Histogram, -} - -pub(crate) static RECONSTRUCT_TIME: Lazy = Lazy::new(|| { - let inner = register_histogram_vec!( - "pageserver_getpage_reconstruct_seconds", - "Time spent in reconstruct_value (reconstruct a page from deltas)", - &["get_kind"], - CRITICAL_OP_BUCKETS.into(), - ) - .expect("failed to define a metric"); - - ReconstructTimeMetrics { - singular: inner.with_label_values(&[GetKind::Singular.into()]), - vectored: inner.with_label_values(&[GetKind::Vectored.into()]), - } -}); - -impl ReconstructTimeMetrics { - pub(crate) fn for_get_kind(&self, get_kind: GetKind) -> &Histogram { - match get_kind { - GetKind::Singular => &self.singular, - GetKind::Vectored => &self.vectored, - } - } -} - -pub(crate) struct ReconstructDataTimeMetrics { - singular: Histogram, - vectored: Histogram, -} - -impl ReconstructDataTimeMetrics { - pub(crate) fn for_get_kind(&self, get_kind: GetKind) -> &Histogram { - match get_kind { - GetKind::Singular => &self.singular, - GetKind::Vectored => &self.vectored, - } - } -} - -pub(crate) static GET_RECONSTRUCT_DATA_TIME: Lazy = Lazy::new(|| { - let inner = register_histogram_vec!( - "pageserver_getpage_get_reconstruct_data_seconds", - "Time spent in get_reconstruct_value_data", - &["get_kind"], - CRITICAL_OP_BUCKETS.into(), - ) - .expect("failed to define a metric"); - - ReconstructDataTimeMetrics { - singular: inner.with_label_values(&[GetKind::Singular.into()]), - vectored: inner.with_label_values(&[GetKind::Vectored.into()]), - } -}); - pub(crate) struct GetVectoredLatency { map: EnumMap>, } @@ -3919,7 +3852,6 @@ pub fn preinitialize_metrics(conf: &'static PageServerConf) { }); // Custom - Lazy::force(&RECONSTRUCT_TIME); Lazy::force(&BASEBACKUP_QUERY_TIME); Lazy::force(&COMPUTE_COMMANDS_COUNTERS); Lazy::force(&tokio_epoll_uring::THREAD_LOCAL_METRICS_STORAGE); diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 68ab597551..4509ba6a8d 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -75,6 +75,7 @@ use std::{ ops::{Deref, Range}, }; +use crate::l0_flush::{self, L0FlushGlobalState}; use crate::{ aux_file::AuxFileSizeEstimator, page_service::TenantManagerTypes, @@ -106,10 +107,6 @@ use crate::{ use crate::{ disk_usage_eviction_task::EvictionCandidate, tenant::storage_layer::delta_layer::DeltaEntry, }; -use crate::{ - l0_flush::{self, L0FlushGlobalState}, - metrics::GetKind, -}; use crate::{ metrics::ScanLatencyOngoingRecording, tenant::timeline::logical_size::CurrentLogicalSize, }; @@ -1163,15 +1160,6 @@ impl Timeline { reconstruct_state: &mut ValuesReconstructState, ctx: &RequestContext, ) -> Result>, GetVectoredError> { - let get_kind = if keyspace.total_raw_size() == 1 { - GetKind::Singular - } else { - GetKind::Vectored - }; - - let get_data_timer = crate::metrics::GET_RECONSTRUCT_DATA_TIME - .for_get_kind(get_kind) - .start_timer(); let traversal_res: Result<(), _> = self .get_vectored_reconstruct_data(keyspace.clone(), lsn, reconstruct_state, ctx) .await; @@ -1185,11 +1173,7 @@ impl Timeline { while collect_futs.next().await.is_some() {} return Err(err); }; - get_data_timer.stop_and_record(); - let reconstruct_timer = crate::metrics::RECONSTRUCT_TIME - .for_get_kind(get_kind) - .start_timer(); let layers_visited = reconstruct_state.get_layers_visited(); let futs = FuturesUnordered::new(); @@ -1227,8 +1211,6 @@ impl Timeline { .collect::>>() .await; - reconstruct_timer.stop_and_record(); - // For aux file keys (v1 or v2) the vectored read path does not return an error // when they're missing. Instead they are omitted from the resulting btree // (this is a requirement, not a bug). Skip updating the metric in these cases diff --git a/test_runner/fixtures/metrics.py b/test_runner/fixtures/metrics.py index fa541bad17..fd7e193778 100644 --- a/test_runner/fixtures/metrics.py +++ b/test_runner/fixtures/metrics.py @@ -126,12 +126,8 @@ PAGESERVER_GLOBAL_METRICS: tuple[str, ...] = ( "pageserver_page_cache_read_accesses_total", "pageserver_page_cache_size_current_bytes", "pageserver_page_cache_size_max_bytes", - "pageserver_getpage_reconstruct_seconds_bucket", - "pageserver_getpage_reconstruct_seconds_count", - "pageserver_getpage_reconstruct_seconds_sum", *[f"pageserver_basebackup_query_seconds_{x}" for x in ["bucket", "count", "sum"]], *histogram("pageserver_smgr_query_seconds_global"), - *histogram("pageserver_getpage_get_reconstruct_data_seconds"), *histogram("pageserver_wait_lsn_seconds"), *histogram("pageserver_remote_operation_seconds"), *histogram("pageserver_io_operations_seconds"),