From 00145888e7641df2abbcf3522daa4a80f855f049 Mon Sep 17 00:00:00 2001 From: Ameer Ghani Date: Wed, 1 Nov 2023 18:46:51 -0500 Subject: [PATCH] Use simpler body for trace config endpoint (#2208) (#2216) --- aggregator/src/binary_utils.rs | 88 +++++++++++----------------------- docs/DEPLOYING.md | 6 +-- 2 files changed, 31 insertions(+), 63 deletions(-) diff --git a/aggregator/src/binary_utils.rs b/aggregator/src/binary_utils.rs index 92f6733ad..d9232c786 100644 --- a/aggregator/src/binary_utils.rs +++ b/aggregator/src/binary_utils.rs @@ -18,7 +18,6 @@ use janus_aggregator_core::datastore::{Crypter, Datastore}; use janus_core::time::Clock; use opentelemetry::metrics::Meter; use ring::aead::{LessSafeKey, UnboundKey, AES_128_GCM}; -use serde::{Deserialize, Serialize}; use std::{ fmt::{self, Debug, Formatter}, fs, @@ -35,7 +34,7 @@ use tokio_postgres::NoTls; use tracing::{debug, info}; use tracing_subscriber::EnvFilter; use trillium::{Handler, Headers, Info, Init, Status}; -use trillium_api::{api, Json, State}; +use trillium_api::{api, State}; use trillium_head::Head; use trillium_router::Router; use trillium_tokio::Stopper; @@ -364,15 +363,13 @@ fn zpages_handler(trace_reload_handle: TraceReloadHandle) -> impl Handler { async fn get_traceconfigz( conn: &mut trillium::Conn, State(trace_reload_handle): State>, -) -> Result, Status> { - Ok(Json(TraceconfigzBody { - filter: trace_reload_handle - .with_current(|trace_filter| trace_filter.to_string()) - .map_err(|err| { - conn.set_body(format!("failed to get current filter: {err}")); - Status::InternalServerError - })?, - })) +) -> Result { + trace_reload_handle + .with_current(|trace_filter| trace_filter.to_string()) + .map_err(|err| { + conn.set_body(format!("failed to get current filter: {err}")); + Status::InternalServerError + }) } /// Allows modifying the runtime tracing filter. Accepts a request with a body corresponding to @@ -380,12 +377,9 @@ async fn get_traceconfigz( /// See [`EnvFilter::try_new`] for details. async fn put_traceconfigz( conn: &mut trillium::Conn, - (State(trace_reload_handle), Json(req)): ( - State>, - Json, - ), -) -> Result, Status> { - let new_filter = EnvFilter::try_new(req.filter).map_err(|err| { + (State(trace_reload_handle), request): (State>, String), +) -> Result { + let new_filter = EnvFilter::try_new(request).map_err(|err| { conn.set_body(format!("invalid filter: {err}")); Status::BadRequest })?; @@ -393,22 +387,12 @@ async fn put_traceconfigz( conn.set_body(format!("failed to update filter: {err}")); Status::InternalServerError })?; - Ok(Json(TraceconfigzBody { - filter: trace_reload_handle - .with_current(|trace_filter| trace_filter.to_string()) - .map_err(|err| { - conn.set_body(format!("failed to get current filter: {err}")); - Status::InternalServerError - })?, - })) -} - -/// The response and request body used by /traceconfigz for reporting and updating its configuration. -#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] -struct TraceconfigzBody { - /// The directive that filters spans and events. This field follows the [`EnvFilter`][1] syntax. - /// [1]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html#directives - filter: String, + trace_reload_handle + .with_current(|trace_filter| trace_filter.to_string()) + .map_err(|err| { + conn.set_body(format!("failed to get current filter: {err}")); + Status::InternalServerError + }) } /// Register a signal handler for SIGTERM, and stop the [`Stopper`] when a SIGTERM signal is @@ -473,8 +457,7 @@ pub async fn setup_server( mod tests { use super::CommonBinaryOptions; use crate::{ - aggregator::http_handlers::test_util::take_response_body, - binary_utils::{zpages_handler, TraceconfigzBody}, + aggregator::http_handlers::test_util::take_response_body, binary_utils::zpages_handler, }; use clap::CommandFactory; use tracing_subscriber::{reload, EnvFilter}; @@ -503,36 +486,24 @@ mod tests { let mut test_conn = get("/traceconfigz").run_async(&handler).await; assert_eq!(test_conn.status(), Some(Status::Ok)); assert_eq!( - serde_json::from_slice::(&take_response_body(&mut test_conn).await) - .unwrap(), - TraceconfigzBody { - filter: "info".to_string() - } + String::from_utf8_lossy(&take_response_body(&mut test_conn).await), + "info", ); - let req = TraceconfigzBody { - filter: "debug".to_string(), - }; let mut test_conn = put("/traceconfigz") - .with_request_body(serde_json::to_vec(&req).unwrap()) + .with_request_body("debug") .run_async(&handler) .await; assert_eq!(test_conn.status(), Some(Status::Ok)); assert_eq!( - serde_json::from_slice::(&take_response_body(&mut test_conn).await) - .unwrap(), - req, + String::from_utf8_lossy(&take_response_body(&mut test_conn).await), + "debug", ); - let req = TraceconfigzBody { - filter: "!(#*$#@)".to_string(), - }; - let mut test_conn = dbg!( - put("/traceconfigz") - .with_request_body(serde_json::to_vec(&req).unwrap()) - .run_async(&handler) - .await - ); + let mut test_conn = put("/traceconfigz") + .with_request_body("!@($*$#)") + .run_async(&handler) + .await; assert_eq!(test_conn.status(), Some(Status::BadRequest)); assert!( String::from_utf8_lossy(&take_response_body(&mut test_conn).await) @@ -553,11 +524,8 @@ mod tests { .starts_with("failed to get current filter:") ); - let req = TraceconfigzBody { - filter: "debug".to_string(), - }; let mut test_conn = put("/traceconfigz") - .with_request_body(serde_json::to_vec(&req).unwrap()) + .with_request_body("debug") .run_async(&handler) .await; assert_eq!(test_conn.status(), Some(Status::InternalServerError)); diff --git a/docs/DEPLOYING.md b/docs/DEPLOYING.md index ab282d8e9..04d952a30 100644 --- a/docs/DEPLOYING.md +++ b/docs/DEPLOYING.md @@ -56,10 +56,10 @@ an example using this with `curl`: $ HEALTH_CHECK_LISTEN_ADDRESS=http://localhost:8000 $ curl $HEALTH_CHECK_LISTEN_ADDRESS/traceconfigz -{"filter":"info"} +info -$ curl $HEALTH_CHECK_LISTEN_ADDRESS/traceconfigz -X PUT -d '{"filter":"debug"}' -{"filter":"debug"} +$ curl $HEALTH_CHECK_LISTEN_ADDRESS/traceconfigz -X PUT -d 'debug' +debug ``` The `filter` field corresponds exactly to the [EnvFilter] format.