Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Sep 25, 2024

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Aug 29, 2024

This builds on #587 and addresses a couple of nits:

  • Hide new oclumns by default; they take up too much space
  • Default to showing '-' in new columns when no data is available.

When I tested this locally, I saw no data for a bunch of the new columns and need to understand why that is before merging.

@CyndieKamau
Copy link
Contributor

Hey @jsdw I can answer that for you.

So when setting up a local dev node, depending on the type of OS for your machine then the output is different.

For instance, in macOS you can only see around 3 data columns, but for Linux all the columns are filled. This is something I also noted down and informed @bLd75 about.

So this setup was configured to collect more information from a linux environment than macos, so the telemetry server will get more details from linux than macos.

So to confirm everything was working we had to test on a linux environment.

@MrishoLukamba
Copy link
Contributor

This builds on #587 and addresses a couple of nits:

  • Hide new oclumns by default; they take up too much space
  • Default to showing '-' in new columns when no data is available.

When I tested this locally, I saw no data for a bunch of the new columns and need to understand why that is before merging.

We did it in a way that you have to toggle to actually display new columns. So by default they are hidden

@jsdw
Copy link
Collaborator Author

jsdw commented Aug 30, 2024

We did it in a way that you have to toggle to actually display new columns. So by default they are hidden

If you have already edited the telemetry settings, they are saved in localStorage and so it might be that that was the case for you, and so adding new things were hidden.

When I tested it, I had no settings saved and so they all showed up by default (see https://github.com/paritytech/substrate-telemetry/pull/591/files#diff-e56cb91573ddb6a97ecd071925fe26504bb5a65f921dc64c63e534162950e1ebR73-R83 where I set them to false by default)

@jsdw
Copy link
Collaborator Author

jsdw commented Aug 30, 2024

So to confirm everything was working we had to test on a linux environment.

Ah, thankyou for this! I'll point a linux based node to it when I'm back home and have my linux laptop to hand :)

@MrishoLukamba
Copy link
Contributor

MrishoLukamba commented Sep 16, 2024

So to confirm everything was working we had to test on a linux environment.

Ah, thankyou for this! I'll point a linux based node to it when I'm back home and have my linux laptop to hand :)

any updates on this, as it is ready to be merged @jsdw

@jsdw
Copy link
Collaborator Author

jsdw commented Sep 17, 2024

Sorry; this fell off my radar again! We'll aim to get it reviewed and, if all is well testing a linux node pointing to it, merged, in the coming days!

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

//Column for specifying type of distro each node is running (Linux Distribution)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Column for specifying type of distro each node is running (Linux Distribution)
// Column for specifying type of distro each node is running (Linux Distribution)

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

//Column for specifying which kernel each node is running on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Column for specifying which kernel each node is running on
// Column for specifying which kernel each node is running on

public static readonly label = 'version';
public static readonly icon = icon;
public static readonly width = 154;
public static readonly setting = 'version';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used anywhere?

@@ -33,3 +33,12 @@ export * from './BlockTimeColumn';
export * from './BlockPropagationColumn';
export * from './LastBlockColumn';
export * from './UptimeColumn';
export * from './CpuArchitectureColumn'; //extra columns added
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dq: but given new column follow the same structure and same pattern, can't they be factored out into one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm sure we could simplify this stuff a bunch!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think that shouldnt be the scope of this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

target_os,
target_arch,
target_env,
customNullField,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this is the reason for undefined earlier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@MrishoLukamba
Copy link
Contributor

wen merge? @jsdw @niklasad1 @pkhry

@jsdw jsdw merged commit 0cd8726 into master Sep 25, 2024
9 checks passed
@jsdw jsdw deleted the pr-587 branch September 25, 2024 13:01
@jsdw
Copy link
Collaborator Author

jsdw commented Sep 25, 2024

Merged :)

@CyndieKamau
Copy link
Contributor

Yay! Thank you @jsdw @niklasad1 @pkhry for the reviews! :)

@CyndieKamau
Copy link
Contributor

@jsdw I think we can safely close the first PR yes? It's still open

@jsdw
Copy link
Collaborator Author

jsdw commented Sep 26, 2024

Yup; FYI I have just deployed this to live, so the new columns are now available on telemetry.polkadot.io.

Thanks for the contribution guys; nice work!

@bLd75
Copy link

bLd75 commented Sep 26, 2024

Great job here, very useful infos added on nodes!
Just a remark: when adding several fields, names get truncated a lot in the list.
It seems the table was designed for a max size and name is the only adjustable column, it would be great if it was scrollable horizontally and keep full columns length.
image

@jsdw
Copy link
Collaborator Author

jsdw commented Sep 26, 2024

Yup, and scrolling to the right is naff too; I added #592 for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants