XXX more features, more example exercising things

Author: nshalman

dropshot/examples/otel.rs

@@ -68,6 +68,7 @@ async fn main() -> Result<(), String> {
     api.register(example_api_put_counter).unwrap();
     api.register(example_api_get).unwrap();
     api.register(example_api_error).unwrap();
+    api.register(example_api_panic).unwrap();
 
     // The functions that implement our API endpoints will share this context.
     let api_context = ExampleContext::new();
@@ -201,6 +202,9 @@ async fn example_api_get(
     let req = traced_request("http://localhost:4000/error", &cx).await;
     let _res = client.request(req).await;
 
+    let req = traced_request("http://localhost:4000/panic", &cx).await;
+    let _res = client.request(req).await;
+
     let api_context = rqctx.context();
     Ok(HttpResponseOk(CounterValue {
         counter: api_context.counter.load(Ordering::SeqCst),
@@ -231,6 +235,26 @@ async fn example_api_error(
     _rqctx: RequestContext<ExampleContext>,
 ) -> Result<HttpResponseOk<CounterValue>, HttpError> {
     //XXX Why does this create a 499 rather than a 500 error???
+    // This feels like a bug in dropshot. As a downstream consumer
+    // I just want anything blowing up in my handler to be a somewhat useful 500 error.
+    // It does help that the compiler is strict about what can otherwise be returned...
+    //panic!("This handler is totally broken!");
+
+    Err(HttpError::for_internal_error("This endpoint is broken".to_string()))
+}
+
+/// This endpoint panics!
+#[endpoint {
+    method = GET,
+    path = "/panic",
+}]
+async fn example_api_panic(
+    _rqctx: RequestContext<ExampleContext>,
+) -> Result<HttpResponseOk<CounterValue>, HttpError> {
+    //XXX Why does this create a 499 rather than a 500 error???
+    // This feels like a bug in dropshot. As a downstream consumer
+    // I just want anything blowing up in my handler to be a somewhat useful 500 error.
+    // It does help that the compiler is strict about what can otherwise be returned...
     panic!("This handler is totally broken!");
 }
 

dropshot/src/otel.rs

@@ -43,13 +43,10 @@ pub(crate) struct RequestInfo {
 // - error.message
 // - error.cause_chain
 // - otel.status_code
-#[derive(Debug, Clone, serde::Serialize)]
-pub(crate) struct ResponseInfo {
-    pub id: String,
-    pub local_addr: std::net::SocketAddr,
-    pub remote_addr: std::net::SocketAddr,
+pub(crate) struct ResponseInfo<'a> {
     pub status_code: u16,
     pub message: String,
+    pub error: Option<&'a crate::handler::HandlerError>,
 }
 
 fn extract_context_from_request(
@@ -60,8 +57,6 @@ fn extract_context_from_request(
     })
 }
 
-// - otel.kind
-// - otel.name
 pub fn create_request_span(
     request: &hyper::Request<hyper::body::Incoming>,
 ) -> opentelemetry::global::BoxedSpan {
@@ -74,7 +69,7 @@ pub fn create_request_span(
     let tracer = tracer_provider.tracer_with_scope(scope);
     let parent_cx = extract_context_from_request(&request);
     tracer
-        .span_builder("dropshot_request") //XXX Fixme
+        .span_builder("dropshot_request") // TODO? Naming is hard.
         .with_kind(opentelemetry::trace::SpanKind::Server)
         .start_with_context(&tracer, &parent_cx)
 }
@@ -124,6 +119,10 @@ impl TraceDropshot for opentelemetry::global::BoxedSpan {
                 response.message,
             ),
         ]);
-        //XXX if this is a 5xx error, mark the span as an error
+        if let Some(error) = response.error{
+            self.set_status(
+                opentelemetry::trace::Status::error(error.internal_message().clone())
+                );
+        }
     }
 }

dropshot/src/server.rs

@@ -814,7 +814,7 @@ async fn http_request_handle_wrap<C: ServerContext>(
 
     // Copy local address to report later during the finish probe, as the
     // server is passed by value to the request handler function.
-    #[cfg(any(feature = "usdt-probes", feature = "otel-tracing"))]
+    #[cfg(feature = "usdt-probes")]
     let local_addr = server.local_addr;
 
     #[cfg(feature = "otel-tracing")]
@@ -831,14 +831,12 @@ async fn http_request_handle_wrap<C: ServerContext>(
 
         #[cfg(feature = "otel-tracing")]
         span.trace_response(crate::otel::ResponseInfo {
-            id: request_id.clone(),
-            local_addr,
-            remote_addr,
             // 499 is a non-standard code popularized by nginx to mean "client disconnected".
             status_code: 499,
             message: String::from(
                 "client disconnected before response returned",
             ),
+            error: None,
         });
 
         #[cfg(feature = "usdt-probes")]
@@ -851,6 +849,7 @@ async fn http_request_handle_wrap<C: ServerContext>(
                 status_code: 499,
                 message: String::from(
                     "client disconnected before response returned",
+                error: None,
                 ),
             }
         });
@@ -881,13 +880,11 @@ async fn http_request_handle_wrap<C: ServerContext>(
 
                 #[cfg(feature = "otel-tracing")]
                 span.trace_response(crate::otel::ResponseInfo {
-                    id: request_id.clone(),
-                    local_addr,
-                    remote_addr,
                     status_code: status.as_u16(),
                     message: message_external
                         .cloned()
                         .unwrap_or_else(|| message_internal.clone()),
+                    error: Some(&error),
                 });
 
                 #[cfg(feature = "usdt-probes")]
@@ -923,11 +920,9 @@ async fn http_request_handle_wrap<C: ServerContext>(
 
             #[cfg(feature = "otel-tracing")]
             span.trace_response(crate::otel::ResponseInfo {
-                id: request_id.parse().unwrap(),
-                local_addr,
-                remote_addr,
                 status_code: response.status().as_u16(),
                 message: "".to_string(),
+                error: None,
             });
 
             #[cfg(feature = "usdt-probes")]