diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs index 48eae0063d..3e2cc42810 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/features.rs b/index-scheduler/src/features.rs index e78a41b5b0..1db27bcd5c 100644 --- a/index-scheduler/src/features.rs +++ b/index-scheduler/src/features.rs @@ -1,6 +1,8 @@ +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::FeatureNotEnabledError; use crate::Result; @@ -9,20 +11,19 @@ const EXPERIMENTAL_FEATURES: &str = "experimental-features"; #[derive(Clone)] pub(crate) struct FeatureData { - runtime: Database>, - instance: InstanceTogglableFeatures, + persisted: Database>, + runtime: Arc>, } #[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 }) + fn new(data: &FeatureData) -> Self { + let runtime = data.runtime_features(); + Self { runtime } } pub fn runtime_features(&self) -> RuntimeTogglableFeatures { @@ -43,7 +44,7 @@ impl RoFeatures { } pub fn check_metrics(&self) -> Result<()> { - if self.instance.metrics { + if self.runtime.metrics { Ok(()) } else { Err(FeatureNotEnabledError { @@ -85,10 +86,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 runtime_features_db = env.create_database(&mut wtxn, Some(EXPERIMENTAL_FEATURES))?; wtxn.commit()?; - Ok(Self { runtime: runtime_features, instance: instance_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( @@ -96,16 +105,25 @@ 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()?; + + // 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, txn: RoTxn) -> Result { - Ok(self.runtime.get(&txn, EXPERIMENTAL_FEATURES)?.unwrap_or_default()) + 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, txn: RoTxn) -> Result { - RoFeatures::new(txn, self) + pub fn features(&self) -> RoFeatures { + RoFeatures::new(self) } } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index 0b3a5d58af..43ac2355c4 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,9 +1293,8 @@ impl IndexScheduler { Ok(IndexStats { is_indexing, inner_stats: index_stats }) } - pub fn features(&self) -> Result { - let rtxn = self.read_txn()?; - self.features.features(rtxn) + pub fn features(&self) -> RoFeatures { + self.features.features() } pub fn put_runtime_features(&self, features: RuntimeTogglableFeatures) -> Result<()> { diff --git a/meilisearch-types/src/features.rs b/meilisearch-types/src/features.rs index a8a3646f60..33afe2d244 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, pub export_puffin_reports: bool, } diff --git a/meilisearch/src/lib.rs b/meilisearch/src/lib.rs index ef821f49f5..603d8ff865 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::RouteMetrics); app.wrap( Cors::default() .send_wildcard() diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs index a8c981dca2..5b87dee347 100644 --- a/meilisearch/src/middleware.rs +++ b/meilisearch/src/middleware.rs @@ -3,8 +3,10 @@ use std::future::{ready, Ready}; 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::IndexScheduler; use prometheus::HistogramTimer; pub struct RouteMetrics; @@ -47,19 +49,27 @@ 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 + + // 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 = index_scheduler.features(); + + 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 { + 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); diff --git a/meilisearch/src/routes/features.rs b/meilisearch/src/routes/features.rs index 6f68b5b3c2..e7fd8de22b 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)] @@ -45,6 +45,8 @@ pub struct RuntimeTogglableFeatures { #[deserr(default)] pub vector_store: Option, #[deserr(default)] + pub metrics: Option, + #[deserr(default)] pub export_puffin_reports: Option, } @@ -57,12 +59,13 @@ 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 { 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), export_puffin_reports: new_features .0 .export_puffin_reports @@ -75,6 +78,7 @@ async fn patch_features( let meilisearch_types::features::RuntimeTogglableFeatures { score_details, vector_store, + metrics, export_puffin_reports, } = new_features; @@ -83,6 +87,7 @@ async fn patch_features( json!({ "score_details": score_details, "vector_store": vector_store, + "metrics": metrics, "export_puffin_reports": export_puffin_reports, }), Some(&req), diff --git a/meilisearch/src/routes/indexes/facet_search.rs b/meilisearch/src/routes/indexes/facet_search.rs index 831c45a855..142a424c06 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 3eabf61f3a..5a0a9e92b2 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 af1f2b536d..7a13a758f0 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 e257af1cf3..3a028022a0 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 diff --git a/meilisearch/tests/auth/authorization.rs b/meilisearch/tests/auth/authorization.rs index 883c232675..af028060db 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 58f561eb83..27feb187f1 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 bc3d4e7983..abb006ac81 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. @@ -17,6 +20,7 @@ async fn experimental_features() { { "scoreDetails": false, "vectorStore": false, + "metrics": false, "exportPuffinReports": false } "###); @@ -28,6 +32,7 @@ async fn experimental_features() { { "scoreDetails": false, "vectorStore": true, + "metrics": false, "exportPuffinReports": false } "###); @@ -39,6 +44,7 @@ async fn experimental_features() { { "scoreDetails": false, "vectorStore": true, + "metrics": false, "exportPuffinReports": false } "###); @@ -51,6 +57,7 @@ async fn experimental_features() { { "scoreDetails": false, "vectorStore": true, + "metrics": false, "exportPuffinReports": false } "###); @@ -63,11 +70,72 @@ async fn experimental_features() { { "scoreDetails": false, "vectorStore": true, + "metrics": false, "exportPuffinReports": 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, + "exportPuffinReports": false + } + "###); + + 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/product/discussions/625", + "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"); + + // 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] async fn errors() { let server = Server::new().await; @@ -78,7 +146,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`, `exportPuffinReports`", + "message": "Unknown field `NotAFeature`: expected one of `scoreDetails`, `vectorStore`, `metrics`, `exportPuffinReports`", "code": "bad_request", "type": "invalid_request", "link": "https://docs.meilisearch.com/errors#bad_request"