Skip to content

Commit

Permalink
fix(telemetry): borrowing issue with multi-threads
Browse files Browse the repository at this point in the history
  • Loading branch information
peterwht committed May 6, 2024
1 parent a6d6764 commit c7c87c2
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 19 deletions.
2 changes: 0 additions & 2 deletions crates/pop-cli/src/commands/new/parachain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ use std::{

use cliclack::{clear_screen, confirm, input, intro, log, outro, outro_cancel, set_theme};
use pop_parachains::{instantiate_template_dir, Config, Git, GitHub, Provider, Release, Template};
use pop_telemetry::Result as TelResult;
use tokio::task::JoinHandle;

#[derive(Args, Clone)]
pub struct NewParachainCommand {
Expand Down
4 changes: 1 addition & 3 deletions crates/pop-cli/src/commands/test/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ use std::path::PathBuf;
use clap::Args;
use cliclack::{clear_screen, intro, outro};
use pop_contracts::{test_e2e_smart_contract, test_smart_contract};
use pop_telemetry::Result as TelResult;
use tokio::task::JoinHandle;

use crate::style::style;

Expand All @@ -21,7 +19,7 @@ pub(crate) struct TestContractCommand {
impl TestContractCommand {
pub(crate) fn execute(&self) -> anyhow::Result<&str> {
clear_screen()?;
let mut feature = "";
let feature;

if self.features.is_some() && self.features.clone().unwrap().contains("e2e-tests") {
intro(format!(
Expand Down
11 changes: 6 additions & 5 deletions crates/pop-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use commands::*;
use pop_telemetry::{config_file_path, record_cli_command, record_cli_used, Telemetry};
use serde_json::{json, Value};
use std::{fs::create_dir_all, path::PathBuf};
use tokio::{spawn, task::JoinHandle};
use tokio::spawn;

#[derive(Parser)]
#[command(author, version, about, styles=style::get_styles())]
Expand Down Expand Up @@ -53,11 +53,11 @@ async fn main() -> Result<()> {

let maybe_config_file = config_file_path();
// if config file errors set telemetry to None, otherwise Some(tel)
let maybe_tel = maybe_config_file.ok().map(|path| &Telemetry::new(endpoint.to_string(), path));
let maybe_tel = maybe_config_file.ok().map(|path| Telemetry::new(endpoint.to_string(), path));

// handle for await not used here as telemetry should complete before any of the commands do.
// Sends a generic ping saying the CLI was used
spawn(record_cli_used(maybe_tel));
spawn(record_cli_used(maybe_tel.clone()));

// type to represent static telemetry data. I.e., does not contain data dynamically chosen by user
// like in pop new parachain.
Expand Down Expand Up @@ -143,12 +143,13 @@ async fn main() -> Result<()> {
};

// Best effort to send on first try, no action if failure
let _ = record_cli_command(maybe_tel, tel_data.0, json!({tel_data.1: tel_data.2})).await;
let _ =
record_cli_command(maybe_tel.clone(), tel_data.0, json!({tel_data.1: tel_data.2})).await;

// Send if error
if res.is_err() {
let _ =
spawn(record_cli_command(maybe_tel, "error", json!({tel_data.0: tel_data.1}))).await;
record_cli_command(maybe_tel.clone(), "error", json!({tel_data.0: tel_data.1})).await;
}

res
Expand Down
18 changes: 9 additions & 9 deletions crates/pop-telemetry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub enum TelemetryError {

pub type Result<T> = std::result::Result<T, TelemetryError>;

#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct Telemetry {
// Endpoint to the telemetry API.
// This should include the domain and api path (e.g. localhost:3000/api/send)
Expand Down Expand Up @@ -86,7 +86,7 @@ impl Telemetry {
/// Generically reports that the CLI was used to the telemetry endpoint.
/// There is explicitly no reqwest retries on failure to ensure overhead
/// stays to a minimum.
pub async fn record_cli_used(maybe_tel: Option<&Telemetry>) -> Result<()> {
pub async fn record_cli_used(maybe_tel: Option<Telemetry>) -> Result<()> {
let tel = maybe_tel.ok_or(TelemetryError::TelemetryNotSet)?;

let payload = generate_payload("", json!({}));
Expand All @@ -104,7 +104,7 @@ pub async fn record_cli_used(maybe_tel: Option<&Telemetry>) -> Result<()> {
/// `data`: the JSON representation of subcommands. This should never include any user inputted
/// data like a file name.
pub async fn record_cli_command(
maybe_tel: Option<&Telemetry>,
maybe_tel: Option<Telemetry>,
command_name: &str,
data: Value,
) -> Result<()> {
Expand Down Expand Up @@ -275,7 +275,7 @@ mod tests {

let tel = Telemetry::new(endpoint.clone(), config_path);

assert!(record_cli_used(Some(&tel)).await.is_ok());
assert!(record_cli_used(Some(tel)).await.is_ok());
mock.assert_async().await;
}

Expand All @@ -297,7 +297,7 @@ mod tests {

let tel = Telemetry::new(endpoint.clone(), config_path);

assert!(record_cli_command(Some(&tel), "new", json!("parachain")).await.is_ok());
assert!(record_cli_command(Some(tel), "new", json!("parachain")).await.is_ok());
mock.assert_async().await;
}

Expand All @@ -323,11 +323,11 @@ mod tests {
Err(TelemetryError::NotOptedIn)
));
assert!(matches!(
record_cli_used(Some(&tel_with_bad_config_path)).await,
record_cli_used(Some(tel_with_bad_config_path.clone())).await,
Err(TelemetryError::NotOptedIn)
));
assert!(matches!(
record_cli_command(Some(&tel_with_bad_config_path), "foo", json!("bar")).await,
record_cli_command(Some(tel_with_bad_config_path.clone()), "foo", json!("bar")).await,
Err(TelemetryError::NotOptedIn)
));
mock.assert_async().await;
Expand All @@ -336,8 +336,8 @@ mod tests {
tel_with_bad_config_path.opt_in = true;
let mock = mock.expect_at_most(3);
assert!(tel_with_bad_config_path.send_json(json!("foo")).await.is_ok(),);
assert!(record_cli_used(Some(&tel_with_bad_config_path)).await.is_ok());
assert!(record_cli_command(Some(&tel_with_bad_config_path), "foo", json!("bar"))
assert!(record_cli_used(Some(tel_with_bad_config_path.clone())).await.is_ok());
assert!(record_cli_command(Some(tel_with_bad_config_path), "foo", json!("bar"))
.await
.is_ok());
mock.assert_async().await;
Expand Down

0 comments on commit c7c87c2

Please sign in to comment.