Documentation

Signed-off-by: Caleb Schoepp <caleb.schoepp@fermyon.com>

Author: calebschoepp

crates/factor-observe/src/lib.rs

@@ -38,8 +38,6 @@ impl Factor for ObserveFactor {
         &self,
         ctx: spin_factors::PrepareContext<T, Self>,
     ) -> anyhow::Result<Self::InstanceBuilder> {
-        // TODO(Lann): Is it fine that we're using BoxedTracer and BoxedSpan? Are there perf tradeoffs?
-        // TODO(Lann): Would it make sense to put the tracer on the app state and just hold a reference to it in the instance state?
         let tracer = global::tracer(ctx.app_component().app.id().to_string());
         Ok(InstanceState {
             state: Arc::new(RwLock::new(State {
@@ -73,11 +71,10 @@ pub(crate) struct State {
     /// A resource table that holds the guest spans.
     pub(crate) guest_spans: table::Table<GuestSpan>,
 
-    /// A LIFO stack of guest spans that are currently active.
+    /// A stack of resource ids for all the active guest spans. The topmost span is the active span.
     ///
-    /// Only a reference ID to the guest span is held here. The actual guest span must be looked up
-    /// in the `guest_spans` table using the reference ID.
-    /// TODO(Caleb): Fix comment
+    /// When a guest span is ended it is removed from this stack (regardless of whether is the
+    /// active span) and all other spans are shifted back to retain relative order.
     pub(crate) active_spans: IndexSet<u32>,
 
     /// Id of the last span emitted from within the host before entering the guest.
@@ -93,13 +90,17 @@ pub struct GuestSpan {
     pub inner: opentelemetry::global::BoxedSpan,
 }
 
-/// TODO(Caleb): comment
+/// Manages access to the ObserveFactor state for the purpose of maintaining proper span
+/// parent/child relationships when WASI Observe spans are being created.
 pub struct ObserveContext {
     pub(crate) state: Option<Arc<RwLock<State>>>,
 }
 
 impl ObserveContext {
-    /// TODO(Caleb): Comment
+    /// Creates an [`ObserveContext`] from a [`PrepareContext`].
+    ///
+    /// If [`RuntimeFactors`] does not contain an [`ObserveFactor`], then calling
+    /// [`ObserveContext::reparent_tracing_span`] will be a no-op.
     pub fn from_prepare_context<T: RuntimeFactors, F: Factor>(
         prepare_context: &mut PrepareContext<T, F>,
     ) -> anyhow::Result<Self> {
@@ -111,21 +112,45 @@ impl ObserveContext {
         Ok(Self { state })
     }
 
-    /// TODO(Caleb): comment
-    /// Make sure to mention this should only be called from an instrumented function in a factor.
-    /// Make sure this is called before any awaits
+    /// Reparents the current [tracing] span to be a child of the last active guest span.
+    ///
+    /// The observe factor enables guests to emit spans that should be part of the same trace as the
+    /// host is producing for a request. Below is an example trace. A request is made to an app, a
+    /// guest span is created and then the host is re-entered to fetch a key value.
+    ///
+    /// ```text
+    /// | GET /... _________________________________|
+    ///    | execute_wasm_component foo ___________|
+    ///       | my_guest_span ___________________|
+    ///          | spin_key_value.get |
+    /// ```
+    ///
+    ///  Setting the guest spans parent as the host is trivially done. However, the more difficult
+    /// task is having the host factor spans be children of the guest span.
+    /// [`ObserveContext::reparent_tracing_span`] handles this by reparenting the current span to be
+    /// a child of the last active guest span (which is tracked internally in the observe factor).
+    ///
+    /// Note that if the observe factor is not in your [`RuntimeFactors`] than this is effectively a
+    /// no-op.
+    ///
+    /// This MUST only be called from a factor host implementation function that is instrumented.
+    ///
+    /// This MUST be called at the very start of the function before any awaits.
     pub fn reparent_tracing_span(&self) {
-        // TODO(Caleb): Refactor to be similar to start
+        // If state is None then we want to return early b/c the factor doesn't depend on the
+        // Observe factor and therefore there is nothing to do
         let state = if let Some(state) = self.state.as_ref() {
             state.read().unwrap()
         } else {
             return;
         };
 
+        // If there are no active guest spans then there is nothing to do
         if state.active_spans.is_empty() {
             return;
         }
 
+        // Ensure that we are not reparenting the original host span
         if let Some(original_host_span_id) = state.original_host_span_id {
             if tracing::Span::current()
                 .context()
@@ -134,19 +159,20 @@ impl ObserveContext {
                 .span_id()
                 .eq(&original_host_span_id)
             {
-                panic!("TODO(Caleb): This should not happen")
+                // TODO(Caleb): Figure out why this occurs under load
+                panic!("The original host span was incorrectly reparented. Likely `reparent_tracing_span` was called in an incorrect location.")
             }
         }
 
-        let parent_context = Context::new().with_remote_span_context(
-            state
-                .guest_spans
-                .get(*state.active_spans.last().unwrap())
-                .unwrap()
-                .inner
-                .span_context()
-                .clone(),
-        );
+        // Now reparent the current span to the last active guest span
+        let span_context = state
+            .guest_spans
+            .get(*state.active_spans.last().unwrap())
+            .unwrap()
+            .inner
+            .span_context()
+            .clone();
+        let parent_context = Context::new().with_remote_span_context(span_context);
         tracing::Span::current().set_parent(parent_context);
     }
 }

wit/deps/observe/world.wit

@@ -3,3 +3,6 @@ package wasi:observe@0.2.0-draft;
 world imports {
     import tracer;
 }
+
+// TODO(Reviewer): Should we make this an experimental wasi package or a Spin package
+// TODO(Reviewer): Would adding a metrics interface to this in a future version be a breaking change?