From 689ec7c7ad3225a8e098f2f3104ea341c58846d1 Mon Sep 17 00:00:00 2001 From: bwbonanno Date: Fri, 13 Oct 2023 22:12:54 +0000 Subject: [PATCH 1/6] Make the experimental route /metrics activable via HTTP --- index-scheduler/src/features.rs | 11 ++-- meilisearch-types/src/features.rs | 1 + meilisearch/src/routes/features.rs | 11 +++- meilisearch/tests/auth/authorization.rs | 8 ++- meilisearch/tests/common/server.rs | 6 +- meilisearch/tests/features/mod.rs | 73 ++++++++++++++++++++++--- 6 files changed, 93 insertions(+), 17 deletions(-) diff --git a/index-scheduler/src/features.rs b/index-scheduler/src/features.rs index 5a663fe67..442a43320 100644 --- a/index-scheduler/src/features.rs +++ b/index-scheduler/src/features.rs @@ -10,19 +10,17 @@ const EXPERIMENTAL_FEATURES: &str = "experimental-features"; #[derive(Clone)] pub(crate) struct FeatureData { runtime: Database>, - instance: InstanceTogglableFeatures, } #[derive(Debug, Clone, Copy)] pub struct RoFeatures { runtime: RuntimeTogglableFeatures, - instance: InstanceTogglableFeatures, } impl RoFeatures { fn new(txn: RoTxn<'_>, data: &FeatureData) -> Result { let runtime = data.runtime_features(txn)?; - Ok(Self { runtime, instance: data.instance }) + Ok(Self { runtime }) } pub fn runtime_features(&self) -> RuntimeTogglableFeatures { @@ -43,7 +41,7 @@ impl RoFeatures { } pub fn check_metrics(&self) -> Result<()> { - if self.instance.metrics { + if self.runtime.metrics { Ok(()) } else { Err(FeatureNotEnabledError { @@ -73,9 +71,12 @@ impl FeatureData { pub fn new(env: &Env, instance_features: InstanceTogglableFeatures) -> Result { let mut wtxn = env.write_txn()?; let runtime_features = env.create_database(&mut wtxn, Some(EXPERIMENTAL_FEATURES))?; + let default_features = + RuntimeTogglableFeatures { metrics: instance_features.metrics, ..Default::default() }; + runtime_features.put(&mut wtxn, EXPERIMENTAL_FEATURES, &default_features)?; wtxn.commit()?; - Ok(Self { runtime: runtime_features, instance: instance_features }) + Ok(Self { runtime: runtime_features }) } pub fn put_runtime_features( diff --git a/meilisearch-types/src/features.rs b/meilisearch-types/src/features.rs index f62300485..4fe4affd4 100644 --- a/meilisearch-types/src/features.rs +++ b/meilisearch-types/src/features.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; pub struct RuntimeTogglableFeatures { pub score_details: bool, pub vector_store: bool, + pub metrics: bool, } #[derive(Default, Debug, Clone, Copy)] diff --git a/meilisearch/src/routes/features.rs b/meilisearch/src/routes/features.rs index a2822b4d4..8e35acc56 100644 --- a/meilisearch/src/routes/features.rs +++ b/meilisearch/src/routes/features.rs @@ -44,6 +44,8 @@ pub struct RuntimeTogglableFeatures { pub score_details: Option, #[deserr(default)] pub vector_store: Option, + #[deserr(default)] + pub metrics: Option, } async fn patch_features( @@ -62,19 +64,24 @@ async fn patch_features( let new_features = meilisearch_types::features::RuntimeTogglableFeatures { score_details: new_features.0.score_details.unwrap_or(old_features.score_details), vector_store: new_features.0.vector_store.unwrap_or(old_features.vector_store), + metrics: new_features.0.metrics.unwrap_or(old_features.metrics), }; // explicitly destructure for analytics rather than using the `Serialize` implementation, because // the it renames to camelCase, which we don't want for analytics. // **Do not** ignore fields with `..` or `_` here, because we want to add them in the future. - let meilisearch_types::features::RuntimeTogglableFeatures { score_details, vector_store } = - new_features; + let meilisearch_types::features::RuntimeTogglableFeatures { + score_details, + vector_store, + metrics, + } = new_features; analytics.publish( "Experimental features Updated".to_string(), json!({ "score_details": score_details, "vector_store": vector_store, + "metrics": metrics, }), Some(&req), ); diff --git a/meilisearch/tests/auth/authorization.rs b/meilisearch/tests/auth/authorization.rs index 883c23267..af028060d 100644 --- a/meilisearch/tests/auth/authorization.rs +++ b/meilisearch/tests/auth/authorization.rs @@ -2,10 +2,12 @@ use std::collections::{HashMap, HashSet}; use ::time::format_description::well_known::Rfc3339; use maplit::{hashmap, hashset}; +use meilisearch::Opt; use once_cell::sync::Lazy; +use tempfile::TempDir; use time::{Duration, OffsetDateTime}; -use crate::common::{Server, Value}; +use crate::common::{default_settings, Server, Value}; use crate::json; pub static AUTHORIZATIONS: Lazy>> = @@ -195,7 +197,9 @@ async fn access_authorized_master_key() { #[actix_rt::test] async fn access_authorized_restricted_index() { - let mut server = Server::new_auth().await; + let dir = TempDir::new().unwrap(); + let enable_metrics = Opt { experimental_enable_metrics: true, ..default_settings(dir.path()) }; + let mut server = Server::new_auth_with_options(enable_metrics, dir).await; for ((method, route), actions) in AUTHORIZATIONS.iter() { for action in actions { // create a new API key letting only the needed action. diff --git a/meilisearch/tests/common/server.rs b/meilisearch/tests/common/server.rs index 58f561eb8..27feb187f 100644 --- a/meilisearch/tests/common/server.rs +++ b/meilisearch/tests/common/server.rs @@ -202,6 +202,10 @@ impl Server { pub async fn set_features(&self, value: Value) -> (Value, StatusCode) { self.service.patch("/experimental-features", value).await } + + pub async fn get_metrics(&self) -> (Value, StatusCode) { + self.service.get("/metrics").await + } } pub fn default_settings(dir: impl AsRef) -> Opt { @@ -221,7 +225,7 @@ pub fn default_settings(dir: impl AsRef) -> Opt { skip_index_budget: true, ..Parser::parse_from(None as Option<&str>) }, - experimental_enable_metrics: true, + experimental_enable_metrics: false, ..Parser::parse_from(None as Option<&str>) } } diff --git a/meilisearch/tests/features/mod.rs b/meilisearch/tests/features/mod.rs index 348deb5e2..9de829d50 100644 --- a/meilisearch/tests/features/mod.rs +++ b/meilisearch/tests/features/mod.rs @@ -1,4 +1,7 @@ -use crate::common::Server; +use meilisearch::Opt; +use tempfile::TempDir; + +use crate::common::{default_settings, Server}; use crate::json; /// Feature name to test against. @@ -16,7 +19,8 @@ async fn experimental_features() { meili_snap::snapshot!(meili_snap::json_string!(response), @r###" { "scoreDetails": false, - "vectorStore": false + "vectorStore": false, + "metrics": false } "###); @@ -26,7 +30,8 @@ async fn experimental_features() { meili_snap::snapshot!(meili_snap::json_string!(response), @r###" { "scoreDetails": false, - "vectorStore": true + "vectorStore": true, + "metrics": false } "###); @@ -36,7 +41,8 @@ async fn experimental_features() { meili_snap::snapshot!(meili_snap::json_string!(response), @r###" { "scoreDetails": false, - "vectorStore": true + "vectorStore": true, + "metrics": false } "###); @@ -47,7 +53,8 @@ async fn experimental_features() { meili_snap::snapshot!(meili_snap::json_string!(response), @r###" { "scoreDetails": false, - "vectorStore": true + "vectorStore": true, + "metrics": false } "###); @@ -58,11 +65,63 @@ async fn experimental_features() { meili_snap::snapshot!(meili_snap::json_string!(response), @r###" { "scoreDetails": false, - "vectorStore": true + "vectorStore": true, + "metrics": false } "###); } +#[actix_rt::test] +async fn experimental_feature_metrics() { + // instance flag for metrics enables metrics at startup + let dir = TempDir::new().unwrap(); + let enable_metrics = Opt { experimental_enable_metrics: true, ..default_settings(dir.path()) }; + let server = Server::new_with_options(enable_metrics).await.unwrap(); + + let (response, code) = server.get_features().await; + + meili_snap::snapshot!(code, @"200 OK"); + meili_snap::snapshot!(meili_snap::json_string!(response), @r###" + { + "scoreDetails": false, + "vectorStore": false, + "metrics": true + } + "###); + + let (response, code) = server.get_metrics().await; + meili_snap::snapshot!(code, @"200 OK"); + + // metrics are not returned in json format + // so the test server will return null + meili_snap::snapshot!(response, @"null"); + + // disabling metrics results in invalid request + let (response, code) = server.set_features(json!({"metrics": false})).await; + meili_snap::snapshot!(code, @"200 OK"); + meili_snap::snapshot!(response["metrics"], @"false"); + + let (response, code) = server.get_metrics().await; + meili_snap::snapshot!(code, @"400 Bad Request"); + meili_snap::snapshot!(meili_snap::json_string!(response), @r###" + { + "message": "Getting metrics requires enabling the `metrics` experimental feature. See https://github.com/meilisearch/meilisearch/discussions/3518", + "code": "feature_not_enabled", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#feature_not_enabled" + } + "###); + + // enabling metrics via HTTP results in valid request + let (response, code) = server.set_features(json!({"metrics": true})).await; + meili_snap::snapshot!(code, @"200 OK"); + meili_snap::snapshot!(response["metrics"], @"true"); + + let (response, code) = server.get_metrics().await; + meili_snap::snapshot!(code, @"200 OK"); + meili_snap::snapshot!(response, @"null"); +} + #[actix_rt::test] async fn errors() { let server = Server::new().await; @@ -73,7 +132,7 @@ async fn errors() { meili_snap::snapshot!(code, @"400 Bad Request"); meili_snap::snapshot!(meili_snap::json_string!(response), @r###" { - "message": "Unknown field `NotAFeature`: expected one of `scoreDetails`, `vectorStore`", + "message": "Unknown field `NotAFeature`: expected one of `scoreDetails`, `vectorStore`, `metrics`", "code": "bad_request", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#bad_request" From 956cfc5487135756d56156a9d152ee8fa7bd8edb Mon Sep 17 00:00:00 2001 From: bwbonanno Date: Mon, 16 Oct 2023 13:48:57 -0700 Subject: [PATCH 2/6] Add runtime check to metrics middleware --- meilisearch/src/lib.rs | 5 +--- meilisearch/src/middleware.rs | 44 ++++++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/meilisearch/src/lib.rs b/meilisearch/src/lib.rs index ef821f49f..ec6584b43 100644 --- a/meilisearch/src/lib.rs +++ b/meilisearch/src/lib.rs @@ -114,10 +114,7 @@ pub fn create_app( .configure(routes::configure) .configure(|s| dashboard(s, enable_dashboard)); - let app = app.wrap(actix_web::middleware::Condition::new( - opt.experimental_enable_metrics, - middleware::RouteMetrics, - )); + let app = app.wrap(middleware::RouteMetricsMiddlewareFactory::new(index_scheduler)); app.wrap( Cors::default() .send_wildcard() diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs index a8c981dca..b3802856d 100644 --- a/meilisearch/src/middleware.rs +++ b/meilisearch/src/middleware.rs @@ -4,15 +4,28 @@ use std::future::{ready, Ready}; use actix_web::dev::{self, Service, ServiceRequest, ServiceResponse, Transform}; use actix_web::Error; +use actix_web::web::Data; use futures_util::future::LocalBoxFuture; +use index_scheduler::IndexScheduler; use prometheus::HistogramTimer; pub struct RouteMetrics; +pub struct RouteMetricsMiddlewareFactory { + index_scheduler: Data, +} + +impl RouteMetricsMiddlewareFactory { + pub fn new(index_scheduler: Data) -> Self { + RouteMetricsMiddlewareFactory { index_scheduler } + } +} + + // Middleware factory is `Transform` trait from actix-service crate // `S` - type of the next service // `B` - type of response's body -impl Transform for RouteMetrics +impl Transform for RouteMetricsMiddlewareFactory where S: Service, Error = Error>, S::Future: 'static, @@ -25,12 +38,13 @@ where type Future = Ready>; fn new_transform(&self, service: S) -> Self::Future { - ready(Ok(RouteMetricsMiddleware { service })) + ready(Ok(RouteMetricsMiddleware { service, index_scheduler: self.index_scheduler.clone() })) } } pub struct RouteMetricsMiddleware { service: S, + index_scheduler: Data, } impl Service for RouteMetricsMiddleware @@ -47,19 +61,21 @@ where fn call(&self, req: ServiceRequest) -> Self::Future { let mut histogram_timer: Option = None; - let request_path = req.path(); - let is_registered_resource = req.resource_map().has_resource(request_path); - if is_registered_resource { - let request_method = req.method().to_string(); - histogram_timer = Some( - crate::metrics::MEILISEARCH_HTTP_RESPONSE_TIME_SECONDS + if let Ok(()) = self.index_scheduler.features().and_then(|features| features.check_metrics()) { + let request_path = req.path(); + let is_registered_resource = req.resource_map().has_resource(request_path); + if is_registered_resource { + let request_method = req.method().to_string(); + histogram_timer = Some( + crate::metrics::MEILISEARCH_HTTP_RESPONSE_TIME_SECONDS + .with_label_values(&[&request_method, request_path]) + .start_timer(), + ); + crate::metrics::MEILISEARCH_HTTP_REQUESTS_TOTAL .with_label_values(&[&request_method, request_path]) - .start_timer(), - ); - crate::metrics::MEILISEARCH_HTTP_REQUESTS_TOTAL - .with_label_values(&[&request_method, request_path]) - .inc(); - } + .inc(); + } + }; let fut = self.service.call(req); From 2b3adef7962a09f48dbb9bc5229ec0912d3b143a Mon Sep 17 00:00:00 2001 From: bwbonanno Date: Tue, 17 Oct 2023 08:17:13 -0700 Subject: [PATCH 3/6] Use index_scheduler from configured app_data in middleware --- meilisearch/src/lib.rs | 2 +- meilisearch/src/middleware.rs | 28 ++++++++++++---------------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/meilisearch/src/lib.rs b/meilisearch/src/lib.rs index ec6584b43..603d8ff86 100644 --- a/meilisearch/src/lib.rs +++ b/meilisearch/src/lib.rs @@ -114,7 +114,7 @@ pub fn create_app( .configure(routes::configure) .configure(|s| dashboard(s, enable_dashboard)); - let app = app.wrap(middleware::RouteMetricsMiddlewareFactory::new(index_scheduler)); + let app = app.wrap(middleware::RouteMetrics); app.wrap( Cors::default() .send_wildcard() diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs index b3802856d..de74b3a37 100644 --- a/meilisearch/src/middleware.rs +++ b/meilisearch/src/middleware.rs @@ -3,29 +3,18 @@ use std::future::{ready, Ready}; use actix_web::dev::{self, Service, ServiceRequest, ServiceResponse, Transform}; -use actix_web::Error; use actix_web::web::Data; +use actix_web::Error; use futures_util::future::LocalBoxFuture; use index_scheduler::IndexScheduler; use prometheus::HistogramTimer; pub struct RouteMetrics; -pub struct RouteMetricsMiddlewareFactory { - index_scheduler: Data, -} - -impl RouteMetricsMiddlewareFactory { - pub fn new(index_scheduler: Data) -> Self { - RouteMetricsMiddlewareFactory { index_scheduler } - } -} - - // Middleware factory is `Transform` trait from actix-service crate // `S` - type of the next service // `B` - type of response's body -impl Transform for RouteMetricsMiddlewareFactory +impl Transform for RouteMetrics where S: Service, Error = Error>, S::Future: 'static, @@ -38,13 +27,12 @@ where type Future = Ready>; fn new_transform(&self, service: S) -> Self::Future { - ready(Ok(RouteMetricsMiddleware { service, index_scheduler: self.index_scheduler.clone() })) + ready(Ok(RouteMetricsMiddleware { service })) } } pub struct RouteMetricsMiddleware { service: S, - index_scheduler: Data, } impl Service for RouteMetricsMiddleware @@ -61,7 +49,15 @@ where fn call(&self, req: ServiceRequest) -> Self::Future { let mut histogram_timer: Option = None; - if let Ok(()) = self.index_scheduler.features().and_then(|features| features.check_metrics()) { + let data = req.app_data::>(); + let metrics_enabled = data + .ok_or("Could not get index scheduler") + .and_then(|index_scheduler| { + index_scheduler.features().map_err(|_| "Could not get features") + }) + .and_then(|features| features.check_metrics().map_err(|_| "Metrics not enabled")); + + if let Ok(()) = metrics_enabled { let request_path = req.path(); let is_registered_resource = req.resource_map().has_resource(request_path); if is_registered_resource { From d8c649b3cd1bdd6a0be8f2525ca8a9376aa143bd Mon Sep 17 00:00:00 2001 From: bwbonanno Date: Wed, 18 Oct 2023 08:28:24 -0700 Subject: [PATCH 4/6] Return recoverable error if we fail to retrieve metrics state --- index-scheduler/src/error.rs | 4 ++++ meilisearch/src/middleware.rs | 23 +++++++++++++++-------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index ddc6960f7..417a55be8 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -113,6 +113,8 @@ pub enum Error { Dump(#[from] dump::Error), #[error(transparent)] Heed(#[from] heed::Error), + #[error("Unable to record metrics for request.")] + CannotRecordMetrics, #[error(transparent)] Milli(#[from] milli::Error), #[error("An unexpected crash occurred when processing the task.")] @@ -177,6 +179,7 @@ impl Error { | Error::TaskCancelationWithEmptyQuery | Error::Dump(_) | Error::Heed(_) + | Error::CannotRecordMetrics | Error::Milli(_) | Error::ProcessBatchPanicked | Error::FileStore(_) @@ -223,6 +226,7 @@ impl ErrorCode for Error { Error::Milli(e) => e.error_code(), Error::ProcessBatchPanicked => Code::Internal, Error::Heed(e) => e.error_code(), + Error::CannotRecordMetrics => Code::Internal, Error::HeedTransaction(e) => e.error_code(), Error::FileStore(e) => e.error_code(), Error::IoError(e) => e.error_code(), diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs index de74b3a37..7f7429186 100644 --- a/meilisearch/src/middleware.rs +++ b/meilisearch/src/middleware.rs @@ -6,7 +6,9 @@ use actix_web::dev::{self, Service, ServiceRequest, ServiceResponse, Transform}; use actix_web::web::Data; use actix_web::Error; use futures_util::future::LocalBoxFuture; +use index_scheduler::Error::CannotRecordMetrics; use index_scheduler::IndexScheduler; +use meilisearch_types::error::ResponseError; use prometheus::HistogramTimer; pub struct RouteMetrics; @@ -49,15 +51,20 @@ where fn call(&self, req: ServiceRequest) -> Self::Future { let mut histogram_timer: Option = None; - let data = req.app_data::>(); - let metrics_enabled = data - .ok_or("Could not get index scheduler") - .and_then(|index_scheduler| { - index_scheduler.features().map_err(|_| "Could not get features") - }) - .and_then(|features| features.check_metrics().map_err(|_| "Metrics not enabled")); - if let Ok(()) = metrics_enabled { + // calling unwrap here is safe because index scheduler is added to app data while creating actix app. + // also, the tests will fail if this is not present. + let index_scheduler = req.app_data::>().unwrap(); + let features = match index_scheduler.features() { + Ok(features) => features, + Err(_e) => { + return Box::pin( + async move { Err(ResponseError::from(CannotRecordMetrics).into()) }, + ); + } + }; + + if features.check_metrics().is_ok() { let request_path = req.path(); let is_registered_resource = req.resource_map().has_resource(request_path); if is_registered_resource { From dd619913da1349d3587cf864114450c01bd5551f Mon Sep 17 00:00:00 2001 From: bwbonanno Date: Thu, 19 Oct 2023 12:45:57 -0700 Subject: [PATCH 5/6] Use RwLock to never persist cli state to db --- index-scheduler/src/error.rs | 4 ++++ index-scheduler/src/features.rs | 40 ++++++++++++++++++++----------- index-scheduler/src/lib.rs | 3 +-- meilisearch/tests/features/mod.rs | 8 +++++++ 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index 417a55be8..d901129d2 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -127,6 +127,8 @@ pub enum Error { Persist(#[from] tempfile::PersistError), #[error(transparent)] FeatureNotEnabled(#[from] FeatureNotEnabledError), + #[error("An unexpected error occurred when accessing the runtime features.")] + RuntimeFeatureToggleError, #[error(transparent)] Anyhow(#[from] anyhow::Error), @@ -186,6 +188,7 @@ impl Error { | Error::IoError(_) | Error::Persist(_) | Error::FeatureNotEnabled(_) + | Error::RuntimeFeatureToggleError | Error::Anyhow(_) => true, Error::CreateBatch(_) | Error::CorruptedTaskQueue @@ -232,6 +235,7 @@ impl ErrorCode for Error { Error::IoError(e) => e.error_code(), Error::Persist(e) => e.error_code(), Error::FeatureNotEnabled(_) => Code::FeatureNotEnabled, + Error::RuntimeFeatureToggleError => Code::Internal, // Irrecoverable errors Error::Anyhow(_) => Code::Internal, diff --git a/index-scheduler/src/features.rs b/index-scheduler/src/features.rs index a9d242619..222e4a443 100644 --- a/index-scheduler/src/features.rs +++ b/index-scheduler/src/features.rs @@ -1,7 +1,10 @@ +use std::sync::{Arc, RwLock}; + use meilisearch_types::features::{InstanceTogglableFeatures, RuntimeTogglableFeatures}; use meilisearch_types::heed::types::{SerdeJson, Str}; -use meilisearch_types::heed::{Database, Env, RoTxn, RwTxn}; +use meilisearch_types::heed::{Database, Env, RwTxn}; +use crate::error::Error::RuntimeFeatureToggleError; use crate::error::FeatureNotEnabledError; use crate::Result; @@ -9,7 +12,8 @@ const EXPERIMENTAL_FEATURES: &str = "experimental-features"; #[derive(Clone)] pub(crate) struct FeatureData { - runtime: Database>, + persisted: Database>, + runtime: Arc>, } #[derive(Debug, Clone, Copy)] @@ -18,8 +22,8 @@ pub struct RoFeatures { } impl RoFeatures { - fn new(txn: RoTxn<'_>, data: &FeatureData) -> Result { - let runtime = data.runtime_features(txn)?; + fn new(data: &FeatureData) -> Result { + let runtime = data.runtime_features()?; Ok(Self { runtime }) } @@ -83,13 +87,18 @@ impl RoFeatures { impl FeatureData { pub fn new(env: &Env, instance_features: InstanceTogglableFeatures) -> Result { let mut wtxn = env.write_txn()?; - let runtime_features = env.create_database(&mut wtxn, Some(EXPERIMENTAL_FEATURES))?; - let default_features = - RuntimeTogglableFeatures { metrics: instance_features.metrics, ..Default::default() }; - runtime_features.put(&mut wtxn, EXPERIMENTAL_FEATURES, &default_features)?; + let runtime_features_db = env.create_database(&mut wtxn, Some(EXPERIMENTAL_FEATURES))?; wtxn.commit()?; - Ok(Self { runtime: runtime_features }) + let txn = env.read_txn()?; + let persisted_features: RuntimeTogglableFeatures = + runtime_features_db.get(&txn, EXPERIMENTAL_FEATURES)?.unwrap_or_default(); + let runtime = Arc::new(RwLock::new(RuntimeTogglableFeatures { + metrics: instance_features.metrics || persisted_features.metrics, + ..persisted_features + })); + + Ok(Self { persisted: runtime_features_db, runtime }) } pub fn put_runtime_features( @@ -97,16 +106,19 @@ impl FeatureData { mut wtxn: RwTxn, features: RuntimeTogglableFeatures, ) -> Result<()> { - self.runtime.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?; + self.persisted.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?; wtxn.commit()?; + + let mut toggled_features = self.runtime.write().map_err(|_| RuntimeFeatureToggleError)?; + *toggled_features = features; Ok(()) } - fn runtime_features(&self, txn: RoTxn) -> Result { - Ok(self.runtime.get(&txn, EXPERIMENTAL_FEATURES)?.unwrap_or_default()) + fn runtime_features(&self) -> Result { + Ok(*self.runtime.read().map_err(|_| RuntimeFeatureToggleError)?) } - pub fn features(&self, txn: RoTxn) -> Result { - RoFeatures::new(txn, self) + pub fn features(&self) -> Result { + RoFeatures::new(self) } } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index 0b3a5d58a..b2a263355 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -1300,8 +1300,7 @@ impl IndexScheduler { } pub fn features(&self) -> Result { - let rtxn = self.read_txn()?; - self.features.features(rtxn) + self.features.features() } pub fn put_runtime_features(&self, features: RuntimeTogglableFeatures) -> Result<()> { diff --git a/meilisearch/tests/features/mod.rs b/meilisearch/tests/features/mod.rs index 8ac73c097..abb006ac8 100644 --- a/meilisearch/tests/features/mod.rs +++ b/meilisearch/tests/features/mod.rs @@ -126,6 +126,14 @@ async fn experimental_feature_metrics() { let (response, code) = server.get_metrics().await; meili_snap::snapshot!(code, @"200 OK"); meili_snap::snapshot!(response, @"null"); + + // startup without flag respects persisted metrics value + let disable_metrics = + Opt { experimental_enable_metrics: false, ..default_settings(dir.path()) }; + let server_no_flag = Server::new_with_options(disable_metrics).await.unwrap(); + let (response, code) = server_no_flag.get_metrics().await; + meili_snap::snapshot!(code, @"200 OK"); + meili_snap::snapshot!(response, @"null"); } #[actix_rt::test] From cf8dad1ca0f182a427d9f9f47774c89605722712 Mon Sep 17 00:00:00 2001 From: Louis Dureuil Date: Mon, 23 Oct 2023 10:38:56 +0200 Subject: [PATCH 6/6] index_scheduler.features() is no longer fallible --- index-scheduler/src/batch.rs | 2 +- index-scheduler/src/error.rs | 8 ------- index-scheduler/src/features.rs | 21 ++++++++++++------- index-scheduler/src/lib.rs | 10 ++------- meilisearch/src/middleware.rs | 11 +--------- meilisearch/src/routes/features.rs | 8 +++---- .../src/routes/indexes/facet_search.rs | 2 +- meilisearch/src/routes/indexes/search.rs | 4 ++-- meilisearch/src/routes/metrics.rs | 2 +- meilisearch/src/routes/multi_search.rs | 2 +- 10 files changed, 26 insertions(+), 44 deletions(-) diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs index 48eae0063..3e2cc4281 100644 --- a/index-scheduler/src/batch.rs +++ b/index-scheduler/src/batch.rs @@ -896,7 +896,7 @@ impl IndexScheduler { })?; // 4. Dump experimental feature settings - let features = self.features()?.runtime_features(); + let features = self.features().runtime_features(); dump.create_experimental_features(features)?; let dump_uid = started_at.format(format_description!( diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index d901129d2..ddc6960f7 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -113,8 +113,6 @@ pub enum Error { Dump(#[from] dump::Error), #[error(transparent)] Heed(#[from] heed::Error), - #[error("Unable to record metrics for request.")] - CannotRecordMetrics, #[error(transparent)] Milli(#[from] milli::Error), #[error("An unexpected crash occurred when processing the task.")] @@ -127,8 +125,6 @@ pub enum Error { Persist(#[from] tempfile::PersistError), #[error(transparent)] FeatureNotEnabled(#[from] FeatureNotEnabledError), - #[error("An unexpected error occurred when accessing the runtime features.")] - RuntimeFeatureToggleError, #[error(transparent)] Anyhow(#[from] anyhow::Error), @@ -181,14 +177,12 @@ impl Error { | Error::TaskCancelationWithEmptyQuery | Error::Dump(_) | Error::Heed(_) - | Error::CannotRecordMetrics | Error::Milli(_) | Error::ProcessBatchPanicked | Error::FileStore(_) | Error::IoError(_) | Error::Persist(_) | Error::FeatureNotEnabled(_) - | Error::RuntimeFeatureToggleError | Error::Anyhow(_) => true, Error::CreateBatch(_) | Error::CorruptedTaskQueue @@ -229,13 +223,11 @@ impl ErrorCode for Error { Error::Milli(e) => e.error_code(), Error::ProcessBatchPanicked => Code::Internal, Error::Heed(e) => e.error_code(), - Error::CannotRecordMetrics => Code::Internal, Error::HeedTransaction(e) => e.error_code(), Error::FileStore(e) => e.error_code(), Error::IoError(e) => e.error_code(), Error::Persist(e) => e.error_code(), Error::FeatureNotEnabled(_) => Code::FeatureNotEnabled, - Error::RuntimeFeatureToggleError => Code::Internal, // Irrecoverable errors Error::Anyhow(_) => Code::Internal, diff --git a/index-scheduler/src/features.rs b/index-scheduler/src/features.rs index 222e4a443..1db27bcd5 100644 --- a/index-scheduler/src/features.rs +++ b/index-scheduler/src/features.rs @@ -4,7 +4,6 @@ use meilisearch_types::features::{InstanceTogglableFeatures, RuntimeTogglableFea use meilisearch_types::heed::types::{SerdeJson, Str}; use meilisearch_types::heed::{Database, Env, RwTxn}; -use crate::error::Error::RuntimeFeatureToggleError; use crate::error::FeatureNotEnabledError; use crate::Result; @@ -22,9 +21,9 @@ pub struct RoFeatures { } impl RoFeatures { - fn new(data: &FeatureData) -> Result { - let runtime = data.runtime_features()?; - Ok(Self { runtime }) + fn new(data: &FeatureData) -> Self { + let runtime = data.runtime_features(); + Self { runtime } } pub fn runtime_features(&self) -> RuntimeTogglableFeatures { @@ -109,16 +108,22 @@ impl FeatureData { self.persisted.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?; wtxn.commit()?; - let mut toggled_features = self.runtime.write().map_err(|_| RuntimeFeatureToggleError)?; + // safe to unwrap, the lock will only fail if: + // 1. requested by the same thread concurrently -> it is called and released in methods that don't call each other + // 2. there's a panic while the thread is held -> it is only used for an assignment here. + let mut toggled_features = self.runtime.write().unwrap(); *toggled_features = features; Ok(()) } - fn runtime_features(&self) -> Result { - Ok(*self.runtime.read().map_err(|_| RuntimeFeatureToggleError)?) + fn runtime_features(&self) -> RuntimeTogglableFeatures { + // sound to unwrap, the lock will only fail if: + // 1. requested by the same thread concurrently -> it is called and released in methods that don't call each other + // 2. there's a panic while the thread is held -> it is only used for copying the data here + *self.runtime.read().unwrap() } - pub fn features(&self) -> Result { + pub fn features(&self) -> RoFeatures { RoFeatures::new(self) } } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index b2a263355..43ac2355c 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -579,13 +579,7 @@ impl IndexScheduler { run.wake_up.wait(); loop { - let puffin_enabled = match run.features() { - Ok(features) => features.check_puffin().is_ok(), - Err(e) => { - log::error!("{e}"); - continue; - } - }; + let puffin_enabled = run.features().check_puffin().is_ok(); puffin::set_scopes_on(puffin_enabled); puffin::GlobalProfiler::lock().new_frame(); @@ -1299,7 +1293,7 @@ impl IndexScheduler { Ok(IndexStats { is_indexing, inner_stats: index_stats }) } - pub fn features(&self) -> Result { + pub fn features(&self) -> RoFeatures { self.features.features() } diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs index 7f7429186..5b87dee34 100644 --- a/meilisearch/src/middleware.rs +++ b/meilisearch/src/middleware.rs @@ -6,9 +6,7 @@ use actix_web::dev::{self, Service, ServiceRequest, ServiceResponse, Transform}; use actix_web::web::Data; use actix_web::Error; use futures_util::future::LocalBoxFuture; -use index_scheduler::Error::CannotRecordMetrics; use index_scheduler::IndexScheduler; -use meilisearch_types::error::ResponseError; use prometheus::HistogramTimer; pub struct RouteMetrics; @@ -55,14 +53,7 @@ where // calling unwrap here is safe because index scheduler is added to app data while creating actix app. // also, the tests will fail if this is not present. let index_scheduler = req.app_data::>().unwrap(); - let features = match index_scheduler.features() { - Ok(features) => features, - Err(_e) => { - return Box::pin( - async move { Err(ResponseError::from(CannotRecordMetrics).into()) }, - ); - } - }; + let features = index_scheduler.features(); if features.check_metrics().is_ok() { let request_path = req.path(); diff --git a/meilisearch/src/routes/features.rs b/meilisearch/src/routes/features.rs index 4437f602d..e7fd8de22 100644 --- a/meilisearch/src/routes/features.rs +++ b/meilisearch/src/routes/features.rs @@ -29,12 +29,12 @@ async fn get_features( >, req: HttpRequest, analytics: Data, -) -> Result { - let features = index_scheduler.features()?; +) -> HttpResponse { + let features = index_scheduler.features(); analytics.publish("Experimental features Seen".to_string(), json!(null), Some(&req)); debug!("returns: {:?}", features.runtime_features()); - Ok(HttpResponse::Ok().json(features.runtime_features())) + HttpResponse::Ok().json(features.runtime_features()) } #[derive(Debug, Deserr)] @@ -59,7 +59,7 @@ async fn patch_features( req: HttpRequest, analytics: Data, ) -> Result { - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let old_features = features.runtime_features(); let new_features = meilisearch_types::features::RuntimeTogglableFeatures { diff --git a/meilisearch/src/routes/indexes/facet_search.rs b/meilisearch/src/routes/indexes/facet_search.rs index 831c45a85..142a424c0 100644 --- a/meilisearch/src/routes/indexes/facet_search.rs +++ b/meilisearch/src/routes/indexes/facet_search.rs @@ -68,7 +68,7 @@ pub async fn search( } let index = index_scheduler.index(&index_uid)?; - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let search_result = tokio::task::spawn_blocking(move || { perform_facet_search(&index, search_query, facet_query, facet_name, features) }) diff --git a/meilisearch/src/routes/indexes/search.rs b/meilisearch/src/routes/indexes/search.rs index 3eabf61f3..5a0a9e92b 100644 --- a/meilisearch/src/routes/indexes/search.rs +++ b/meilisearch/src/routes/indexes/search.rs @@ -157,7 +157,7 @@ pub async fn search_with_url_query( let mut aggregate = SearchAggregator::from_query(&query, &req); let index = index_scheduler.index(&index_uid)?; - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let search_result = tokio::task::spawn_blocking(move || perform_search(&index, query, features)).await?; if let Ok(ref search_result) = search_result { @@ -192,7 +192,7 @@ pub async fn search_with_post( let index = index_scheduler.index(&index_uid)?; - let features = index_scheduler.features()?; + let features = index_scheduler.features(); let search_result = tokio::task::spawn_blocking(move || perform_search(&index, query, features)).await?; if let Ok(ref search_result) = search_result { diff --git a/meilisearch/src/routes/metrics.rs b/meilisearch/src/routes/metrics.rs index af1f2b536..7a13a758f 100644 --- a/meilisearch/src/routes/metrics.rs +++ b/meilisearch/src/routes/metrics.rs @@ -19,7 +19,7 @@ pub async fn get_metrics( index_scheduler: GuardedData, Data>, auth_controller: Data, ) -> Result { - index_scheduler.features()?.check_metrics()?; + index_scheduler.features().check_metrics()?; let auth_filters = index_scheduler.filters(); if !auth_filters.all_indexes_authorized() { let mut error = ResponseError::from(AuthenticationError::InvalidToken); diff --git a/meilisearch/src/routes/multi_search.rs b/meilisearch/src/routes/multi_search.rs index e257af1cf..3a028022a 100644 --- a/meilisearch/src/routes/multi_search.rs +++ b/meilisearch/src/routes/multi_search.rs @@ -41,7 +41,7 @@ pub async fn multi_search_with_post( let queries = params.into_inner().queries; let mut multi_aggregate = MultiSearchAggregator::from_queries(&queries, &req); - let features = index_scheduler.features()?; + let features = index_scheduler.features(); // Explicitly expect a `(ResponseError, usize)` for the error type rather than `ResponseError` only, // so that `?` doesn't work if it doesn't use `with_index`, ensuring that it is not forgotten in case of code