Telemetry per node #7463

cecton · 2020-10-29T13:59:07Z

polkadot companion: paritytech/polkadot#1948

bkchr

Looks good so far.

And it is really good that we got rid off slog :)

client/service/src/config.rs

client/telemetry/src/layer.rs

client/telemetry/src/lib.rs

cecton · 2020-10-30T17:22:18Z

client/telemetry/src/layer.rs

+			// NOTE: this is a hack to inject the span id into the json
+			let mut message = format!(r#"{{"id":{},"#, (*self.0).id);
+			message.push_str(&value[1..]);
+			(*self.0).json = Some(message)


@bkchr 😱 😱 😱

I think I should open an issue on tracing to see if they plan to do something to allow passing anything through the key-values of the log. If we had that, I could serialize the json later when I actually have that id.

cecton · 2020-11-05T16:16:24Z

Linking this here: old-storyai/tracing-wasm#13

TL;DR

Telemetry in the browser needs some update. It works already! But the logs look completely different because it's not using the same library than before.

client/service/src/logging.rs

client/service/src/builder.rs

client/telemetry/src/lib.rs

client/telemetry/src/worker.rs

client/telemetry/src/worker/node_pool.rs

bkchr

Thank you for all the hard work. 👍

Some last nitpicks are left and than we are good to merge this IMHO.

bkchr · 2021-01-15T16:04:50Z

client/telemetry/src/lib.rs

+					for addr in addresses {
+						if let Some(node) = node_pool.get_mut(&addr) {
+							node.telemetry_connection_notifier
+								.push(connection_notifier.clone());


Suggested change

.push(connection_notifier.clone());

.push(connection_notifier);

Can't! It's in a loop

bkchr · 2021-01-15T16:10:21Z

client/telemetry/src/lib.rs

+			let input = if let Some(x) = input {
+				x
+			} else {
+				log::error!(
 					target: "telemetry",
-					"The telemetry seems to be polled multiple times simultaneously"
+					"Unexpected end of stream. This is a bug.",
 				);
-				// Returning `Pending` here means that we may never get polled again, but this is
-				// ok because we're in a situation where something else is actually currently doing
-				// the polling.
-				return Poll::Pending;
+				return;
+			};


Can be an expect, as we hold sender/receiver and this can not happen.

Fixed in f462418

bkchr · 2021-01-15T16:10:31Z

client/telemetry/src/lib.rs

+			let (id, verbosity, message) = if let Some(x) = input {
+				x
+			} else {
+				log::error!(
+					target: "telemetry",
+					"Unexpected end of stream. This is a bug.",
+				);
+				return;
+			};


Fixed in f462418

bkchr · 2021-01-15T16:10:41Z

client/telemetry/src/lib.rs

+						addr,
+						verbosity,
+					);
+					return;


Suggested change

return;

continue;

😱 Fixed in f6ec36d

bkchr · 2021-01-15T16:13:04Z

client/telemetry/src/lib.rs

+							Node::new(transport.clone(), addr.clone(), Vec::new(), Vec::new())
+						});
+
+						node.connection_messages.extend(connection_message.clone());


Suggested change

node.connection_messages.extend(connection_message.clone());

node.connection_messages.extend(connection_message);

Can't, it's in a loop

bkchr · 2021-01-15T16:14:10Z

client/telemetry/src/lib.rs

+		let mut node_pool: HashMap<Multiaddr, _> = HashMap::new();
+
+		// initialize the telemetry nodes and register new notifiers
+		async fn process_register(


Perfect 👍

Now just move this and the other function below out of the run function :P

Done in 24596be

(...like that??)

bkchr · 2021-01-15T16:51:33Z

client/telemetry/src/lib.rs

@@ -16,339 +16,486 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.

-//! Telemetry utilities.
+//! To start using this module, please initialize the global logger from `sc-tracing`. This will


I would really like to have these docs more a high level description on how the system works.

This means:

How does it work with the span

How does the communication works with the telemetry worker

etc

I just imagine some sort of "diagram" explaining it :D

However, I will not make this a hard requirement to merge this.

Does this look better? acf1ec8

More doc bc36136

cecton · 2021-01-20T11:28:35Z

bot merge

ghost · 2021-01-20T11:28:43Z

Checks failed; merge aborted.

dvdplm

Was half-way through when I noticed the PR was merged. Feel free to open a followup with the suggested changes (and let me know if I should do the second half).

dvdplm · 2021-01-20T11:12:05Z

client/cli/src/config.rs

-			disable_log_color,
-		})?;
+		let mut logger = GlobalLoggerBuilder::new(self.log_filters()?);
+		logger.with_log_reloading(!self.is_log_filter_reloading_disabled()?);


Suggested change

logger.with_log_reloading(!self.is_log_filter_reloading_disabled()?);

let reloadable = !self.is_log_filter_reloading_disabled().expect("Hardcoded Ok; qed");

logger.with_log_reloading(reloadable);

…but I think the booleans should be flipped so we'd have logger.with_log_reloading(self.log_reloading).

The origin of these funny booleans isn't this PR, but maybe we can avoid extending the awkwardness and have to do one bool flip here, and then another one in GlobalLoggerBuilder::with_log_reloading(). I'd rename GlobalLoggerBuilder.disable_log_reloading to just log_reloading: bool and work backwards from there.

dvdplm · 2021-01-20T11:36:57Z

client/cli/src/runner.rs

+
+	/// Get a new [`TelemetryHandle`].
+	///
+	/// This is used when you want to register a new telemetry for a Substrate node.


A new telemetry… what? Telemetry endpoint? Telemetry worker? Telemetry channel? Please elaborate (same on handle()).

dvdplm · 2021-01-20T11:40:27Z

client/service/src/builder.rs

+	let telemetry_span = if config.telemetry_endpoints.is_some() {
+		Some(TelemetrySpan::new())
+	} else {
+		None
+	};


Suggested change

let telemetry_span = if config.telemetry_endpoints.is_some() {

Some(TelemetrySpan::new())

} else {

None

};

let telemetry_span = config.telemetry_endpoints.as_ref().map(|_| TelemetrySpan::new() );

(same for full client)

dvdplm · 2021-01-20T11:49:05Z

client/service/src/config.rs

+		match self.telemetry_endpoints.as_ref() {
+			// Don't initialise telemetry if `telemetry_endpoints` == Some([])
+			Some(endpoints) if !endpoints.is_empty() => Some(endpoints),
+			_ => None,
+		}


Suggested change

match self.telemetry_endpoints.as_ref() {

// Don't initialise telemetry if `telemetry_endpoints` == Some([])

Some(endpoints) if !endpoints.is_empty() => Some(endpoints),

_ => None,

}

// Don't initialise telemetry if `telemetry_endpoints` == Some([])

self.telemetry_endpoints

.as_ref()

.filter(|endpoints| endpoints.is_empty())```

dvdplm · 2021-01-20T11:50:44Z

client/service/src/task_manager/mod.rs

@@ -228,14 +233,17 @@ pub struct TaskManager {
 	/// terminates and gracefully shutdown. Also ends the parent `future()` if a child's essential
 	/// task fails.
 	children: Vec<TaskManager>,
+	/// A telemetry handle used to enter the telemetry span when a task is spawned.


This is a bit confusing; it's a TelemetrySpan, not a TelemetryHandle. Can you reword this?

Oh no, that's because I changed the implementation at some point. Before that the handle also handled the span

dvdplm · 2021-01-20T11:56:14Z

client/telemetry/README.md


-> **Note**: Cloning the [`Telemetry`] and polling from multiple clones has an unspecified behaviour.
+Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`].


Suggested change

Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`].

Substrate's nodes initialize/register with the [`TelemetryWorker`] using a [`TelemetryHandle`].

dvdplm · 2021-01-20T11:56:38Z

client/telemetry/README.md

-> **Note**: Cloning the [`Telemetry`] and polling from multiple clones has an unspecified behaviour.
+Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`].
+This handle can be cloned and passed around. It uses an asynchronous channel to communicate with
+the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry can happen at


Suggested change

the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry can happen at

the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry source can happen at

?

dvdplm · 2021-01-20T11:57:29Z

client/telemetry/src/endpoints.rs

+/// The URL string can be either a URL or a multiaddress.
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)]
+pub struct TelemetryEndpoints(
+	#[serde(deserialize_with = "url_or_multiaddr_deser")] pub(crate) Vec<(Multiaddr, u8)>,


Suggested change

#[serde(deserialize_with = "url_or_multiaddr_deser")] pub(crate) Vec<(Multiaddr, u8)>,

#[serde(deserialize_with = "url_or_multiaddr_deser")]

pub(crate) Vec<(Multiaddr, u8)>,

dvdplm · 2021-01-20T11:59:02Z

client/telemetry/src/layer.rs

+impl TelemetryLayer {
+	/// Create a new [`TelemetryLayer`] and [`TelemetryWorker`].
+	///
+	/// If not provided, the `buffer_size` will be 16 by default.


Suggested change

/// If not provided, the `buffer_size` will be 16 by default.

/// The `buffer_size` defaults to 16.

dvdplm · 2021-01-20T12:00:53Z

client/telemetry/src/lib.rs

-	worker: Option<worker::TelemetryWorker>,
-	/// Receives log entries for them to be dispatched to the worker.
-	receiver: mpsc::Receiver<async_record::AsyncRecord>,
+/// It should be ran as a background task using the [`TelemetryWorker::run`] method. This method


Suggested change

/// It should be ran as a background task using the [`TelemetryWorker::run`] method. This method

/// It should run as a background task using the [`TelemetryWorker::run`] method. This method

cecton · 2021-01-20T12:08:36Z

@dvdplm np I will make another PR

* Adapt service creation to new substrate API * Fix test * WIP * Revert "WIP" This reverts commit 816a363. * WIP * Adapt to current code * Fix tests * Yet another fix * CLEANUP * WIP * WIP * WIP * Adapt code to changes on substrate * Adapt code * Introduce kick. * Fixes * WIP * WIP * WIP * WIP * Bump * Update sp-io * WIP Co-authored-by: Gav Wood <[email protected]> Co-authored-by: Parity Benchmarking Bot <[email protected]>

dvdplm

Some more comments.

One slight concern I have that I'm not sure is valid is: there are a few places where you note that it's important to not call things out of order or at most once. It's a bit of a code smell and I wonder if we can do better than that, OnceCell-something-something. More something to ponder than a request for change.

client/tracing/src/logging/layers/prefix_layer.rs

client/tracing/src/logging/mod.rs

client/tracing/src/logging/directives.rs

* master: Companion for sub/7040 (#1719) Companion PR for paritytech/substrate#7463 (#1948) Bump serde from 1.0.119 to 1.0.120 (#2292)

cecton · 2021-01-27T12:53:52Z

One slight concern I have that I'm not sure is valid is: there are a few places where you note that it's important to not call things out of order or at most once. It's a bit of a code smell and I wonder if we can do better than that, OnceCell-something-something. More something to ponder than a request for change.

Sorry I forgot to answer you.

Now that the telemetry can be started in the middle of anything I think the only thing that remains is if you start the telemetry twice for the same substrate node. It's not really breaking, it will just extend the existing connections if necessary but I think it will add another connection message so every time the connection re-establishes it will send 2 connection messages instead of one. I could fix that to make it override the previous connection message.

* Adapt service creation to new substrate API * Fix test * WIP * Revert "WIP" This reverts commit 816a3633e91abc943b12b2bfa77ce98b959e78b2. * WIP * Adapt to current code * Fix tests * Yet another fix * CLEANUP * WIP * WIP * WIP * Adapt code to changes on substrate * Adapt code * Introduce kick. * Fixes * WIP * WIP * WIP * WIP * Bump * Update sp-io * WIP Co-authored-by: Gav Wood <[email protected]> Co-authored-by: Parity Benchmarking Bot <[email protected]>

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 29, 2020

cecton added 7 commits October 29, 2020 17:40

WIP

59429ab

Serialize instead of stringify

c7e824a

WIP

28a79a0

WIP

6ac4ea8

CLEANUP

6d6da6d

CLEANUP

e1bd592

CLEANUP

389da31

bkchr reviewed Oct 30, 2020

View reviewed changes

cecton added 5 commits October 30, 2020 16:49

Re-using telemetry nodes

4602bcf

Refactoring guarding object

aadae1c

Inject span id in the json (not elegant)

520c38b

Merge commit 3664cdb (no conflict)

fe28d76

Merge commit cab9865 (conflicts)

ab089f9

cecton force-pushed the cecton-telemetry-without-global branch from b801f26 to ab089f9 Compare October 30, 2020 17:04

Fix passing transport argument

7385c62

cecton commented Oct 30, 2020

View reviewed changes

maciejhirsz mentioned this pull request Oct 30, 2020

Parachain multiplexing paritytech/substrate-telemetry#295

Merged

2 tasks

cecton added 4 commits November 4, 2020 10:16

CLEANUP

cc41d72

Spawn 2 nodes instead of one

a5c64fd

WIP

79f05ac

WIP

24f3c0d

cecton mentioned this pull request Nov 5, 2020

RPC to allow setting the log filter #7474

Merged

5 tasks

WIP

1476571

cecton added 3 commits November 6, 2020 13:51

Use proper FormatEvent in the browser

295a18e

Smol fix

2f40937

CLEANUP

b17aa9d

bkchr reviewed Nov 6, 2020

View reviewed changes

client/service/src/logging.rs Outdated Show resolved Hide resolved

dvdplm reviewed Nov 9, 2020

View reviewed changes

Merge commit 8ee55dd (conflicts)

842c4d7

cecton marked this pull request as ready for review January 15, 2021 15:22

cecton added 3 commits January 15, 2021 16:38

Fix line width

cd716e4

Added missing code adaptation

36ec61b

Merge commit 946c0e1 (no conflict)

def0202

bkchr approved these changes Jan 15, 2021

View reviewed changes

cecton added 2 commits January 20, 2021 09:55

Merge commit b64ec19 (no conflict)

fbc58c4

Move inner functions out of run function

24596be

cecton marked this pull request as draft January 20, 2021 09:01

cecton added 4 commits January 20, 2021 10:04

Fixed incorrect return of function

f6ec36d

Change error to expect

f462418

Another attempt to have a higher level doc

acf1ec8

More doc

bc36136

cecton marked this pull request as ready for review January 20, 2021 09:38

cecton added the B0-silent Changes should not be mentioned in any release notes label Jan 20, 2021

cecton merged commit 8f43280 into master Jan 20, 2021

cecton deleted the cecton-telemetry-without-global branch January 20, 2021 11:28

dvdplm suggested changes Jan 20, 2021

View reviewed changes

cecton mentioned this pull request Jan 20, 2021

Doc fixes for sc-telemetry & API struct rename #7934

Merged

dvdplm reviewed Jan 20, 2021

View reviewed changes

dvdplm mentioned this pull request Jan 22, 2021

Fix telemetry span not entering properly & enter span in sc-cli #7951

Merged

ordian added a commit to paritytech/polkadot that referenced this pull request Jan 23, 2021

Merge branch 'master' into ao-approval-distribution-impl

f42874d

* master: Companion for sub/7040 (#1719) Companion PR for paritytech/substrate#7463 (#1948) Bump serde from 1.0.119 to 1.0.120 (#2292)

dvdplm mentioned this pull request Jan 25, 2021

Telemetry spam #7960

Closed

	.push(connection_notifier.clone());
	.push(connection_notifier);

	node.connection_messages.extend(connection_message.clone());
	node.connection_messages.extend(connection_message);

	logger.with_log_reloading(!self.is_log_filter_reloading_disabled()?);
	let reloadable = !self.is_log_filter_reloading_disabled().expect("Hardcoded Ok; qed");
	logger.with_log_reloading(reloadable);


		> Note: Cloning the [`Telemetry`] and polling from multiple clones has an unspecified behaviour.
		Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`].

	Substrate's nodes initialize/register to the [`TelemetryWorker`] using a [`TelemetryHandle`].
	Substrate's nodes initialize/register with the [`TelemetryWorker`] using a [`TelemetryHandle`].

	the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry can happen at
	the running [`TelemetryWorker`] dedicated to registration. Registering a telemetry source can happen at

	#[serde(deserialize_with = "url_or_multiaddr_deser")] pub(crate) Vec<(Multiaddr, u8)>,
	#[serde(deserialize_with = "url_or_multiaddr_deser")]
	pub(crate) Vec<(Multiaddr, u8)>,

	/// If not provided, the `buffer_size` will be 16 by default.
	/// The `buffer_size` defaults to 16.

	/// It should be ran as a background task using the [`TelemetryWorker::run`] method. This method
	/// It should run as a background task using the [`TelemetryWorker::run`] method. This method

Telemetry per node #7463

Telemetry per node #7463

Conversation

cecton commented Oct 29, 2020 • edited Loading

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Nov 5, 2020

bkchr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jan 20, 2021

ghost commented Jan 20, 2021

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jan 20, 2021

dvdplm left a comment

Choose a reason for hiding this comment

cecton commented Jan 27, 2021

cecton commented Oct 29, 2020 •

edited

Loading