report udp stats from validator #20587

jbiseda · 2021-10-11T11:35:35Z

Problem

UDP stats are collected from net-stats.sh which isn't commonly run by validators.

Summary of Changes

Collect UDP stats from the validator process.

Fixes #

codecov · 2021-10-11T13:47:54Z

Codecov Report

Merging #20587 (5ec0d9a) into master (ad0a88f) will increase coverage by 0.0%.
The diff coverage is 84.5%.

@@           Coverage Diff            @@
##           master   #20587    +/-   ##
========================================
  Coverage    81.8%    81.9%            
========================================
  Files         495      496     +1     
  Lines      138041   138138    +97     
========================================
+ Hits       113049   113168   +119     
+ Misses      24992    24970    -22

brooksprumo

Overall the idea looks good to me. I have some minor comments on tweaks that I've left below. I haven't looked at getting these stats before, so my review assumes that part is correct. Lastly, I'll defer to @carllin for things like (1) the process/module name, and (2) how frequently this task should run (i.e. sleep & sample interval).

core/src/system_monitor_service.rs

brooksprumo · 2021-10-11T16:58:16Z

core/src/system_monitor_service.rs

+    thread_hdl: JoinHandle<()>,
+}
+
+#[allow(dead_code)]


There are a few spots where #[allow(dead_code)] has been added, but I wasn't sure why. Is it needed for the non-linux case due to the #[cfg(not(target_os = "linux"))] spots?

Yeah, this is due to the #[cfg(not(target_os = "linux"))] spots. I didn't want to remove these for non-linux so that the unit test can run on non-linux platforms. This seemed cleaner than prefixing with "_". Is there a better way to annotate this?

Makes sense. Unfortunately I don't know of a better way to handle this.

core/src/system_monitor_service.rs

brooksprumo

This all looks good to me. I think someone else should give a final Approve, since I don't have a good picture of the whole system to know if there are any unknown-to-me negative interactions due to adding a new service.

You know, maybe adding a CLI arg like --no-system-monitor to disable this might be a good idea? From what I understand, most voting/staked validators turn off metrics, so it seems like turning off this service might be a reasonable request.

@carllin What do you think?

carllin · 2021-10-12T00:03:48Z

core/src/system_monitor_service.rs

+
+    #[cfg(target_os = "linux")]
+    fn report_net_stats(udp_stats: &mut Option<UdpStats>) {
+        match read_udp_stats("/proc/net/snmp") {


Nit: Extract /proc/net/snmp into a constant string above

On startup of this service in new(), could we verify that we have access to this path, and if not, return an error and fail validator startup

Oh! This path could be a CLI arg too.

@carllin would we want to fail startup? It will currently log a WARN message. I don't think access would typically be a problem. /proc/net is a symlink to /proc/self/net which is owned by the process.

@brooksprumo I don't think this path would change unless you hacked and recompiled your kernel. Changing this path would break things like netstat.

@jbiseda Gotcha. I guess if we ever need to support a different path it'll be easy to add it then.

@jbiseda yeah people have a tendency to not check their logs :P, I think a hard failure so that validators don't silently underreport is a good idea

@carllin made this a fatal error or startup

@jbiseda, hmm I dont see the change, is this pushed?

@carllin pushed now

jbiseda · 2021-10-12T01:23:38Z

You know, maybe adding a CLI arg like --no-system-monitor to disable this might be a good idea? From what I understand, most voting/staked validators turn off metrics, so it seems like turning off this service might be a reasonable request.

I'm adding this here because we really would like to have these stats and validators aren't running the net-stats.sh script. I think from the last incident there were only 2 validators that had reported these OS level stats. I would suspect that if you didn't want to report stats you would just leave SOLANA_METRICS_CONFIG unset.

core/src/system_monitor_service.rs

Pull request has been modified.

core/src/system_monitor_service.rs

t-nelson

thanks!

(cherry picked from commit 4cac662) # Conflicts: # core/src/validator.rs

* report udp stats from validator (#20587) (cherry picked from commit 4cac662) # Conflicts: # core/src/validator.rs * resolve merge conflicts Co-authored-by: Jeff Biseda <jbiseda@gmail.com>

This reverts commit 46133b6.

report udp stats from validator

af77934

vercel bot temporarily deployed to Preview October 11, 2021 11:35 Inactive

fix checks

4826b01

vercel bot temporarily deployed to Preview October 11, 2021 11:46 Inactive

jbiseda marked this pull request as ready for review October 11, 2021 11:57

jbiseda requested review from brooksprumo and carllin October 11, 2021 11:57

brooksprumo reviewed Oct 11, 2021

View reviewed changes

review

75210b6

vercel bot temporarily deployed to Preview October 11, 2021 21:03 Inactive

jbiseda requested a review from brooksprumo October 11, 2021 21:18

brooksprumo reviewed Oct 11, 2021

View reviewed changes

carllin reviewed Oct 12, 2021

View reviewed changes

const snmp path

77a395a

vercel bot temporarily deployed to Preview October 12, 2021 01:39 Inactive

t-nelson reviewed Oct 12, 2021

View reviewed changes

core/src/system_monitor_service.rs Outdated Show resolved Hide resolved

separate stats reporting

cb64b0b

vercel bot temporarily deployed to Preview October 12, 2021 14:14 Inactive

change annotation

38e833b

vercel bot temporarily deployed to Preview October 12, 2021 14:16 Inactive

scope

90270d1

vercel bot temporarily deployed to Preview October 12, 2021 15:11 Inactive

checks

d60adce

vercel bot temporarily deployed to Preview October 12, 2021 15:38 Inactive

jbiseda requested review from t-nelson and carllin October 12, 2021 16:21

jbiseda added 2 commits October 14, 2021 14:09

Merge branch 'master' into net-stats

5d1a5e5

failure to access stats is a fatal error on startup

a8ef5ce

carllin previously approved these changes Oct 15, 2021

View reviewed changes

add ok(())

77f1736

t-nelson reviewed Oct 15, 2021

View reviewed changes

core/src/system_monitor_service.rs Outdated Show resolved Hide resolved

core/src/system_monitor_service.rs Show resolved Hide resolved

annotations

5ec0d9a

t-nelson approved these changes Oct 15, 2021

View reviewed changes

carllin approved these changes Oct 15, 2021

View reviewed changes

jbiseda merged commit 4cac662 into solana-labs:master Oct 15, 2021

jbiseda added the v1.8 label Oct 19, 2021

mergify bot pushed a commit that referenced this pull request Oct 19, 2021

report udp stats from validator (#20587)

0cb601f

(cherry picked from commit 4cac662) # Conflicts: # core/src/validator.rs

mergify bot mentioned this pull request Oct 19, 2021

report udp stats from validator (backport #20587) #20799

Merged

ix42 mentioned this pull request Nov 12, 2021

The UDP monitoring feature prevents the validator starting on WSL #21253

Closed

dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021

report udp stats from validator (solana-labs#20587)

46133b6

frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021

Revert "report udp stats from validator (solana-labs#20587)"

1ae614e

This reverts commit 46133b6.

jbiseda deleted the net-stats branch January 31, 2023 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report udp stats from validator #20587

report udp stats from validator #20587

jbiseda commented Oct 11, 2021 •

edited

Loading

codecov bot commented Oct 11, 2021 •

edited

Loading

brooksprumo left a comment •

edited

Loading

brooksprumo Oct 11, 2021

jbiseda Oct 11, 2021

brooksprumo Oct 11, 2021

brooksprumo left a comment

carllin Oct 12, 2021 •

edited

Loading

carllin Oct 12, 2021 •

edited

Loading

brooksprumo Oct 12, 2021

jbiseda Oct 12, 2021

brooksprumo Oct 12, 2021

carllin Oct 14, 2021

jbiseda Oct 15, 2021

carllin Oct 15, 2021

jbiseda Oct 15, 2021

jbiseda commented Oct 12, 2021

t-nelson left a comment

report udp stats from validator #20587

report udp stats from validator #20587

Conversation

jbiseda commented Oct 11, 2021 • edited Loading

Problem

Summary of Changes

codecov bot commented Oct 11, 2021 • edited Loading

Codecov Report

brooksprumo left a comment • 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

brooksprumo left a comment

Choose a reason for hiding this comment

carllin Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

carllin Oct 12, 2021 • 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

jbiseda commented Oct 12, 2021

t-nelson left a comment

Choose a reason for hiding this comment

jbiseda commented Oct 11, 2021 •

edited

Loading

codecov bot commented Oct 11, 2021 •

edited

Loading

brooksprumo left a comment •

edited

Loading

carllin Oct 12, 2021 •

edited

Loading

carllin Oct 12, 2021 •

edited

Loading