remove telemetry configuration hot reloading

NEXT_CHANGELOG.md

@@ -43,6 +43,20 @@ telemetry:
 
 By [@SimonSapin](https://github.com/SimonSapin)
 
+### Remove telemetry configuration hot reloading ([PR #1463](https://github.com/apollographql/router/pull/1463))
+
+Configuration hot reloading is not very useful for telemetry, and is the
+source of regular bugs that are hard to fix.
+
+This removes the support for configuration reloading entirely. Now, the
+router will reject a configuration reload with an error log if the
+telemetry configuration changed.
+
+It is now possible to create a subscriber and pass it explicitely to the telemetry plugin
+when creating it. It will then be modified to integrate the telemetry plugin's layer.
+
+By [@geal](https://github.com/geal) in https://github.com/apollographql/router/pull/1463
+
 ## 🚀 Features
 
 ### Add support of global rate limit and timeout. [PR #1347](https://github.com/apollographql/router/pull/1347)

apollo-router/Cargo.toml

@@ -166,7 +166,7 @@ tempfile = "3.3.0"
 test-log = { version = "0.2.10", default-features = false, features = [
     "trace",
 ] }
-test-span = "0.6"
+test-span = "0.7"
 tower-test = "0.4.0"
 tracing-subscriber = { version = "0.3", default-features = false, features = [
     "env-filter",

apollo-router/src/configuration/mod.rs

@@ -93,6 +93,7 @@ pub struct Configuration {
 }
 
 const APOLLO_PLUGIN_PREFIX: &str = "apollo.";
+const TELEMETRY_KEY: &str = "telemetry";
 
 fn default_listen() -> ListenAddr {
     SocketAddr::from_str("127.0.0.1:4000").unwrap().into()
@@ -143,6 +144,17 @@ impl Configuration {
 
         plugins
     }
+
+    // checks that we can reload configuration from the current one to the new one
+    pub fn is_compatible(&self, new: &Configuration) -> Result<(), &'static str> {
+        if self.apollo_plugins.plugins.get(TELEMETRY_KEY)
+            == new.apollo_plugins.plugins.get(TELEMETRY_KEY)
+        {
+            Ok(())
+        } else {
+            Err("incompatible telemetry configuration. Telemetry cannot be reloaded and its configuration must stay the same for the entire life of the process")
+        }
+    }
 }
 
 impl FromStr for Configuration {

apollo-router/src/error.rs

@@ -22,7 +22,6 @@ pub(crate) use crate::graphql::Error;
 use crate::graphql::Response;
 use crate::json_ext::Path;
 use crate::json_ext::Value;
-pub use crate::reload::Error as ReloadError;
 pub use crate::spec::SpecError;
 
 /// Error types for execution.

apollo-router/src/executable.rs

@@ -15,6 +15,9 @@ use clap::CommandFactory;
 use clap::Parser;
 use directories::ProjectDirs;
 use once_cell::sync::OnceCell;
+use tracing::dispatcher::with_default;
+use tracing::dispatcher::Dispatch;
+use tracing::instrument::WithSubscriber;
 use tracing_subscriber::EnvFilter;
 use url::ParseError;
 use url::Url;
@@ -26,10 +29,8 @@ use crate::router::ApolloRouter;
 use crate::router::ConfigurationKind;
 use crate::router::SchemaKind;
 use crate::router::ShutdownKind;
-use crate::subscriber::set_global_subscriber;
-use crate::subscriber::RouterSubscriber;
 
-static GLOBAL_ENV_FILTER: OnceCell<String> = OnceCell::new();
+pub(crate) static GLOBAL_ENV_FILTER: OnceCell<String> = OnceCell::new();
 
 /// Options for the router
 #[derive(Parser, Debug)]
@@ -192,24 +193,33 @@ impl Executable {
             return Ok(());
         }
 
-        // This is more complex than I'd like it to be. Really, we just want to pass
-        // a FmtSubscriber to set_global_subscriber(), but we can't because of the
-        // generic nature of FmtSubscriber. See: https://github.com/tokio-rs/tracing/issues/380
-        // for more details.
         let builder = tracing_subscriber::fmt::fmt().with_env_filter(
             EnvFilter::try_new(&opt.log_level).context("could not parse log configuration")?,
         );
 
-        let subscriber: RouterSubscriber = if atty::is(atty::Stream::Stdout) {
-            RouterSubscriber::TextSubscriber(builder.finish())
+        let dispatcher = if atty::is(atty::Stream::Stdout) {
+            Dispatch::new(builder.finish())
         } else {
-            RouterSubscriber::JsonSubscriber(builder.json().finish())
+            Dispatch::new(builder.json().finish())
         };
 
-        set_global_subscriber(subscriber)?;
+        GLOBAL_ENV_FILTER.set(opt.log_level.clone()).expect(
+            "failed setting the global env filter. THe start() function should only be called once",
+        );
 
-        GLOBAL_ENV_FILTER.set(opt.log_level).unwrap();
+        // The dispatcher we created is passed explicitely here to make sure we display the logs
+        // in the initialization pahse and in the state machine code, before a global subscriber
+        // is set using the configuration file
+        Self::inner_start(router_builder_fn, opt, dispatcher.clone())
+            .with_subscriber(dispatcher)
+            .await
+    }
 
+    async fn inner_start(
+        router_builder_fn: Option<fn(ConfigurationKind, SchemaKind) -> ApolloRouter>,
+        opt: Opt,
+        dispatcher: Dispatch,
+    ) -> Result<()> {
         let current_directory = std::env::current_dir()?;
 
         let configuration = opt
@@ -229,11 +239,12 @@ impl Executable {
                 }
             })
             .unwrap_or_else(|| Configuration::builder().build().into());
+
         let apollo_router_msg = format!("Apollo Router v{} // (c) Apollo Graph, Inc. // Licensed as ELv2 (https://go.apollo.dev/elv2)", std::env!("CARGO_PKG_VERSION"));
         let schema = match (opt.supergraph_path, opt.apollo_key) {
             (Some(supergraph_path), _) => {
                 tracing::info!("{apollo_router_msg}");
-                setup_panic_handler();
+                setup_panic_handler(dispatcher.clone());
 
                 let supergraph_path = if supergraph_path.is_relative() {
                     current_directory.join(supergraph_path)
@@ -248,6 +259,7 @@ impl Executable {
             }
             (None, Some(apollo_key)) => {
                 tracing::info!("{apollo_router_msg}");
+
                 let apollo_graph_ref = opt.apollo_graph_ref.ok_or_else(||anyhow!("cannot fetch the supergraph from Apollo Studio without setting the APOLLO_GRAPH_REF environment variable"))?;
                 if opt.apollo_uplink_poll_interval < Duration::from_secs(10) {
                     return Err(anyhow!("Apollo poll interval must be at least 10s"));
@@ -322,7 +334,7 @@ impl Executable {
     }
 }
 
-fn setup_panic_handler() {
+fn setup_panic_handler(dispatcher: Dispatch) {
     // Redirect panics to the logs.
     let backtrace_env = std::env::var("RUST_BACKTRACE");
     let show_backtraces =
@@ -331,12 +343,14 @@ fn setup_panic_handler() {
         tracing::warn!("RUST_BACKTRACE={} detected. This use useful for diagnostics but will have a performance impact and may leak sensitive information", backtrace_env.as_ref().unwrap());
     }
     std::panic::set_hook(Box::new(move |e| {
-        if show_backtraces {
-            let backtrace = backtrace::Backtrace::new();
-            tracing::error!("{}\n{:?}", e, backtrace)
-        } else {
-            tracing::error!("{}", e)
-        }
+        with_default(&dispatcher, || {
+            if show_backtraces {
+                let backtrace = backtrace::Backtrace::new();
+                tracing::error!("{}\n{:?}", e, backtrace)
+            } else {
+                tracing::error!("{}", e)
+            }
+        });
     }));
 }
 

apollo-router/src/lib.rs

@@ -62,15 +62,13 @@ pub mod layers;
 pub mod plugin;
 pub mod plugins;
 pub mod query_planner;
-mod reload;
 mod request;
 mod response;
 mod router;
 mod router_factory;
 pub mod services;
 mod spec;
 mod state_machine;
-pub mod subscriber;
 mod traits;
 
 pub use configuration::*;