core: fix missing spans in serve tasks (#1243)

Currently, the `serve` function spawns tasks `instrument`ed with a
`tracing` span for each accepted connection. This span is at the `DEBUG`
level, so it's disabled with the default tracing configuration. This is
fine, as the per-connection context is relatively verbose and is mainly
useful for debugging.

However, we also rely on this span for propagating the `INFO`-level
spans which indicate which part of the proxy an event occurred in
(inbound, outbound, etc). When the `DEBUG` span is enabled, it will be a
child of these spans, so they are propagated to the spawned tasks.
However, when the `DEBUG` span is _not_ enabled, nothing propagates the
`INFO` spans. Since the default `tracing` configuration enables `INFO`
but not `DEBUG`, we want those spans to be propagated to the tasks
spawned in `serve`.

This commit fixes the missing spans by moving the spawn inside of the
`Span::in_scope` call, and using `in_current_span` rather than
`instrument`. Now, if the per-connection `DEBUG` span is enabled, it
will be the current span...but if it isn't, the `INFO` span will still
be current, so the task will still have the `INFO` span as part of its
span context regardless.

Alternatively, we could have fixed this by changing the `instrument()`
call to:
```rust
    .instrument(span)
    .in_current_span()
```
so that the task is always spawned in both the `DEBUG` span _and_ the
current span. However, this is a bit less efficient, as it wraps the
tasks in both spans even when the `INFO` span is not needed, so every
time the task is polled, we would enter both spans.

Author: hawkw

linkerd/app/core/src/serve.rs

@@ -8,8 +8,7 @@ use linkerd_error::Error;
 use tower::util::ServiceExt;
 use tracing::{debug, debug_span, info, instrument::Instrument, warn};
 
-/// Spawns a task that binds an `L`-typed listener with an `A`-typed
-/// connection-accepting service.
+/// Spawns a task that binds an `L`-typed listener with an `A`-typed connection-accepting service.
 ///
 /// The task is driven until shutdown is signaled.
 pub async fn serve<M, S, I, A>(
@@ -40,38 +39,39 @@ pub async fn serve<M, S, I, A>(
                     };
 
                     // The local addr should be instrumented from the listener's context.
-                    let span = debug_span!("accept", client.addr = %addrs.param());
+                    debug_span!("accept", client.addr = %addrs.param()).in_scope(|| {
+                        let accept = new_accept.new_service(addrs);
 
-                    let accept = span.in_scope(|| new_accept.new_service(addrs));
-
-                    // Dispatch all of the work for a given connection onto a connection-specific task.
-                    tokio::spawn(
-                        async move {
-                            match accept.ready_oneshot().err_into::<Error>().await {
-                                Ok(mut accept) => {
-                                    match accept
-                                        .call(io::ScopedIo::server(io))
-                                        .err_into::<Error>()
-                                        .await
-                                    {
-                                        Ok(()) => debug!("Connection closed"),
-                                        Err(reason) if is_io(&*reason) => {
-                                            debug!(%reason, "Connection closed")
+                        // Dispatch all of the work for a given connection onto a
+                        // connection-specific task.
+                        tokio::spawn(
+                            async move {
+                                match accept.ready_oneshot().err_into::<Error>().await {
+                                    Ok(mut accept) => {
+                                        match accept
+                                            .call(io::ScopedIo::server(io))
+                                            .err_into::<Error>()
+                                            .await
+                                        {
+                                            Ok(()) => debug!("Connection closed"),
+                                            Err(reason) if is_io(&*reason) => {
+                                                debug!(%reason, "Connection closed")
+                                            }
+                                            Err(error) => info!(%error, "Connection closed"),
                                         }
-                                        Err(error) => info!(%error, "Connection closed"),
+                                        // Hold the service until the connection is complete. This
+                                        // helps tie any inner cache lifetimes to the services they
+                                        // return.
+                                        drop(accept);
+                                    }
+                                    Err(error) => {
+                                        warn!(%error, "Server failed to become ready");
                                     }
-                                    // Hold the service until the connection is
-                                    // complete. This helps tie any inner cache
-                                    // lifetimes to the services they return.
-                                    drop(accept);
-                                }
-                                Err(error) => {
-                                    warn!(%error, "Server failed to become ready");
                                 }
                             }
-                        }
-                        .instrument(span),
-                    );
+                            .in_current_span(),
+                        );
+                    });
                 }
             }
         }