Use simpler body for trace config endpoint (#2208) (#2216)

divviup · Nov 1, 2023 · 0014588 · 0014588
1 parent d152ea6
commit 0014588
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 63 deletions.
diff --git 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,51 +363,36 @@ fn zpages_handler(trace_reload_handle: TraceReloadHandle) -> impl Handler {
 async fn get_traceconfigz(
     conn: &mut trillium::Conn,
     State(trace_reload_handle): State<Arc<TraceReloadHandle>>,
-) -> Result<Json<TraceconfigzBody>, 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<String, Status> {
+    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
 /// [`TraceconfigzBody`]. If the `filter` field is empty, the filter will fallback to `error`.
 /// See [`EnvFilter::try_new`] for details.
 async fn put_traceconfigz(
     conn: &mut trillium::Conn,
-    (State(trace_reload_handle), Json(req)): (
-        State<Arc<TraceReloadHandle>>,
-        Json<TraceconfigzBody>,
-    ),
-) -> Result<Json<TraceconfigzBody>, Status> {
-    let new_filter = EnvFilter::try_new(req.filter).map_err(|err| {
+    (State(trace_reload_handle), request): (State<Arc<TraceReloadHandle>>, String),
+) -> Result<String, Status> {
+    let new_filter = EnvFilter::try_new(request).map_err(|err| {
         conn.set_body(format!("invalid filter: {err}"));
         Status::BadRequest
     })?;
     trace_reload_handle.reload(new_filter).map_err(|err| {
         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::<TraceconfigzBody>(&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::<TraceconfigzBody>(&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
@@ -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.