XXX Better code documentation

Author: nshalman

dropshot/examples/multiple-servers.rs

@@ -100,6 +100,10 @@ use tokio::sync::Mutex;
 
 #[tokio::main]
 async fn main() -> Result<(), String> {
+    // XXX Is there interest in adding the optional integration into some of the existing examples?
+    #[cfg(feature = "otel-tracing")]
+    let _otel_guard = equinix_otel_tools::init(env!("CARGO_CRATE_NAME"));
+
     // Initial set of servers to start. Once they're running, we may add or
     // remove servers based on client requests.
     let initial_servers = [("A", "127.0.0.1:12345"), ("B", "127.0.0.1:12346")];

dropshot/examples/otel.rs

@@ -46,11 +46,8 @@ use opentelemetry_http::{Bytes, HeaderInjector};
 async fn main() -> Result<(), String> {
     let _otel_guard = equinix_otel_tools::init("otel-demo");
 
-    let config_dropshot: ConfigDropshot = ConfigDropshot {
-        bind_address: std::net::SocketAddr::new(
-            std::net::IpAddr::V4(std::net::Ipv4Addr::new(127, 0, 0, 1)),
-            4000,
-        ),
+    let config_dropshot = ConfigDropshot {
+        bind_address: "127.0.0.1:4000".parse().unwrap(),
         ..Default::default()
     };
 
@@ -248,6 +245,7 @@ async fn example_api_error(
     method = GET,
     path = "/panic",
 }]
+#[cfg_attr(feature = "tokio-tracing", tracing::instrument(skip_all, err))]
 async fn example_api_panic(
     _rqctx: RequestContext<ExampleContext>,
 ) -> Result<HttpResponseOk<CounterValue>, HttpError> {
@@ -264,7 +262,7 @@ async fn example_api_panic(
     method = PUT,
     path = "/counter",
 }]
-#[cfg_attr(feature = "tokio-tracing", tracing::instrument)]
+#[cfg_attr(feature = "tokio-tracing", tracing::instrument(skip_all, err))]
 async fn example_api_put_counter(
     rqctx: RequestContext<ExampleContext>,
     update: TypedBody<CounterValue>,

dropshot/src/otel.rs

@@ -1,33 +1,35 @@
 // Copyright 2024 Oxide Computer Company
 //! Opentelemetry tracing support
 //!
+// XXX Not sure if we want to just mimic reqwest-tracing
+// or not...
 //! Fields that we want to produce to provide comparable
-//! functionality to reqwest-tracing[1]:
+//! functionality to [reqwest-tracing]:
 //!
-//! - http.request.method
-//! - url.scheme
-//! - server.address
-//! - server.port
-//! - otel.kind
-//! - otel.name
-//! - otel.status_code
-//! - user_agent.original
-//! - http.response.status_code
-//! - error.message
-//! - error.cause_chain
+//! - [ ] error.cause_chain
+//! - [ ] error.message
+//! - [x] http.request.method
+//! - [x] http.response.status_code
+//! - [x] otel.kind (showing up as span.kind?)
+//! - [x] otel.name
+//! - [ ] otel.status_code (needs investigation)
+//! - [x] server.address
+//! - [x] server.port
+//! - [ ] url.scheme
+//! - [x] user_agent.original
 //!
-//! [1] <https://docs.rs/reqwest-tracing/0.5.4/reqwest_tracing/macro.reqwest_otel_span.html>
+//! Where possible we use the [opentelemetry-semantic-conventions] crate for naming.
+//!
+//! [reqwest-tracing]: https://docs.rs/reqwest-tracing/0.5.4/reqwest_tracing/macro.reqwest_otel_span.html
+//! [opentelemetry-semantic-conventions]:
+//! <https://docs.rs/opentelemetry-semantic-conventions/latest/opentelemetry_semantic_conventions/trace/index.html>
 
 use opentelemetry::{
     global, trace::Span, trace::Tracer, trace::TracerProvider,
 };
 use opentelemetry_http::HeaderExtractor;
 use opentelemetry_semantic_conventions::trace;
 
-// - url.scheme
-// - server.address
-// - server.port
-// - user_agent.original
 #[derive(Debug, Clone, serde::Serialize)]
 pub(crate) struct RequestInfo {
     pub id: String,
@@ -39,16 +41,14 @@ pub(crate) struct RequestInfo {
     pub user_agent: String,
 }
 
-// - http.response.status_code
-// - error.message
-// - error.cause_chain
-// - otel.status_code
 pub(crate) struct ResponseInfo<'a> {
     pub status_code: u16,
     pub message: String,
     pub error: Option<&'a crate::handler::HandlerError>,
 }
 
+/// Generate an opentelementry Context based on the headers
+/// in the incoming hyper request.
 fn extract_context_from_request(
     request: &hyper::Request<hyper::body::Incoming>,
 ) -> opentelemetry::Context {
@@ -57,6 +57,7 @@ fn extract_context_from_request(
     })
 }
 
+/// Create an opentelemetry Span to represent the handling of the incoming request.
 pub fn create_request_span(
     request: &hyper::Request<hyper::body::Incoming>,
 ) -> opentelemetry::global::BoxedSpan {
@@ -74,8 +75,14 @@ pub fn create_request_span(
         .start_with_context(&tracer, &parent_cx)
 }
 
+/// This trait contains all functionality needed for dropshot to annotate
+/// opentelemetry spans with information about a request and the corresponding response.
 pub trait TraceDropshot {
+    /// Attach attributes to the span based on the provided
+    /// RequestInfo
     fn trace_request(&mut self, request: RequestInfo);
+    /// Attach the information from the ResponseInfo to the span
+    /// including marking the span as an error if an error occurred.
     fn trace_response(&mut self, response: ResponseInfo);
 }
 
@@ -84,30 +91,28 @@ impl TraceDropshot for opentelemetry::global::BoxedSpan {
         self.set_attributes(vec![
             // Rename to dropshot.id ????
             opentelemetry::KeyValue::new("http.id".to_string(), request.id),
+            opentelemetry::KeyValue::new(trace::URL_PATH, request.path),
             opentelemetry::KeyValue::new(
-                "http.method".to_string(),
+                trace::HTTP_REQUEST_METHOD,
                 request.method,
             ),
-            opentelemetry::KeyValue::new("http.path".to_string(), request.path),
             opentelemetry::KeyValue::new(
-                "server.address".to_string(),
+                trace::SERVER_ADDRESS,
                 request.local_addr.ip().to_string(),
             ),
             opentelemetry::KeyValue::new(
-                "server.port".to_string(),
+                trace::SERVER_PORT,
                 request.local_addr.port().to_string(),
             ),
             opentelemetry::KeyValue::new(
-                "user_agent.original".to_string(),
+                trace::USER_AGENT_ORIGINAL,
                 request.user_agent,
             ),
         ]);
     }
 
-    // - [x] http.response.status_code
-    // - [x] error.message
-    // - [ ] error.cause_chain
-    // - [ ] otel.status_code
+    /// TODO: Do we want to differentiate between 4xx vs 5xx errors
+    /// and not mark 4xx spans as error spans?
     fn trace_response(&mut self, response: ResponseInfo) {
         self.set_attributes(vec![
             opentelemetry::KeyValue::new(