Feat: Map Node Stats (Version, Operating System etc) to each Node in Feed #587

CyndieKamau · 2024-04-30T15:59:42Z

Hello, this PR is focused on providing a more detailed view of node metrics on the telemetry platform, by mapping the extra node info aggregated as a summary in ChainStats to specific nodes in the nodes' list table.

Some of the key changes implemented include:

Backend

Modified NodeDetails struct to include the extra information (target_os, target_arch, target_env, sysinfo) in common/src/node_types.rs
Updated the AddedNode payload to always expose the sysinfo details by default to get the additional stats (cpu, memory, core_count, linux_kernel, linux_distro, is_virtual_machine) found in NodeSysInfo

Frontend

Updated the Node constructor to handle the extra details within NodeDetails
Added new columns and components to display the newly available information

The objective is to provide telemetry users with deeper insights into the operational aspects of nodes. By exposing more detailed system information, users can better understand and track node operations.

cla-bot-2021 · 2024-04-30T15:59:51Z

User @CyndieKamau, please sign the CLA here.

jsdw · 2024-08-07T09:25:26Z

backend/telemetry_core/src/feed_message.rs

-            (&None, &None, &None)
-        };
+        let ip = if *expose_node_details { &details.ip } else { &None };
+        let sys_info = &details.sysinfo;


I'm not sure how I feel about sys_info no longer respecting the --expose-node-details flag. I think the point of this flag is to provide some privacy for nodes on the default telemetry instance, since they will connect to it by default (ie they aren't explicitly opting in to having their details shared). Then, on eg the 1kv telemetry instance we allow node details to be exposed since nodes must opt in to sharing telemetry there anyway.

It's debatable whether this privacy is required or not for the system info though. Perhaps there is a case for hiding IPs but exposing the hardware details, but I'd want to ask around a bit on this and gather some thoughts.

jsdw · 2024-08-07T09:27:33Z

backend/telemetry_core/src/state/chain_stats.rs

@@ -16,7 +16,7 @@

 use super::counter::{Counter, CounterValue};
 use crate::feed_message::ChainStats;
-
+//use arrayvec::ArrayString;


Nit: remove?

jsdw · 2024-08-07T09:32:00Z

backend/telemetry_core/src/state/counter.rs

 use crate::feed_message::Ranking;
-use std::collections::HashMap;
+use std::{collections::HashMap, /*str::FromStr*/};


Suggested change

use std::{collections::HashMap, /*str::FromStr*/};

use std::collections::HashMap;

jsdw · 2024-08-07T09:34:36Z

backend/common/src/node_types.rs

@@ -38,7 +38,7 @@ pub struct NodeDetails {
    pub validator: Option<Box<str>>,
    pub network_id: NetworkId,
    pub startup_time: Option<Box<str>>,
-    pub target_os: Option<Box<str>>,
+    pub target_os: Option<Box<str>>,   //starts here


Suggested change

pub target_os: Option<Box<str>>, //starts here

pub target_os: Option<Box<str>>,

jsdw · 2024-08-07T09:35:28Z

backend/telemetry_shard/src/json_message/mod.rs

@@ -19,5 +19,5 @@
 mod hash;
 mod node_message;

-pub use hash::Hash;
+//pub use hash::Hash;


Suggested change

//pub use hash::Hash;

jsdw · 2024-08-07T09:37:10Z

backend/telemetry_core/src/feed_message.rs

+/// Ranking list out node details elements per node id and counts per elements
+/// `list`: List of node detail element per its count [( item, count )]
+/// `node_map`: A map 


This comment doesn't make sense to me :)

jsdw · 2024-08-07T10:01:25Z

frontend/src/components/List/Row.css

@@ -44,5 +44,5 @@ along with this program. If not, see <https://www.gnu.org/licenses/>.
 }

 .Row:hover {
-  background-color: #1e1e1e;
+  background-color: #1e1e1e;  /*remember to change to #1e1e1e*/


Suggested change

background-color: #1e1e1e; /*remember to change to #1e1e1e*/

background-color: #1e1e1e;

jsdw · 2024-08-07T10:01:41Z

frontend/src/components/List/Row.tsx

@@ -86,6 +104,9 @@ export class Row extends React.Component<RowProps, RowState> {
  public render() {
    const { node, columns } = this.props;

+  console.log('node details:',node );


jsdw · 2024-08-07T10:01:58Z

frontend/src/App.tsx

@@ -71,6 +72,20 @@ export default class App extends React.Component {
        blockpropagation: true,
        blocklasttime: false,
        uptime: false,
+        version: true, //starts here


Suggested change

version: true, //starts here

version: true,

jsdw · 2024-08-07T10:02:21Z

frontend/src/Connection.ts

@@ -133,7 +133,7 @@ export class Connection {
    this.bindSocket();
  }

-  public subscribe(chain: Types.GenesisHash) {
+  public subscribe(chain: Types.GenesisHash) { //It has subscribed to the telemetry core


Remove or add proper comment above?

jsdw · 2024-08-07T10:02:55Z

frontend/src/state.ts

@@ -255,6 +291,22 @@ export interface StateSettings {
  blockpropagation: boolean;
  blocklasttime: boolean;
  uptime: boolean;
+
+  version: boolean; //starts here


Suggested change

version: boolean; //starts here

version: boolean;

jsdw · 2024-08-07T10:03:28Z

frontend/src/common/feed.ts

@@ -98,7 +98,8 @@ interface AddedNodeMessage extends MessageBase {
    NodeHardware,
    BlockDetails,
    Maybe<NodeLocation>,
-    Maybe<Timestamp>
+    Maybe<Timestamp>,
+


Remove empty line?

jsdw · 2024-08-07T10:04:48Z

backend/telemetry_core/src/state/counter.rs

+
+    /// The map of Node Id ( Network Id) to the node detail element
+    /// i.e {"123DE": "aarch64"}
+    node_map: HashMap<ArrayString<64>,K>


What's the reasoning behind adding this?

As far as I understand, the relevant properties are exposed in NodeDetails, and then they are sent to the UI already (some depending on --expose-node-details). So I wonder whether you can avoid modifying all of this chain stats/counter/ranking stuff? Offhand I can't see it being used anywhere in the frontend changes.

jsdw · 2024-08-07T10:09:45Z

Overall I'm happy to expose node system info for each node. Whether or not it should only be when --expose-node-details is set is still a question in my mind though.

There are also changes to the Counter type and related which look to be adding node details per network ID, but these details are already exposed and sent to the UI via AddedNode by the looks of it, so would be good to justify or scrap those changes (especially since they involve more HashMaps and cloning of things that would be best to avoid if we can.

CyndieKamau · 2024-08-07T12:25:21Z

Hey @jsdw thank you for the reviews!!!

Yeah actually we first tried mapping each node ID to the extra set of details needed (cpu, cpuarch etc), that was before we had understood the entire codebase and the specific payloads !!! :)

Yeah in hindsight I should have just scraped off those changes and left the latest ones. Let me do just that and get back to you! ^^

jsdw · 2024-08-08T10:11:35Z

That sounds great to me; thank you!

CyndieKamau · 2024-08-08T17:10:42Z

Hey @jsdw I have made the changes requested! ^^
I've scraped off the unnecessary code and the personal comments I missed out

jsdw · 2024-08-09T10:39:24Z

backend/telemetry_core/src/feed_message.rs

+
+

Nit; remove extra lines

Suggested change

jsdw · 2024-08-09T10:39:53Z

backend/common/src/node_types.rs

@@ -38,7 +38,7 @@ pub struct NodeDetails {
    pub validator: Option<Box<str>>,
    pub network_id: NetworkId,
    pub startup_time: Option<Box<str>>,
-    pub target_os: Option<Box<str>>,
+    pub target_os: Option<Box<str>>, 


Nit: remove unnecessary space?

Suggested change

pub target_os: Option<Box<str>>,

pub target_os: Option<Box<str>>,

jsdw · 2024-08-09T10:40:29Z

backend/telemetry_core/src/state/chain_stats.rs

@@ -16,7 +16,6 @@

 use super::counter::{Counter, CounterValue};
 use crate::feed_message::ChainStats;
-


Add back newline between imports and constants?

jsdw · 2024-08-09T10:41:05Z

backend/telemetry_core/src/feed_message.rs

+
 #[derive(Serialize)]
 pub struct ChainStatsUpdate<'a>(pub &'a ChainStats);

+


Nit: Unnecessary newlines?

Suggested change

#[derive(Serialize)]

pub struct ChainStatsUpdate<'a>(pub &'a ChainStats);

#[derive(Serialize)]

pub struct ChainStatsUpdate<'a>(pub &'a ChainStats);

jsdw · 2024-08-09T10:41:32Z

backend/telemetry_core/src/state/chain_stats.rs

@@ -162,10 +161,10 @@ impl ChainStatsCollator {
        self.version.modify(Some(&*details.version), op);

        self.target_os
-            .modify(details.target_os.as_ref().map(|value| &**value), op);
+        .modify(details.target_os.as_ref().map(|value| &**value), op);


Suggested change

.modify(details.target_os.as_ref().map(|value| &**value), op);

.modify(details.target_os.as_ref().map(|value| &**value), op);

jsdw · 2024-08-09T10:41:39Z

backend/telemetry_core/src/state/chain_stats.rs


        self.target_arch
-            .modify(details.target_arch.as_ref().map(|value| &**value), op);
+        .modify(details.target_arch.as_ref().map(|value| &**value), op);


Suggested change

.modify(details.target_arch.as_ref().map(|value| &**value), op);

.modify(details.target_arch.as_ref().map(|value| &**value), op);

jsdw · 2024-08-09T10:41:48Z

backend/telemetry_core/src/state/chain_stats.rs

@@ -179,7 +178,7 @@ impl ChainStatsCollator {
        self.memory.modify(memory.as_ref(), op);

        self.core_count
-            .modify(sysinfo.and_then(|sysinfo| sysinfo.core_count.as_ref()), op);
+        .modify(sysinfo.and_then(|sysinfo| sysinfo.core_count.as_ref()), op);


Suggested change

.modify(sysinfo.and_then(|sysinfo| sysinfo.core_count.as_ref()), op);

.modify(sysinfo.and_then(|sysinfo| sysinfo.core_count.as_ref()), op);

jsdw · 2024-08-09T10:42:52Z

Thanks!

I haven't looked through it all yet but would run cargo fmt on the rust codebase to clear up a bunch of spacing and indentation nits / make CI pass :)

CyndieKamau · 2024-08-15T00:41:00Z

Hey @jsdw apologies I was held up abit. So I've formatted both frontend and backend, what's left is actually some e2e tests which are failing:

test e2e_max_nodes_per_connection_is_enforced
test e2e_feeds_told_about_chain_rename_and_stay_subscribed
test e2e_feed_can_subscribe_and_unsubscribe_from_chain

Upon quick introspection I can see its because there's a mismatch between the expected and actual data structure when decoding the JSON message, since we altered it to fit the new inputs.

The solution would be to update the tests to handle this new structure, but otherwise it would be great to hear your opinion on this! Thanks :)

jsdw · 2024-08-23T16:05:53Z

Hey @CyndieKamau, no worries!

Having a quick look at the tests, I think it's just that this enum needs updating to match the feed message updates you did:

substrate-telemetry/backend/test_utils/src/feed_message_de.rs

Line 25 in bb4c727

pub enum FeedMessage {

(I can't remember why it's a separate struct now, but hopefully it's pretty straightforward to tweak to match the new expected structure!)

Added new item in Ranking node_map, mapping node id to node detail Co-authored-by: Cyndie Kamau <[email protected]>

MrishoLukamba · 2024-08-26T14:25:12Z

Hey @CyndieKamau, no worries!

Having a quick look at the tests, I think it's just that this enum needs updating to match the feed message updates you did:

substrate-telemetry/backend/test_utils/src/feed_message_de.rs

Line 25 in bb4c727

pub enum FeedMessage {

(I can't remember why it's a separate struct now, but hopefully it's pretty straightforward to tweak to match the new expected structure!)

@jsdw fixed it, ready to be merged

MrishoLukamba · 2024-08-29T12:06:48Z

@jsdw any pendings that needs to be addressed for the PR to be merged?

jsdw · 2024-08-29T12:36:47Z

Hey! I'll have to have another proper look over the code, but great to see the tests passing now, nice one!

I do see a .idea folder now added and wonder whether that's best removed?

MrishoLukamba · 2024-08-29T14:06:30Z

Hey! I'll have to have another proper look over the code, but great to see the tests passing now, nice one!

I do see a .idea folder now added and wonder whether that's best removed?

removed it

jsdw · 2024-08-29T16:26:01Z

I opened #591 to address a few nits with this. Feel free to merge that into here (I can't easily do this) or we can continue from there!

One thing I noticed is that when I run a polkadot --dev node locally and point it to this telemetry server, I don't see any data for most of the new columns. Do you? I'm not sure what data nodes send by default, but need to check that we actually get data from nodes before we can merge this.

niklasad1 · 2024-09-25T13:24:51Z

#591 is merged so closing this one, thanks

jsdw reviewed Aug 7, 2024

View reviewed changes

jsdw reviewed Aug 9, 2024

View reviewed changes

MrishoLukamba and others added 2 commits August 26, 2024 14:37

added functionality for specifying node details per each node

6cd6b0e

Backend done

30c9081

Added new item in Ranking node_map, mapping node id to node detail Co-authored-by: Cyndie Kamau <[email protected]>

CyndieKamau and others added 7 commits August 26, 2024 14:37

feat: last frontend working version

efd4ef6

chore: Clean up unused code

276fa60

fix(frontend): update node details to carry 10 fields

7ae78ff

chore: remove unnecessary code

8071a3d

chore: run cargo fmt for formatting

3dfb3da

chore: run prettier to format frontend

50d3a74

fixed e2e tests added missing struct params

aecd20b

MrishoLukamba force-pushed the master branch from be7f7e6 to aecd20b Compare August 26, 2024 14:22

remoted .idea file

c69e044

jsdw mentioned this pull request Aug 29, 2024

Feat: Map Node Stats (Version, Operating System etc) to each Node in Feed #591

Merged

niklasad1 closed this Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Map Node Stats (Version, Operating System etc) to each Node in Feed #587

Feat: Map Node Stats (Version, Operating System etc) to each Node in Feed #587

CyndieKamau commented Apr 30, 2024

cla-bot-2021 bot commented Apr 30, 2024

jsdw Aug 7, 2024 •

edited

Loading

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw Aug 7, 2024

jsdw commented Aug 7, 2024

CyndieKamau commented Aug 7, 2024 •

edited

Loading

jsdw commented Aug 8, 2024

CyndieKamau commented Aug 8, 2024

jsdw Aug 9, 2024 •

edited

Loading

jsdw Aug 9, 2024

jsdw Aug 9, 2024

jsdw Aug 9, 2024

jsdw Aug 9, 2024

jsdw Aug 9, 2024

jsdw Aug 9, 2024

jsdw commented Aug 9, 2024

CyndieKamau commented Aug 15, 2024

jsdw commented Aug 23, 2024

MrishoLukamba commented Aug 26, 2024

MrishoLukamba commented Aug 29, 2024

jsdw commented Aug 29, 2024

MrishoLukamba commented Aug 29, 2024

jsdw commented Aug 29, 2024

niklasad1 commented Sep 25, 2024

	use std::{collections::HashMap, /str::FromStr/};
	use std::collections::HashMap;

	pub target_os: Option<Box<str>>, //starts here
	pub target_os: Option<Box<str>>,

	background-color: #1e1e1e; /remember to change to #1e1e1e/
	background-color: #1e1e1e;

		@@ -16,7 +16,6 @@

		use super::counter::{Counter, CounterValue};
		use crate::feed_message::ChainStats;


		#[derive(Serialize)]
		pub struct ChainStatsUpdate<'a>(pub &'a ChainStats);

	.modify(details.target_os.as_ref().map(\|value\| &**value), op);
	.modify(details.target_os.as_ref().map(\|value\| &**value), op);

	.modify(details.target_arch.as_ref().map(\|value\| &**value), op);
	.modify(details.target_arch.as_ref().map(\|value\| &**value), op);

	.modify(sysinfo.and_then(\|sysinfo\| sysinfo.core_count.as_ref()), op);
	.modify(sysinfo.and_then(\|sysinfo\| sysinfo.core_count.as_ref()), op);

Feat: Map Node Stats (Version, Operating System etc) to each Node in Feed #587

Feat: Map Node Stats (Version, Operating System etc) to each Node in Feed #587

Conversation

CyndieKamau commented Apr 30, 2024

Backend

Frontend

cla-bot-2021 bot commented Apr 30, 2024

jsdw Aug 7, 2024 • edited Loading

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

jsdw commented Aug 7, 2024

CyndieKamau commented Aug 7, 2024 • edited Loading

jsdw commented Aug 8, 2024

CyndieKamau commented Aug 8, 2024

jsdw Aug 9, 2024 • edited Loading

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

jsdw commented Aug 9, 2024

CyndieKamau commented Aug 15, 2024

jsdw commented Aug 23, 2024

MrishoLukamba commented Aug 26, 2024

MrishoLukamba commented Aug 29, 2024

jsdw commented Aug 29, 2024

MrishoLukamba commented Aug 29, 2024

jsdw commented Aug 29, 2024

niklasad1 commented Sep 25, 2024

jsdw Aug 7, 2024 •

edited

Loading

CyndieKamau commented Aug 7, 2024 •

edited

Loading

jsdw Aug 9, 2024 •

edited

Loading