From bfe89a7609ae8092a48c018c33639b7ce6ab27f4 Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 16 Jan 2025 10:14:10 +0100 Subject: [PATCH] error handling, simplify filelogsettings creation --- crates/stackable-telemetry/CHANGELOG.md | 2 +- crates/stackable-telemetry/src/tracing/mod.rs | 12 ++- .../src/tracing/settings/file_log.rs | 97 ++----------------- .../src/tracing/settings/mod.rs | 12 ++- 4 files changed, 24 insertions(+), 99 deletions(-) diff --git a/crates/stackable-telemetry/CHANGELOG.md b/crates/stackable-telemetry/CHANGELOG.md index a9e5d4b5..f2162a8d 100644 --- a/crates/stackable-telemetry/CHANGELOG.md +++ b/crates/stackable-telemetry/CHANGELOG.md @@ -7,7 +7,7 @@ All notable changes to this project will be documented in this file. ### Added - Introduce common `Settings` and subscriber specific settings ([#901]). -- Added support for logging to files ([#933]). +- Add support for logging to files ([#933]). ### Changed diff --git a/crates/stackable-telemetry/src/tracing/mod.rs b/crates/stackable-telemetry/src/tracing/mod.rs index 6216cb02..97823963 100644 --- a/crates/stackable-telemetry/src/tracing/mod.rs +++ b/crates/stackable-telemetry/src/tracing/mod.rs @@ -1,5 +1,5 @@ //! This module contains functionality to initialise tracing Subscribers for -//! console output, file output and OpenTelemetry OTLP export for traces and logs. +//! console output, file output, and OpenTelemetry OTLP export for traces and logs. //! //! It is intended to be used by the Stackable Data Platform operators and //! webhooks, but it should be generic enough to be used in any application. @@ -16,7 +16,7 @@ use opentelemetry_sdk::{ use opentelemetry_semantic_conventions::resource; use snafu::{ResultExt as _, Snafu}; use tracing::subscriber::SetGlobalDefaultError; -use tracing_appender::rolling::{RollingFileAppender, Rotation}; +use tracing_appender::rolling::{InitError, RollingFileAppender, Rotation}; use tracing_subscriber::{filter::Directive, layer::SubscriberExt, EnvFilter, Layer, Registry}; use settings::{ConsoleLogSettings, FileLogSettings, OtlpLogSettings, OtlpTraceSettings}; @@ -39,6 +39,9 @@ pub enum Error { #[snafu(display("unable to set the global default subscriber"))] SetGlobalDefaultSubscriber { source: SetGlobalDefaultError }, + + #[snafu(display("failed to initialize rolling file appender"))] + InitRollingFileAppender { source: InitError }, } /// Easily initialize a set of preconfigured [`Subscriber`][1] layers. @@ -195,7 +198,7 @@ impl Tracing { .filename_suffix("tracing-rs.json") .max_log_files(6) .build(&self.file_log_settings.file_log_dir) - .expect("failed to initialize rolling file appender"); + .context(InitRollingFileAppenderSnafu)?; layers.push( tracing_subscriber::fmt::layer() @@ -560,8 +563,7 @@ mod test { .with_environment_variable("ABC_FILE") .with_default_level(LevelFilter::INFO) .enabled(true) - .file_log_settings_builder() - .with_file_log_dir(String::from("/abc_file_dir")) + .file_log_settings_builder(String::from("/abc_file_dir")) .build(), ) .with_otlp_log_exporter( diff --git a/crates/stackable-telemetry/src/tracing/settings/file_log.rs b/crates/stackable-telemetry/src/tracing/settings/file_log.rs index be0c4f97..8daaba41 100644 --- a/crates/stackable-telemetry/src/tracing/settings/file_log.rs +++ b/crates/stackable-telemetry/src/tracing/settings/file_log.rs @@ -2,7 +2,7 @@ use std::{ops::Deref, path::PathBuf}; -use super::{Build, Settings, SettingsBuilder}; +use super::Settings; /// Configure specific settings for the File Log subscriber. #[derive(Debug, Default, PartialEq)] @@ -22,106 +22,22 @@ impl Deref for FileLogSettings { } } -/// This trait is only used for the typestate builder and cannot be implemented -/// outside of this crate. -/// -/// The only reason it has pub visibility is because it needs to be at least as -/// visible as the types that use it. -#[doc(hidden)] -pub trait BuilderState: private::Sealed {} - -/// This private module holds the [`Sealed`][1] trait that is used by the -/// [`BuilderState`], so that it cannot be implemented outside of this crate. -/// -/// We impl Sealed for any types that will use the trait that we want to -/// restrict impls on. In this case, the [`BuilderState`] trait. -/// -/// [1]: private::Sealed -#[doc(hidden)] -mod private { - use super::*; - - pub trait Sealed {} - - impl Sealed for builder_state::PreLogDir {} - impl Sealed for builder_state::Config {} -} - -/// This module holds the possible states that the builder is in. -/// -/// Each state will implement [`BuilderState`] (with no methods), and the -/// Builder struct ([`FileLogSettingsBuilder`][1]) itself will be implemented with -/// each state as a generic parameter. -/// This allows only the methods to be called when the builder is in the -/// applicable state. -/// -/// [1]: super::FileLogSettingsBuilder -#[doc(hidden)] -pub mod builder_state { - /// The initial state, before the log directory path is set. - #[derive(Default)] - pub struct PreLogDir; - - /// The state that allows you to configure the [`FileLogSettings`][1]. - /// - /// [1]: super::FileLogSettings - #[derive(Default)] - pub struct Config; -} - -// Make the states usable -#[doc(hidden)] -impl BuilderState for builder_state::PreLogDir {} - -#[doc(hidden)] -impl BuilderState for builder_state::Config {} - /// For building [`FileLogSettings`]. /// ///
/// Do not use directly, instead use the [`Settings::builder`] associated function. ///
-pub struct FileLogSettingsBuilder { +pub struct FileLogSettingsBuilder { pub(crate) common_settings: Settings, - pub(crate) file_log_dir: Option, - - /// Allow the generic to be used (needed for impls). - _marker: std::marker::PhantomData, + pub(crate) file_log_dir: PathBuf, } -impl FileLogSettingsBuilder { - /// Set the directory for log files. - /// - /// A directory is required for using the File Log subscriber. - pub fn with_file_log_dir(self, path: String) -> FileLogSettingsBuilder { - FileLogSettingsBuilder { - common_settings: self.common_settings, - file_log_dir: Some(PathBuf::from(path)), - _marker: std::marker::PhantomData, - } - } -} - -impl FileLogSettingsBuilder { +impl FileLogSettingsBuilder { /// Consumes self and returns a valid [`FileLogSettings`] instance. pub fn build(self) -> FileLogSettings { FileLogSettings { common_settings: self.common_settings, - file_log_dir: self - .file_log_dir - .expect("file_log_dir must be configured at this point"), - } - } -} - -/// This implementation is used to turn the common settings builder into the file log specific -/// settings builder via the [`SettingsBuilder::file_log_settings_builder`] function. -impl From for FileLogSettingsBuilder { - fn from(value: SettingsBuilder) -> Self { - Self { - common_settings: value.build(), - file_log_dir: None, - _marker: std::marker::PhantomData, + file_log_dir: self.file_log_dir, } } } @@ -146,8 +62,7 @@ mod test { .with_environment_variable("hello") .with_default_level(LevelFilter::DEBUG) .enabled(true) - .file_log_settings_builder() - .with_file_log_dir(String::from("/logs")) + .file_log_settings_builder(String::from("/logs")) .build(); assert_eq!(expected, result); diff --git a/crates/stackable-telemetry/src/tracing/settings/mod.rs b/crates/stackable-telemetry/src/tracing/settings/mod.rs index 6349b533..94915ab0 100644 --- a/crates/stackable-telemetry/src/tracing/settings/mod.rs +++ b/crates/stackable-telemetry/src/tracing/settings/mod.rs @@ -1,5 +1,7 @@ //! Subscriber settings. +use std::path::Path; + use tracing::level_filters::LevelFilter; pub mod console_log; @@ -109,8 +111,14 @@ impl SettingsBuilder { } /// Set specific [`FileLogSettings`]. - pub fn file_log_settings_builder(self) -> FileLogSettingsBuilder { - self.into() + pub fn file_log_settings_builder

(self, path: P) -> FileLogSettingsBuilder + where + P: AsRef, + { + FileLogSettingsBuilder { + common_settings: self.build(), + file_log_dir: path.as_ref().to_path_buf(), + } } /// Set specific [`OtlpLogSettings`].