Repository: dmolokanov/appinsights-rs (language: rust)

Issue:
Consider exporting a non-generic `TelemetryClient`
`TelemetryClient` is actually generic in an (unconstrained) type `C`. All of the practically interesting `impl`'s of `TelemetryClient` bound `C: TelemetryChannel`. Furthermore, the only ctors for `TelemetryClient` assume that `C = InMemoryChannel`, specifically.

For ergonomic reasons, consider hiding this abstraction from users, exposing the abstracted version separately.

One easy mechanism could be via exporting a type alias for `TelemetryClient<InMemoryChannel>`. But, thanks to the recent export of `InMemoryChannel`, users can do this themselves. My guess is that in practice, users will always want to use the default client that uses `InMemoryChannel`, and it would be fine to just assume this for the top-level export, letting exceptional users opt in to a more parameterized ("`Ex`") version.

Another way you could remove the generic parameter: enclose the channel in a `Box<dyn TelemetryChannel>`, at the cost of some indirection.


Discussion / hints:

Fix patch:
diff --git a/appinsights/src/channel/memory.rs b/appinsights/src/channel/memory.rs
index 16bee8a..e3ff032 100644
--- a/appinsights/src/channel/memory.rs
+++ b/appinsights/src/channel/memory.rs
@@ -42,7 +42,7 @@ impl InMemoryChannel {
         }
     }
 
-    async fn shutdown(mut self, command: Command) {
+    async fn shutdown(&mut self, command: Command) {
         // send shutdown command
         if let Some(sender) = self.command_sender.take() {
             send_command(&sender, command);
@@ -69,11 +69,11 @@ impl TelemetryChannel for InMemoryChannel {
         }
     }
 
-    async fn close(self) {
+    async fn close(&mut self) {
         self.shutdown(Command::Close).await
     }
 
-    async fn terminate(self) {
+    async fn terminate(&mut self) {
         self.shutdown(Command::Terminate).await;
     }
 }
diff --git a/appinsights/src/channel/mod.rs b/appinsights/src/channel/mod.rs
index 694c642..40effc3 100644
--- a/appinsights/src/channel/mod.rs
+++ b/appinsights/src/channel/mod.rs
@@ -24,12 +24,12 @@ pub trait TelemetryChannel {
     /// Flushes and tears down the submission flow and closes internal channels.
     /// It blocks the current task until all pending telemetry items have been submitted and it is safe to
     /// shutdown without losing telemetry.
-    async fn close(self);
+    async fn close(&mut self);
 
     /// Flushes and tears down the submission flow and closes internal channels.
     /// It blocks the current task until all pending telemetry items have been submitted and it is safe to
     /// shutdown without losing telemetry.
     /// Tears down the submission flow and closes internal channels. Any telemetry waiting to be sent is discarded.
     /// This is a more abrupt version of [close](#method.close).
-    async fn terminate(self);
+    async fn terminate(&mut self);
 }
diff --git a/appinsights/src/client/mod.rs b/appinsights/src/client/mod.rs
index 4e4364c..e5ebdee 100644
--- a/appinsights/src/client/mod.rs
+++ b/appinsights/src/client/mod.rs
@@ -14,34 +14,32 @@ use crate::{
 };
 
 /// Application Insights telemetry client provides an interface to track telemetry items.
-pub struct TelemetryClient<C> {
+pub struct TelemetryClient {
     enabled: bool,
     context: TelemetryContext,
-    channel: C,
+    channel: Box<dyn TelemetryChannel>,
 }
 
-impl TelemetryClient<InMemoryChannel> {
+impl TelemetryClient {
     /// Creates a new telemetry client that submits telemetry with specified instrumentation key.
     pub fn new(i_key: String) -> Self {
         Self::from_config(TelemetryConfig::new(i_key))
     }
-}
 
-impl TelemetryClient<InMemoryChannel> {
     /// Creates a new telemetry client configured with specified configuration.
     pub fn from_config(config: TelemetryConfig) -> Self {
+        Self::create(&config, InMemoryChannel::new(&config))
+    }
+
+    /// Creates a new telemetry client with custom telemetry channel.
+    pub(crate) fn create<C: TelemetryChannel + 'static>(config: &TelemetryConfig, channel: C) -> Self {
         Self {
             enabled: true,
-            context: TelemetryContext::from_config(&config),
-            channel: InMemoryChannel::new(&config),
+            context: TelemetryContext::from_config(config),
+            channel: Box::new(channel),
         }
     }
-}
 
-impl<C> TelemetryClient<C>
-where
-    C: TelemetryChannel,
-{
     /// Determines whether this client is enabled and will accept telemetry.
     ///
     /// # Examples
@@ -287,7 +285,7 @@ where
     /// // unable to sent any telemetry after client closes its channel
     /// // client.track_event("app is stopped".to_string());
     /// ```
-    pub async fn close_channel(self) {
+    pub async fn close_channel(mut self) {
         self.channel.close().await;
     }
 
@@ -316,27 +314,28 @@ where
     /// // unable to sent any telemetry after client closes its channel
     /// // client.track_event("app is stopped".to_string());
     /// ```
-    pub async fn terminate(self) {
+    pub async fn terminate(mut self) {
         self.channel.terminate().await;
     }
 }
 
-impl From<(TelemetryConfig, TelemetryContext)> for TelemetryClient<InMemoryChannel> {
+impl From<(TelemetryConfig, TelemetryContext)> for TelemetryClient {
     fn from((config, context): (TelemetryConfig, TelemetryContext)) -> Self {
         Self {
             enabled: true,
             context,
-            channel: InMemoryChannel::new(&config),
+            channel: Box::new(InMemoryChannel::new(&config)),
         }
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use std::cell::RefCell;
+    use std::sync::Arc;
 
     use async_trait::async_trait;
     use chrono::{DateTime, Utc};
+    use crossbeam_queue::SegQueue;
     use matches::assert_matches;
 
     use super::*;
@@ -359,22 +358,22 @@ mod tests {
 
     #[tokio::test]
     async fn it_submits_telemetry() {
-        let client = create_client();
+        let events = Arc::new(SegQueue::default());
+        let client = create_client(events.clone());
 
         client.track(TestTelemetry {});
 
-        let events = client.channel.events.borrow();
         assert_eq!(events.len(), 1)
     }
 
     #[tokio::test]
     async fn it_swallows_telemetry_when_disabled() {
-        let mut client = create_client();
+        let events = Arc::new(SegQueue::default());
+        let mut client = create_client(events.clone());
         client.enabled(false);
 
         client.track(TestTelemetry {});
 
-        let events = client.channel.events.borrow();
         assert!(events.is_empty())
     }
 
@@ -393,15 +392,9 @@ mod tests {
         assert!(client.is_enabled())
     }
 
-    fn create_client() -> TelemetryClient<TestChannel> {
+    fn create_client(events: Arc<SegQueue<Envelope>>) -> TelemetryClient {
         let config = TelemetryConfig::new("instrumentation".into());
-        TelemetryClient {
-            enabled: true,
-            context: TelemetryContext::from_config(&config),
-            channel: TestChannel {
-                events: RefCell::new(Vec::new()),
-            },
-        }
+        TelemetryClient::create(&config, TestChannel { events })
     }
 
     struct TestTelemetry {}
@@ -438,24 +431,24 @@ mod tests {
     }
 
     struct TestChannel {
-        events: RefCell<Vec<Envelope>>,
+        events: Arc<SegQueue<Envelope>>,
     }
 
     #[async_trait]
     impl TelemetryChannel for TestChannel {
         fn send(&self, envelop: Envelope) {
-            self.events.borrow_mut().push(envelop);
+            self.events.push(envelop);
         }
 
         fn flush(&self) {
             unimplemented!()
         }
 
-        async fn close(self) {
+        async fn close(&mut self) {
             unimplemented!()
         }
 
-        async fn terminate(self) {
+        async fn terminate(&mut self) {
             unimplemented!()
         }
     }


Test patch:
diff --git a/appinsights/src/client/integration_tests.rs b/appinsights/src/client/integration_tests.rs
index 201ad8b..eb8310e 100644
--- a/appinsights/src/client/integration_tests.rs
+++ b/appinsights/src/client/integration_tests.rs
@@ -21,7 +21,7 @@ use tokio::sync::{
     oneshot,
 };
 
-use crate::{channel::InMemoryChannel, timeout, TelemetryClient, TelemetryConfig};
+use crate::{timeout, TelemetryClient, TelemetryConfig};
 
 lazy_static! {
     /// A global lock since most tests need to run in serial.
@@ -371,7 +371,7 @@ manual_timeout_test! {
 
 // TODO Check case when all retries exhausted. Pending items should not be lost
 
-fn create_client(endpoint: &str) -> TelemetryClient<InMemoryChannel> {
+fn create_client(endpoint: &str) -> TelemetryClient {
     let config = TelemetryConfig::builder()
         .i_key("instrumentation key")
         .endpoint(endpoint)