From 87dd37a2f2da8add5c1e1e8b63aa7b78c5da930d Mon Sep 17 00:00:00 2001 From: Joonas Koivunen Date: Wed, 12 Jul 2023 16:58:40 +0300 Subject: [PATCH] pageserver: Align tenant, timeline id names in spans (#4687) Uses `(tenant|timeline)_id`. Not a statement about endorsing this naming style but it is better to be aligned. --- pageserver/src/disk_usage_eviction_task.rs | 2 +- pageserver/src/http/routes.rs | 22 +++++++++---------- pageserver/src/tenant.rs | 4 ++-- pageserver/src/tenant/mgr.rs | 6 ++--- .../src/tenant/remote_timeline_client.rs | 8 ++++--- pageserver/src/tenant/span.rs | 4 ++-- pageserver/src/tenant/timeline.rs | 8 +++---- pageserver/src/tenant/timeline/span.rs | 6 ++--- pageserver/src/tenant/timeline/uninit.rs | 2 +- 9 files changed, 30 insertions(+), 32 deletions(-) diff --git a/pageserver/src/disk_usage_eviction_task.rs b/pageserver/src/disk_usage_eviction_task.rs index 3cb89ab9e9..b2ca9ab0bb 100644 --- a/pageserver/src/disk_usage_eviction_task.rs +++ b/pageserver/src/disk_usage_eviction_task.rs @@ -305,7 +305,7 @@ pub async fn disk_usage_eviction_task_iteration_impl( let now = SystemTime::now(); for (i, (partition, candidate)) in candidates.iter().enumerate() { debug!( - "cand {}/{}: size={}, no_access_for={}us, parition={:?}, tenant={} timeline={} layer={}", + "cand {}/{}: size={}, no_access_for={}us, partition={:?}, {}/{}/{}", i + 1, candidates.len(), candidate.layer.file_size(), diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs index 58dcbb2aac..f39db891e1 100644 --- a/pageserver/src/http/routes.rs +++ b/pageserver/src/http/routes.rs @@ -346,7 +346,7 @@ async fn timeline_create_handler( Err(tenant::CreateTimelineError::Other(err)) => Err(ApiError::InternalServerError(err)), } } - .instrument(info_span!("timeline_create", tenant = %tenant_id, timeline_id = %new_timeline_id, lsn=?request_data.ancestor_start_lsn, pg_version=?request_data.pg_version)) + .instrument(info_span!("timeline_create", %tenant_id, timeline_id = %new_timeline_id, lsn=?request_data.ancestor_start_lsn, pg_version=?request_data.pg_version)) .await } @@ -381,7 +381,7 @@ async fn timeline_list_handler( } Ok::, ApiError>(response_data) } - .instrument(info_span!("timeline_list", tenant = %tenant_id)) + .instrument(info_span!("timeline_list", %tenant_id)) .await?; json_response(StatusCode::OK, response_data) @@ -418,7 +418,7 @@ async fn timeline_detail_handler( Ok::<_, ApiError>(timeline_info) } - .instrument(info_span!("timeline_detail", tenant = %tenant_id, timeline = %timeline_id)) + .instrument(info_span!("timeline_detail", %tenant_id, %timeline_id)) .await?; json_response(StatusCode::OK, timeline_info) @@ -479,7 +479,7 @@ async fn tenant_attach_handler( remote_storage.clone(), &ctx, ) - .instrument(info_span!("tenant_attach", tenant = %tenant_id)) + .instrument(info_span!("tenant_attach", %tenant_id)) .await?; } else { return Err(ApiError::BadRequest(anyhow!( @@ -501,7 +501,7 @@ async fn timeline_delete_handler( let ctx = RequestContext::new(TaskKind::MgmtRequest, DownloadBehavior::Warn); mgr::delete_timeline(tenant_id, timeline_id, &ctx) - .instrument(info_span!("timeline_delete", tenant = %tenant_id, timeline = %timeline_id)) + .instrument(info_span!("timeline_delete", %tenant_id, %timeline_id)) .await?; // FIXME: needs to be an error for console to retry it. Ideally Accepted should be used and retried until 404. @@ -519,7 +519,7 @@ async fn tenant_detach_handler( let state = get_state(&request); let conf = state.conf; mgr::detach_tenant(conf, tenant_id, detach_ignored.unwrap_or(false)) - .instrument(info_span!("tenant_detach", tenant = %tenant_id)) + .instrument(info_span!("tenant_detach", %tenant_id)) .await?; json_response(StatusCode::OK, ()) @@ -542,7 +542,7 @@ async fn tenant_load_handler( state.remote_storage.clone(), &ctx, ) - .instrument(info_span!("load", tenant = %tenant_id)) + .instrument(info_span!("load", %tenant_id)) .await?; json_response(StatusCode::ACCEPTED, ()) @@ -558,7 +558,7 @@ async fn tenant_ignore_handler( let state = get_state(&request); let conf = state.conf; mgr::ignore_tenant(conf, tenant_id) - .instrument(info_span!("ignore_tenant", tenant = %tenant_id)) + .instrument(info_span!("ignore_tenant", %tenant_id)) .await?; json_response(StatusCode::OK, ()) @@ -611,7 +611,7 @@ async fn tenant_status( attachment_status: state.attachment_status(), }) } - .instrument(info_span!("tenant_status_handler", tenant = %tenant_id)) + .instrument(info_span!("tenant_status_handler", %tenant_id)) .await?; json_response(StatusCode::OK, tenant_info) @@ -850,7 +850,7 @@ async fn tenant_create_handler( state.remote_storage.clone(), &ctx, ) - .instrument(info_span!("tenant_create", tenant = ?target_tenant_id)) + .instrument(info_span!("tenant_create", tenant_id = %target_tenant_id)) .await?; // We created the tenant. Existing API semantics are that the tenant @@ -912,7 +912,7 @@ async fn update_tenant_config_handler( let state = get_state(&request); mgr::set_new_tenant_config(state.conf, tenant_conf, tenant_id) - .instrument(info_span!("tenant_config", tenant = ?tenant_id)) + .instrument(info_span!("tenant_config", %tenant_id)) .await?; json_response(StatusCode::OK, ()) diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs index 5e207408cc..83096faa9c 100644 --- a/pageserver/src/tenant.rs +++ b/pageserver/src/tenant.rs @@ -560,7 +560,7 @@ impl Tenant { .map(move |res| { res.with_context(|| format!("download index part for timeline {timeline_id}")) }) - .instrument(info_span!("download_index_part", timeline=%timeline_id)), + .instrument(info_span!("download_index_part", %timeline_id)), ); } // Wait for all the download tasks to complete & collect results. @@ -1349,7 +1349,7 @@ impl Tenant { for (timeline_id, timeline) in &timelines_to_compact { timeline .compact(ctx) - .instrument(info_span!("compact_timeline", timeline = %timeline_id)) + .instrument(info_span!("compact_timeline", %timeline_id)) .await?; } diff --git a/pageserver/src/tenant/mgr.rs b/pageserver/src/tenant/mgr.rs index 8e31cc2ef1..2cc881ed5e 100644 --- a/pageserver/src/tenant/mgr.rs +++ b/pageserver/src/tenant/mgr.rs @@ -690,7 +690,7 @@ pub async fn immediate_gc( fail::fail_point!("immediate_gc_task_pre"); let result = tenant .gc_iteration(Some(timeline_id), gc_horizon, pitr, &ctx) - .instrument(info_span!("manual_gc", tenant = %tenant_id, timeline = %timeline_id)) + .instrument(info_span!("manual_gc", %tenant_id, %timeline_id)) .await; // FIXME: `gc_iteration` can return an error for multiple reasons; we should handle it // better once the types support it. @@ -740,9 +740,7 @@ pub async fn immediate_compact( async move { let result = timeline .compact(&ctx) - .instrument( - info_span!("manual_compact", tenant = %tenant_id, timeline = %timeline_id), - ) + .instrument(info_span!("manual_compact", %tenant_id, %timeline_id)) .await; match task_done.send(result) { diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index bc185b4f7a..d5468b43d0 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -933,11 +933,11 @@ impl RemoteTimelineClient { // Assign unique ID to this task upload_queue.task_counter += 1; - let task_id = upload_queue.task_counter; + let upload_task_id = upload_queue.task_counter; // Add it to the in-progress map let task = Arc::new(UploadTask { - task_id, + task_id: upload_task_id, op: next_op, retries: AtomicU32::new(0), }); @@ -947,6 +947,8 @@ impl RemoteTimelineClient { // Spawn task to perform the task let self_rc = Arc::clone(self); + let tenant_id = self.tenant_id; + let timeline_id = self.timeline_id; task_mgr::spawn( self.runtime.handle(), TaskKind::RemoteUploadTask, @@ -958,7 +960,7 @@ impl RemoteTimelineClient { self_rc.perform_upload_task(task).await; Ok(()) } - .instrument(info_span!(parent: None, "remote_upload", tenant = %self.tenant_id, timeline = %self.timeline_id, upload_task_id = %task_id)), + .instrument(info_span!(parent: None, "remote_upload", %tenant_id, %timeline_id, %upload_task_id)), ); // Loop back to process next task diff --git a/pageserver/src/tenant/span.rs b/pageserver/src/tenant/span.rs index 3728dd9dd8..a65ad6af47 100644 --- a/pageserver/src/tenant/span.rs +++ b/pageserver/src/tenant/span.rs @@ -5,8 +5,8 @@ use utils::tracing_span_assert::{check_fields_present, MultiNameExtractor}; pub(crate) fn debug_assert_current_span_has_tenant_id() {} #[cfg(debug_assertions)] -pub(crate) static TENANT_ID_EXTRACTOR: once_cell::sync::Lazy> = - once_cell::sync::Lazy::new(|| MultiNameExtractor::new("TenantId", ["tenant_id", "tenant"])); +pub(crate) static TENANT_ID_EXTRACTOR: once_cell::sync::Lazy> = + once_cell::sync::Lazy::new(|| MultiNameExtractor::new("TenantId", ["tenant_id"])); #[cfg(debug_assertions)] #[track_caller] diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index d790f0da1c..58a2adbb58 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -1051,7 +1051,7 @@ impl Timeline { } } - #[instrument(skip_all, fields(tenant = %self.tenant_id, timeline = %self.timeline_id))] + #[instrument(skip_all, fields(tenant_id = %self.tenant_id, timeline_id = %self.timeline_id))] pub async fn download_layer(&self, layer_file_name: &str) -> anyhow::Result> { let Some(layer) = self.find_layer(layer_file_name).await else { return Ok(None) }; let Some(remote_layer) = layer.downcast_remote_layer() else { return Ok(Some(false)) }; @@ -1539,7 +1539,7 @@ impl Timeline { *flush_loop_state = FlushLoopState::Exited; Ok(()) } - .instrument(info_span!(parent: None, "layer flush task", tenant = %self.tenant_id, timeline = %self.timeline_id)) + .instrument(info_span!(parent: None, "layer flush task", tenant_id = %self.tenant_id, timeline_id = %self.timeline_id)) ); } @@ -4104,7 +4104,7 @@ impl Timeline { new_gc_cutoff, ) .instrument( - info_span!("gc_timeline", timeline = %self.timeline_id, cutoff = %new_gc_cutoff), + info_span!("gc_timeline", timeline_id = %self.timeline_id, cutoff = %new_gc_cutoff), ) .await?; @@ -4590,7 +4590,7 @@ impl Timeline { }; Ok(()) } - .instrument(info_span!(parent: None, "download_all_remote_layers", tenant = %self.tenant_id, timeline = %self.timeline_id)) + .instrument(info_span!(parent: None, "download_all_remote_layers", tenant_id = %self.tenant_id, timeline_id = %self.timeline_id)) ); let initial_info = DownloadRemoteLayersTaskInfo { diff --git a/pageserver/src/tenant/timeline/span.rs b/pageserver/src/tenant/timeline/span.rs index 9ef0d5f92a..19a7fdb011 100644 --- a/pageserver/src/tenant/timeline/span.rs +++ b/pageserver/src/tenant/timeline/span.rs @@ -7,10 +7,8 @@ pub(crate) fn debug_assert_current_span_has_tenant_and_timeline_id() {} #[cfg(debug_assertions)] #[track_caller] pub(crate) fn debug_assert_current_span_has_tenant_and_timeline_id() { - static TIMELINE_ID_EXTRACTOR: once_cell::sync::Lazy> = - once_cell::sync::Lazy::new(|| { - MultiNameExtractor::new("TimelineId", ["timeline_id", "timeline"]) - }); + static TIMELINE_ID_EXTRACTOR: once_cell::sync::Lazy> = + once_cell::sync::Lazy::new(|| MultiNameExtractor::new("TimelineId", ["timeline_id"])); let fields: [&dyn Extractor; 2] = [ &*crate::tenant::span::TENANT_ID_EXTRACTOR, diff --git a/pageserver/src/tenant/timeline/uninit.rs b/pageserver/src/tenant/timeline/uninit.rs index 27d43a8b24..b8cc65f4b1 100644 --- a/pageserver/src/tenant/timeline/uninit.rs +++ b/pageserver/src/tenant/timeline/uninit.rs @@ -132,7 +132,7 @@ impl<'t> UninitializedTimeline<'t> { impl Drop for UninitializedTimeline<'_> { fn drop(&mut self) { if let Some((_, uninit_mark)) = self.raw_timeline.take() { - let _entered = info_span!("drop_uninitialized_timeline", tenant = %self.owning_tenant.tenant_id, timeline = %self.timeline_id).entered(); + let _entered = info_span!("drop_uninitialized_timeline", tenant_id = %self.owning_tenant.tenant_id, timeline_id = %self.timeline_id).entered(); error!("Timeline got dropped without initializing, cleaning its files"); cleanup_timeline_directory(uninit_mark); }