-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix telemetry span not entering properly attempt 3 #8043
Changes from 21 commits
5c2f3dc
094fee8
a443b5c
5448b94
f52d1ea
09ef926
6f42906
1b8d945
c2e1e37
4f7f326
60c02ef
d1381de
fd5cfbb
95e4742
3d7adba
34008ec
c94bdc6
4d023d5
17b2010
e7dae73
35fa4c3
e4544c3
4315639
86870f0
b7538be
6d4b06b
5705955
51738dd
d57c361
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| #![cfg(unix)] | ||
|
|
||
| use assert_cmd::cargo::cargo_bin; | ||
| use nix::sys::signal::{kill, Signal::SIGINT}; | ||
| use nix::unistd::Pid; | ||
| use std::convert::TryInto; | ||
| use std::process; | ||
| use tempfile::tempdir; | ||
|
|
||
| pub mod common; | ||
| pub mod websocket_server; | ||
|
|
||
| #[async_std::test] | ||
| async fn telemetry_works() { | ||
|
Comment on lines
+28
to
+29
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you have written a macro for creating async tests in the Substrate context?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And in general I don't understand why we use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will check that tomorrow 😅
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit complicated with all the I think that's good enough for now if that's okay for you.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you educate me on what the tokio situation is in substrate atm? Is the following correct: If the above is correct we clearly have some tech debt to pay off here. :/
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have:
I don't personally mind having multiple different executors in different crates if it is for tests. It's just a dev dependency, it doesn't impact the user. Compiling both isn't a big issue either. 🤷♀️ |
||
| let config = websocket_server::Config { | ||
| capacity: 1, | ||
| max_frame_size: 1048 * 1024, | ||
| send_buffer_len: 32, | ||
| bind_address: "127.0.0.1:0".parse().unwrap(), | ||
| }; | ||
| let mut server = websocket_server::WsServer::new(config).await.unwrap(); | ||
|
|
||
| let addr = server.local_addr().unwrap(); | ||
|
|
||
| let server_task = async_std::task::spawn(async move { | ||
| loop { | ||
| use websocket_server::Event; | ||
|
|
||
| let mut received_telemetry = false; | ||
|
|
||
|
cecton marked this conversation as resolved.
Outdated
|
||
| match server.next_event().await { | ||
| // New connection on the listener. | ||
| Event::ConnectionOpen { address } => { | ||
| println!("New connection from {:?}", address); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this useful or should it be removed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can be useful when debugging. I personally used it for that purpose. But again it doesn't show up unless the test fails and it's an indicator. |
||
| if server.len() < 512 { | ||
| // Rather than passing `()`, it is possible to pass any value. The value is | ||
| // provided back on `TextFrame` and `ConnectionError` events. | ||
| server.accept(()); | ||
| } else { | ||
| server.reject(); | ||
| } | ||
| } | ||
|
|
||
| // Received a message from a connection. | ||
| Event::TextFrame { message, .. } => { | ||
| let json: serde_json::Value = serde_json::from_str(&message).unwrap(); | ||
| let object = json.as_object().unwrap(); | ||
| let payload = object.get("payload").unwrap(); | ||
| let object = payload.as_object().unwrap(); | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| if matches!(object.get("best"), Some(serde_json::Value::String(_))) { | ||
| received_telemetry = true; | ||
| } | ||
| //println!("Received message: {}", json); | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| // Connection has been closed. | ||
| Event::ConnectionError { .. } => {} | ||
| } | ||
|
|
||
| if received_telemetry { | ||
| break; | ||
| } | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| } | ||
| }); | ||
|
|
||
| let base_path = tempdir().expect("could not create a temp dir"); | ||
| let mut substrate = process::Command::new(cargo_bin("substrate")); | ||
|
|
||
| let mut substrate = substrate | ||
| .args(&["--dev", "--telemetry-url"]) | ||
| .arg(format!("ws://{} 10", addr)) | ||
| .arg("-d") | ||
|
cecton marked this conversation as resolved.
Outdated
|
||
| .arg(base_path.path()) | ||
| .stdout(process::Stdio::piped()) | ||
| .stderr(process::Stdio::piped()) | ||
| .stdin(process::Stdio::null()) | ||
| .spawn() | ||
| .unwrap(); | ||
|
|
||
| server_task.await; | ||
|
|
||
| assert!( | ||
| substrate.try_wait().unwrap().is_none(), | ||
| "the process should still be running" | ||
| ); | ||
|
|
||
| // Stop the process | ||
| kill(Pid::from_raw(substrate.id().try_into().unwrap()), SIGINT).unwrap(); | ||
| assert!(common::wait_for(&mut substrate, 40) | ||
| .map(|x| x.success()) | ||
| .unwrap_or_default()); | ||
|
|
||
| let output = substrate.wait_with_output().unwrap(); | ||
|
|
||
| println!("{}", String::from_utf8(output.stdout).unwrap()); | ||
| eprintln!("{}", String::from_utf8(output.stderr).unwrap()); | ||
|
cecton marked this conversation as resolved.
|
||
| assert!(output.status.success()); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.