From 1db15e7b02b38b2c34a0f113302edb10fe164560 Mon Sep 17 00:00:00 2001 From: Nahum Shalman Date: Thu, 12 Dec 2024 21:10:50 +0000 Subject: [PATCH] XXX Better code documentation --- dropshot/examples/multiple-servers.rs | 4 ++ dropshot/examples/otel.rs | 10 ++--- dropshot/src/otel.rs | 65 ++++++++++++++------------- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/dropshot/examples/multiple-servers.rs b/dropshot/examples/multiple-servers.rs index b3810fec2..b2c179953 100644 --- a/dropshot/examples/multiple-servers.rs +++ b/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")]; diff --git a/dropshot/examples/otel.rs b/dropshot/examples/otel.rs index fabb8e0b6..25e59bd8f 100644 --- a/dropshot/examples/otel.rs +++ b/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, ) -> Result, 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, update: TypedBody, diff --git a/dropshot/src/otel.rs b/dropshot/src/otel.rs index cc74edbd5..9135f5f77 100644 --- a/dropshot/src/otel.rs +++ b/dropshot/src/otel.rs @@ -1,22 +1,28 @@ // 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] +//! 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]: +//! use opentelemetry::{ global, trace::Span, trace::Tracer, trace::TracerProvider, @@ -24,10 +30,6 @@ use opentelemetry::{ 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, ) -> 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, ) -> 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(