-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix telemetry span not entering properly attempt 3 #8043
Changes from 26 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 @@ | ||
| // This file is part of Substrate. | ||
|
|
||
| // Copyright (C) 2021 Parity Technologies (UK) Ltd. | ||
| // SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 | ||
|
|
||
| // This program is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
|
|
||
| // This program is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
|
|
||
| // You should have received a copy of the GNU General Public License | ||
| // along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
|
|
||
| use assert_cmd::cargo::cargo_bin; | ||
| use nix::sys::signal::{kill, Signal::SIGINT}; | ||
| use nix::unistd::Pid; | ||
| use std::convert::TryInto; | ||
| use std::process; | ||
|
|
||
| pub mod common; | ||
| pub mod websocket_server; | ||
|
|
||
| #[async_std::test] | ||
| async fn telemetry_works() { | ||
| 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; | ||
| 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. |
||
| server.accept(); | ||
| } | ||
|
|
||
| // Received a message from a connection. | ||
| Event::BinaryFrame { message, .. } => { | ||
| let json: serde_json::Value = serde_json::from_slice(&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(_))) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| Event::TextFrame { .. } => unreachable!(), | ||
|
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. Perhaps it's better to
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. If you want ^_^ I don't particularly mind unwrapping in tests |
||
|
|
||
| // Connection has been closed. | ||
| Event::ConnectionError { .. } => {} | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| let mut substrate = process::Command::new(cargo_bin("substrate")); | ||
|
|
||
| let mut substrate = substrate | ||
| .args(&["--dev", "--tmp", "--telemetry-url"]) | ||
| .arg(format!("ws://{} 10", addr)) | ||
| .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()); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in general I don't understand why we use
async-stdhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check that tomorrow 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit complicated with all the
compatstuff. I couldn't change it easily and it adds more dependency (tokio-util):I think that's good enough for now if that's okay for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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:
libp2pis executor agnostic (but uses tokio 1.0 in examples/tests), other async subsystems use … tokio 0.2, andjsonrpcis stuck on tokio 0.1. And now we addasync-stdhere (or is it already used in other tests too)?If the above is correct we clearly have some tech debt to pay off here. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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. 🤷♀️