Merge pull request 'web_server.rs: Added bucket domain to observability' (#608) from jpds/garage:domain-web-requests into main

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/608

4 files changed, 48 insertions, 15 deletions

diff --git a/doc/book/reference-manual/configuration.md b/doc/book/reference-manual/configuration.md
index f0a3b438..f545de29 100644
--- a/doc/book/reference-manual/configuration.md
+++ b/doc/book/reference-manual/configuration.md
@@ -75,6 +75,7 @@ root_domain = ".s3.garage"
 [s3_web]
 bind_addr = "[::]:3902"
 root_domain = ".web.garage"
+add_host_to_metrics = true
 
 [admin]
 api_bind_addr = "0.0.0.0:3903"
@@ -138,6 +139,7 @@ The `[s3_api]` section:
 [`s3_region`](#s3_region).
 
 The `[s3_web]` section:
+[`add_host_to_metrics`](#web_add_host_to_metrics),
 [`bind_addr`](#web_bind_addr),
 [`root_domain`](#web_root_domain).
 
@@ -744,6 +746,13 @@ For instance, if `root_domain` is `web.garage.eu`, a bucket called `deuxfleurs.f
 will be accessible either with hostname `deuxfleurs.fr.web.garage.eu`
 or with hostname `deuxfleurs.fr`.
 
+#### `add_host_to_metrics` {#web_add_host_to_metrics}
+
+Whether to include the requested domain name (HTTP `Host` header) in the
+Prometheus metrics of the web endpoint. This is disabled by default as the
+number of possible values is not bounded and can be a source of cardinality
+explosion in the exported metrics.
+
 
 ### The `[admin]` section
 
diff --git a/src/garage/server.rs b/src/garage/server.rs
index 9e58fa6d..1dc86fd3 100644
--- a/src/garage/server.rs
+++ b/src/garage/server.rs
@@ -113,7 +113,7 @@ pub async fn run_server(config_file: PathBuf, secrets: Secrets) -> Result<(), Er
 
 	if let Some(web_config) = &config.s3_web {
 		info!("Initializing web server...");
-		let web_server = WebServer::new(garage.clone(), web_config.root_domain.clone());
+		let web_server = WebServer::new(garage.clone(), &web_config);
 		servers.push((
 			"Web",
 			tokio::spawn(web_server.run(web_config.bind_addr.clone(), watch_cancel.clone())),
diff --git a/src/util/config.rs b/src/util/config.rs
index b4e2b008..73fc4ff4 100644
--- a/src/util/config.rs
+++ b/src/util/config.rs
@@ -183,6 +183,9 @@ pub struct WebConfig {
 	pub bind_addr: UnixOrTCPSocketAddress,
 	/// Suffix to remove from domain name to find bucket
 	pub root_domain: String,
+	/// Whether to add the requested domain to exported Prometheus metrics
+	#[serde(default)]
+	pub add_host_to_metrics: bool,
 }
 
 /// Configuration for the admin and monitoring HTTP API
diff --git a/src/web/web_server.rs b/src/web/web_server.rs
index 48dcb5b1..e73dab48 100644
--- a/src/web/web_server.rs
+++ b/src/web/web_server.rs
@@ -33,6 +33,7 @@ use garage_api_s3::get::{handle_get_without_ctx, handle_head_without_ctx};
 use garage_model::garage::Garage;
 
 use garage_table::*;
+use garage_util::config::WebConfig;
 use garage_util::data::Uuid;
 use garage_util::error::Error as GarageError;
 use garage_util::forwarded_headers;
@@ -69,16 +70,18 @@ pub struct WebServer {
 	garage: Arc<Garage>,
 	metrics: Arc<WebMetrics>,
 	root_domain: String,
+	add_host_to_metrics: bool,
 }
 
 impl WebServer {
 	/// Run a web server
-	pub fn new(garage: Arc<Garage>, root_domain: String) -> Arc<Self> {
+	pub fn new(garage: Arc<Garage>, config: &WebConfig) -> Arc<Self> {
 		let metrics = Arc::new(WebMetrics::new());
 		Arc::new(WebServer {
 			garage,
 			metrics,
-			root_domain,
+			root_domain: config.root_domain.clone(),
+			add_host_to_metrics: config.add_host_to_metrics,
 		})
 	}
 
@@ -120,18 +123,27 @@ impl WebServer {
 		req: Request<IncomingBody>,
 		addr: String,
 	) -> Result<Response<BoxBody<Error>>, http::Error> {
+		let host_header = req
+			.headers()
+			.get(HOST)
+			.and_then(|x| x.to_str().ok())
+			.unwrap_or("<unknown>")
+			.to_string();
+
 		if let Ok(forwarded_for_ip_addr) =
 			forwarded_headers::handle_forwarded_for_headers(req.headers())
 		{
+			// uri() below has a preceding '/', so no space with host
 			info!(
-				"{} (via {}) {} {}",
+				"{} (via {}) {} {}{}",
 				forwarded_for_ip_addr,
 				addr,
 				req.method(),
+				host_header,
 				req.uri()
 			);
 		} else {
-			info!("{} {} {}", addr, req.method(), req.uri());
+			info!("{} {} {}{}", addr, req.method(), host_header, req.uri());
 		}
 
 		// Lots of instrumentation
@@ -140,12 +152,16 @@ impl WebServer {
 			.span_builder(format!("Web {} request", req.method()))
 			.with_trace_id(gen_trace_id())
 			.with_attributes(vec![
+				KeyValue::new("host", format!("{}", host_header.clone())),
 				KeyValue::new("method", format!("{}", req.method())),
 				KeyValue::new("uri", req.uri().to_string()),
 			])
 			.start(&tracer);
 
-		let metrics_tags = &[KeyValue::new("method", req.method().to_string())];
+		let mut metrics_tags = vec![KeyValue::new("method", req.method().to_string())];
+		if self.add_host_to_metrics {
+			metrics_tags.push(KeyValue::new("host", host_header.clone()));
+		}
 
 		// The actual handler
 		let res = self
@@ -160,25 +176,30 @@ impl WebServer {
 		// Returning the result
 		match res {
 			Ok(res) => {
-				debug!("{} {} {}", req.method(), res.status(), req.uri());
+				debug!(
+					"{} {} {}{}",
+					req.method(),
+					res.status(),
+					host_header,
+					req.uri()
+				);
 				Ok(res
 					.map(|body| BoxBody::new(http_body_util::BodyExt::map_err(body, Error::from))))
 			}
 			Err(error) => {
 				info!(
-					"{} {} {} {}",
+					"{} {} {}{} {}",
 					req.method(),
 					error.http_status_code(),
+					host_header,
 					req.uri(),
 					error
 				);
-				self.metrics.error_counter.add(
-					1,
-					&[
-						metrics_tags[0].clone(),
-						KeyValue::new("status_code", error.http_status_code().to_string()),
-					],
-				);
+				metrics_tags.push(KeyValue::new(
+					"status_code",
+					error.http_status_code().to_string(),
+				));
+				self.metrics.error_counter.add(1, &metrics_tags);
 				Ok(error_to_res(error))
 			}
 		}