RPC to allow setting the log filter (#7474)

* Add filter reload handle

* add RPC, move logging module from cli to tracing

* remove dup fn

* working example

* Update client/rpc-api/src/system/mod.rs

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

* Prefer "set" to "reload"

* Re-enable the commented out features of the logger

* Remove duplicate code

* cleanup

* unneeded lvar

* Bump to latest patch release

* Add new CLI option to disable log filter reloading,

Move profiling CLI options to SharedParams

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Applied suggestions from reviews

* Fix calls to init_logger()

* Handle errors when parsing logging directives

* Deny `system_setLogFilter` RPC by default

* One more time

* Don't ignore parse errors for log directives set via CLI or RPC

* Improve docs

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Update client/cli/src/config.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* fix merge errors

* include default directives with system_setLogFilter RPC,

implement system_rawSetLogFilter RPC to exclude defaults

* docs etc...

* update test

* refactor: rename fn

* Add a test for system_set_log_filter – NOTE: the code should likely change to return an error when bad directives are passed

* Update client/cli/src/lib.rs

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Address review grumbles

* Add doc note on panicking behaviour

* print all invalid directives before panic

* change RPCs to: addLogFilter and resetLogFilter

* make CLI log directives default

* add comments

* restore previous behaviour to panic when hard-coded directives are invalid

* change/refactor directive parsing

* fix line width

* add test for log filter reloading

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* finish up suggestions from code review

* improve test

* change expect message

* change fn name

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* add docs, remove unused fn

* propagate Err on invalid log directive

* Update tracing-subscriber version

* Improve docs for `disable_log_reloading` CLI param

Co-authored-by: Matt <mattrutherford@users.noreply.github.com>
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/rpc/src/system/mod.rs

@@ -197,4 +197,15 @@ impl<B: traits::Block> SystemApi<B::Hash, <B::Header as HeaderT>::Number> for Sy
 		let _ = self.send_back.unbounded_send(Request::SyncState(tx));
 		Receiver(Compat::new(rx))
 	}
+
+	fn system_add_log_filter(&self, directives: String) -> std::result::Result<(), rpc::Error> {
+		self.deny_unsafe.check_if_safe()?;
+		sc_tracing::add_directives(&directives);
+		sc_tracing::reload_filter().map_err(|_e| rpc::Error::internal_error())
+	}
+
+	fn system_reset_log_filter(&self)-> std::result::Result<(), rpc::Error> {
+		self.deny_unsafe.check_if_safe()?;
+		sc_tracing::reset_log_filter().map_err(|_e| rpc::Error::internal_error())
+	}
 }

substrate/client/rpc/src/system/tests.rs

@@ -24,7 +24,10 @@ use substrate_test_runtime_client::runtime::Block;
 use assert_matches::assert_matches;
 use futures::prelude::*;
 use sp_utils::mpsc::tracing_unbounded;
-use std::thread;
+use std::{
+	process::{Stdio, Command}, env, io::{BufReader, BufRead, Write},
+	sync::{Arc, Mutex}, thread, time::Duration
+};
 
 struct Status {
 	pub peers: usize,
@@ -333,3 +336,81 @@ fn system_network_remove_reserved() {
 	assert_eq!(runtime.block_on(good_fut), Ok(()));
 	assert!(runtime.block_on(bad_fut).is_err());
 }
+
+#[test]
+fn test_add_reset_log_filter() {
+	const EXPECTED_BEFORE_ADD: &'static str = "EXPECTED_BEFORE_ADD";
+	const EXPECTED_AFTER_ADD: &'static str = "EXPECTED_AFTER_ADD";
+
+	// Enter log generation / filter reload
+	if std::env::var("TEST_LOG_FILTER").is_ok() {
+		sc_cli::init_logger("test_before_add=debug", Default::default(), Default::default(), false).unwrap();
+		for line in std::io::stdin().lock().lines() {
+			let line = line.expect("Failed to read bytes");
+			if line.contains("add_reload") {
+				assert!(api(None).system_add_log_filter("test_after_add".to_owned()).is_ok(), "`system_add_log_filter` failed");
+			} else if line.contains("reset") {
+				assert!(api(None).system_reset_log_filter().is_ok(), "`system_reset_log_filter` failed");
+			} else if line.contains("exit") {
+				return;
+			}
+			log::debug!(target: "test_before_add", "{}", EXPECTED_BEFORE_ADD);
+			log::debug!(target: "test_after_add", "{}", EXPECTED_AFTER_ADD);
+		}
+	}
+
+	// Call this test again to enter the log generation / filter reload block
+	let test_executable = env::current_exe().expect("Unable to get current executable!");
+	let mut child_process = Command::new(test_executable)
+		.env("TEST_LOG_FILTER", "1")
+		.args(&["--nocapture", "test_add_reset_log_filter"])
+		.stdin(Stdio::piped())
+		.stderr(Stdio::piped())
+		.spawn()
+		.unwrap();
+
+	let child_stderr = child_process.stderr.take().expect("Could not get child stderr");
+	let mut child_out = BufReader::new(child_stderr);
+	let mut child_in = child_process.stdin.take().expect("Could not get child stdin");
+
+	let child_out_str = Arc::new(Mutex::new(String::new()));
+	let shared = child_out_str.clone();
+
+	let _handle = thread::spawn(move || {
+		let mut line = String::new();
+		while let Ok(_) = child_out.read_line(&mut line) {
+			shared.lock().unwrap().push_str(&line);
+			line.clear();
+		}
+	});
+
+	// Initiate logs loop in child process
+	child_in.write(b"\n").unwrap();
+	thread::sleep(Duration::from_millis(100));
+	let test1_str = child_out_str.lock().unwrap().clone();
+	// Assert that only the first target is present
+	assert!(test1_str.contains(EXPECTED_BEFORE_ADD));
+	assert!(!test1_str.contains(EXPECTED_AFTER_ADD));
+	child_out_str.lock().unwrap().clear();
+
+	// Initiate add directive & reload in child process
+	child_in.write(b"add_reload\n").unwrap();
+	thread::sleep(Duration::from_millis(100));
+	let test2_str = child_out_str.lock().unwrap().clone();
+	// Assert that both targets are now present
+	assert!(test2_str.contains(EXPECTED_BEFORE_ADD));
+	assert!(test2_str.contains(EXPECTED_AFTER_ADD));
+	child_out_str.lock().unwrap().clear();
+
+	// Initiate logs filter reset in child process
+	child_in.write(b"reset\n").unwrap();
+	thread::sleep(Duration::from_millis(100));
+	let test3_str = child_out_str.lock().unwrap().clone();
+	// Assert that only the first target is present as it was initially
+	assert!(test3_str.contains(EXPECTED_BEFORE_ADD));
+	assert!(!test3_str.contains(EXPECTED_AFTER_ADD));
+
+	// Return from child process
+	child_in.write(b"exit\n").unwrap();
+	assert!(child_process.wait().expect("Error waiting for child process").success());
+}