os_mon: allow check intervals smaller than 1 minute #6385

jdamanalo · 2022-10-22T17:15:58Z

Expanded type of disk_space_check_interval to number()

1> disksup:set_check_interval(0.5).

Fix #6283

CLAassistant · 2022-10-22T17:16:03Z

All committers have signed the CLA.

github-actions · 2022-10-22T17:16:54Z

CT Test Results

    2 files   15 suites 3m 55s ⏱️
111 tests 108 ✔️ 3 💤 0 ❌
130 runs 127 ✔️ 3 💤 0 ❌

Results for commit 3fa8a04.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

gomoripeti · 2022-10-22T17:41:37Z

lib/os_mon/src/disksup.erl

-param_type(disk_space_check_interval, Val) when is_integer(Val),
-						Val>=1 -> true;
+param_type(disk_space_check_interval, Val) when is_number(Val),
+						0<Val -> true;


not sure if this is a concern, but this seems to allow an effective check interval of zero if Val is smaller than 1 / 60_000

lukebakken · 2022-10-22T20:19:32Z

@jdamanalo thanks for getting this, it was my next task after #6384

Rather than use a float I was planning to allow a user to specify milliseconds via a tagged tuple:

{millisecond, 500}

...but that's different than what @rickard-green suggested here.

"fractional minutes" is just a weird unit to use IMHO.

jdamanalo · 2022-10-23T01:12:01Z

@lukebakken indeed, it is uncommon to have fractional minutes. I was just conscious of not adding a new "api". Nevertheless, I would be happy to implement the millisecond way. What do you think @rickard-green?

rickard-green

Regarding usage of a {millisecond, Time} tuple. If so, I would rather want it to be {TimeUnit, Time}, where TimeUnit is of the type erlang:time_unit(), than just the millisecond time unit. Also easy to implement using erlang:convert_time_unit/3 for conversion.

rickard-green · 2022-11-01T14:07:42Z

lib/os_mon/doc/src/disksup.xml

@@ -63,7 +63,7 @@
    <p>The following configuration parameters can be used to change
      the default values for time interval and threshold:</p>
    <taglist>
-      <tag><c>disk_space_check_interval = int()>0</c></tag>
+      <tag><c>disk_space_check_interval = float()>=1/60000</c></tag>


Suggested change

<tag><c>disk_space_check_interval = float()>=1/60000</c></tag>

<tag><c>disk_space_check_interval = number()>=1/60000</c></tag>

rickard-green · 2022-11-01T14:08:35Z

lib/os_mon/doc/src/disksup.xml

@@ -126,7 +126,7 @@
      <name since="">set_check_interval(Minutes) -> ok</name>
      <fsummary>Set time interval, in minutes, for the periodic disk space check</fsummary>
      <type>
-        <v>Minutes = int()>=1</v>
+        <v>Minutes = float()>=1/60000</v>


Suggested change

<v>Minutes = float()>=1/60000</v>

<v>Minutes = number()>=1/60000</v>

lukebakken · 2022-11-01T15:06:54Z

Thanks for the feedback @rickard-green

lib/os_mon/src/disksup.erl

lib/os_mon/doc/src/disksup.xml

rickard-green

I squashed and force pushed the doc fix.

We'll have a technical board meeting on Thursday where this PR will be brought up. I expect it to be accepted assuming we have no issues in testing.

- expand type of `disk_space_check_interval` to `Minute | {TimeUnit, Time}`

gomoripeti reviewed Oct 22, 2022

View reviewed changes

jdamanalo force-pushed the os_mon/disksup_interval_fraction branch from 4bf43e5 to 1743a31 Compare October 22, 2022 18:09

rickard-green added the team:VM Assigned to OTP team VM label Oct 24, 2022

rickard-green self-assigned this Oct 24, 2022

rickard-green requested changes Nov 1, 2022

View reviewed changes

rickard-green added the testing currently being tested, tag is used by OTP internal CI label Nov 1, 2022

jdamanalo force-pushed the os_mon/disksup_interval_fraction branch from 1743a31 to 0f45e4b Compare November 3, 2022 09:57

rickard-green requested changes Nov 7, 2022

View reviewed changes

lib/os_mon/src/disksup.erl Outdated Show resolved Hide resolved

lib/os_mon/doc/src/disksup.xml Outdated Show resolved Hide resolved

lib/os_mon/doc/src/disksup.xml Outdated Show resolved Hide resolved

jdamanalo force-pushed the os_mon/disksup_interval_fraction branch from 0f45e4b to 09539cf Compare November 8, 2022 12:17

rickard-green added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Nov 8, 2022

rickard-green requested changes Nov 8, 2022

View reviewed changes

lib/os_mon/doc/src/disksup.xml Outdated Show resolved Hide resolved

rickard-green added the testing currently being tested, tag is used by OTP internal CI label Nov 8, 2022

rickard-green force-pushed the os_mon/disksup_interval_fraction branch from 3304d34 to ec122a7 Compare November 8, 2022 17:03

rickard-green added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Nov 8, 2022

rickard-green approved these changes Nov 8, 2022

View reviewed changes

rickard-green added the testing currently being tested, tag is used by OTP internal CI label Nov 8, 2022

os_mon: allow check intervals smaller than 1 minute

3fa8a04

- expand type of `disk_space_check_interval` to `Minute | {TimeUnit, Time}`

rickard-green force-pushed the os_mon/disksup_interval_fraction branch from ec122a7 to 3fa8a04 Compare November 8, 2022 17:40

rickard-green added testing currently being tested, tag is used by OTP internal CI and removed testing currently being tested, tag is used by OTP internal CI labels Nov 8, 2022

rickard-green merged commit a8974b0 into erlang:maint Nov 10, 2022

rickard-green mentioned this pull request Nov 17, 2022

"disksup" should allow check intervals smaller than 1 minute #6283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os_mon: allow check intervals smaller than 1 minute #6385

os_mon: allow check intervals smaller than 1 minute #6385

jdamanalo commented Oct 22, 2022

CLAassistant commented Oct 22, 2022 •

edited

github-actions bot commented Oct 22, 2022 •

edited

gomoripeti Oct 22, 2022

jdamanalo Oct 22, 2022

lukebakken commented Oct 22, 2022 •

edited

jdamanalo commented Oct 23, 2022

rickard-green left a comment •

edited

rickard-green Nov 1, 2022

rickard-green Nov 1, 2022

lukebakken commented Nov 1, 2022

rickard-green left a comment

	<tag><c>disk_space_check_interval = float()>=1/60000</c></tag>
	<tag><c>disk_space_check_interval = number()>=1/60000</c></tag>

	<v>Minutes = float()>=1/60000</v>
	<v>Minutes = number()>=1/60000</v>

os_mon: allow check intervals smaller than 1 minute #6385

os_mon: allow check intervals smaller than 1 minute #6385

Conversation

jdamanalo commented Oct 22, 2022

CLAassistant commented Oct 22, 2022 • edited

github-actions bot commented Oct 22, 2022 • edited

CT Test Results

Artifacts

gomoripeti Oct 22, 2022

Choose a reason for hiding this comment

jdamanalo Oct 22, 2022

Choose a reason for hiding this comment

lukebakken commented Oct 22, 2022 • edited

jdamanalo commented Oct 23, 2022

rickard-green left a comment • edited

Choose a reason for hiding this comment

rickard-green Nov 1, 2022

Choose a reason for hiding this comment

rickard-green Nov 1, 2022

Choose a reason for hiding this comment

lukebakken commented Nov 1, 2022

rickard-green left a comment

Choose a reason for hiding this comment

CLAassistant commented Oct 22, 2022 •

edited

github-actions bot commented Oct 22, 2022 •

edited

lukebakken commented Oct 22, 2022 •

edited

rickard-green left a comment •

edited