Add fs iotime in Nodes Stats API #67861

easyice · 2021-01-22T13:57:19Z

Related to #67805

This PR add io_time_in_millis in Nodes stats API, In the benchmark, it works almost as well as the iostat command

step 1,Perform random reads and writes on disk

fio -filename=/data00/tmp/1Gb.file -direct=1 -iodepth 1 -thread -rw=randrw -ioengine=libaio -bs=4k -size=10G -numjobs=2 -runtime=1000 -group_reporting -name=mytest

step 2, Compare the results of the io_time and iostat commands,use this script

while true
do
 ts1=`echo $(($(date +%s%N)/1000000))`
 iotime1=`curl -s "127.0.0.1:9200/_nodes/stats/fs?pretty" |grep io_time_in_millis | head -n 1 | awk '{print $3}'`
 sleep 5
 iotime2=`curl -s "127.0.0.1:9200/_nodes/stats/fs?pretty" |grep io_time_in_millis | head -n 1 | awk '{print $3}'`
 ts2=`echo $(($(date +%s%N)/1000000))`

 echo "scale=2;($iotime2-$iotime1)/($ts2-$ts1)*100" | bc
done

the result:

elasticmachine · 2021-01-26T18:58:01Z

Pinging @elastic/es-core-features (Team:Core/Features)

mark-vieira · 2021-02-03T00:16:12Z

@elasticmachine update branch

andreidan

Thanks for working on this @easyice. LGTM

andreidan · 2021-03-18T14:49:37Z

@elasticmachine ok to test

andreidan · 2021-03-18T14:49:48Z

@elasticmachine update branch

andreidan

@easyice I've just left a comment regarding adding backwards compatibility checks for this change. Thanks!

andreidan · 2021-03-18T15:08:41Z

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java

+            out.writeLong(currentIOTime);
+            out.writeLong(previousIOTime);


We'll need to read/write the new fields in a backwards compatible way (similar to how we do it here for eg. https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/AggProvider.java#L137 - there are probably other areas in the code). Let me know if you need any help with these version checks to ensure these fields are added in a BWC way.

Thanks for the detailed instructions, i added the compatibility checks ,use Version.V_7_13_0

andreidan · 2021-03-18T15:09:42Z

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java

+            currentIOTime = in.readLong();
+            previousIOTime = in.readLong();


Version checks for bwc are needed here as well

Thanks for review, Version checks is added

andreidan

Thanks for iterating on this @easyice

Apologies, I should've mentioned, at the time of merge we need the version checks to be against V8_0_0 (and I'll subsequently change them to 7_13_0 after the change is backported to that release line). Can you please modify the version checks?

Also, there are some checkstyles issues. We have more information on how to configure and run the checkstyle locally here - alternatively, running something along the lines of ./gradlew checkStyleMain checkStyleTest checkstyleInternalClusterTest checkstyleIntegTest checkstyleTestFixtures should help spot the issues

easyice · 2021-03-22T11:29:06Z

Thanks for @andreidan , I will pay more attention next time

andreidan

Thanks for fixing the version and the checkstyle

Added just a few more minor comments, this is close to complete.

Besides the comments there's also a test failure in org.elasticsearch.xpack.monitoring.collector.node.NodeStatsMonitoringDocTests.testToXContent

andreidan · 2021-03-22T11:57:16Z

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java


        final DeviceStats[] devicesStats;
        final long totalOperations;
        final long totalReadOperations;
        final long totalWriteOperations;
        final long totalReadKilobytes;
        final long totalWriteKilobytes;
+        long totalIOTimeInMillis;


can we make this final please? (explicitly initialising it to 0 on the else branch after the constructor version check here )

thanks you @andreidan, i will make it as final

andreidan · 2021-03-22T12:00:52Z

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java

+        long currentIOTime;
+        long previousIOTime;


Can we also mark these as final?

Okay, I've changed it to final, thank you for check!

easyice · 2021-03-22T15:25:40Z

Thanks for @andreidan
The test failure for NodeStatsMonitoringDocTests.testToXContent is fixed, but the io_stats field is not add in testToXContent function:

elasticsearch/x-pack/plugin/monitoring/src/test/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsMonitoringDocTests.java

Lines 277 to 283 in f0aa1b1

    
           + "          \"read_operations\": 4," 
        
           + "          \"write_operations\": 4," 
        
           + "          \"read_kilobytes\": 2," 
        
           + "          \"write_kilobytes\": 2" 
        
           + "        }" 
        
           + "      }" 
        
           + "    }"

and NodeStatsMonitoringDoc#XCONTENT_FILTERS

elasticsearch/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/collector/node/NodeStatsMonitoringDoc.java

Lines 115 to 120 in f0aa1b1

    
           "node_stats.fs.io_stats.total.write_operations", 
        
           "node_stats.fs.io_stats.total.read_kilobytes", 
        
           "node_stats.fs.io_stats.total.write_kilobytes", 
        
           "node_stats.fs.total.total_in_bytes", 
        
           "node_stats.fs.total.free_in_bytes", 
        
           "node_stats.fs.total.available_in_bytes",

Do these also need to be add the io_time_in_millis field?

cla-checker-service · 2021-03-22T16:37:49Z

💚 CLA has been signed

easyice · 2021-03-23T02:06:26Z

I'm sorry i use another computer push some code, which git config is not match this user and email, cause the cla check error, when I change to the correct configuration, the cla check still returns an error, can you please help me how to fix this issue? @andreidan

…make some variable as final

easyice · 2021-03-29T13:23:26Z

@andreidan the CLA check failed issue is fixed, Can you please take review again? Thanks!

andreidan

Sorry for the late reply @easyice (I was on holiday). Thanks for addressing the previous comments and for iterating on this! I think this is close to complete, I've left just a couple of super minor formatting comments after which this is complete

andreidan · 2021-04-12T12:25:05Z

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java

+            }
+            else{
+                this.totalIOTimeInMillis = 0;


super minor: there's a formatting issue here

andreidan · 2021-04-12T12:25:17Z

server/src/main/java/org/elasticsearch/monitor/fs/FsInfo.java

+            if (in.getVersion().onOrAfter(Version.V_8_0_0)) {
+                currentIOTime = in.readLong();
+                previousIOTime = in.readLong();
+            }else {


super minor: there's a formatting issue here

andreidan · 2021-04-12T12:28:24Z

@elasticmachine update branch

easyice · 2021-04-12T13:01:33Z

Thanks for @andreidan , happy holiday! the formatting issue is fixed

andreidan · 2021-05-05T09:25:48Z

@elasticmachine update branch

Disable bwc in preparation for merging #67861

andreidan

Thanks for your contribution and for iterating on this @easyice

LGTM

This adds io_time_in_millis to Nodes stats API (cherry picked from commit 71a9cfe) Signed-off-by: Andrei Dan <[email protected]>

This adds io_time_in_millis to Nodes stats API (cherry picked from commit 71a9cfe) Signed-off-by: Andrei Dan <[email protected]> Co-authored-by: zhangchao <[email protected]>

easyice · 2021-05-06T08:05:48Z

Thanks for your help @andreidan

Seems to be missing from elastic#67861 .

jtibshirani added the :Data Management/Stats Statistics tracking and retrieval APIs label Jan 26, 2021

elasticmachine added the Team:Data Management Meta label for data/management team label Jan 26, 2021

danhermann self-requested a review January 28, 2021 16:31

martijnvg removed the request for review from danhermann March 11, 2021 15:31

andreidan self-requested a review March 11, 2021 16:40

andreidan approved these changes Mar 18, 2021

View reviewed changes

andreidan added v7.13.0 v8.0.0 labels Mar 18, 2021

andreidan requested changes Mar 18, 2021

View reviewed changes

easyice requested a review from andreidan March 20, 2021 07:39

andreidan requested changes Mar 22, 2021

View reviewed changes

easyice force-pushed the feature/iotime branch from aac872c to a2b3900 Compare March 25, 2021 13:36

easyice added 9 commits March 25, 2021 22:49

Add fs iotime in Nodes Stats API

258a75a

added backwards compatibility checks

d6de28d

Add missing checks and fix build errors

a66da6b

update version checks to V_8_0_0 and fixed checkstyles issues

1a00c9d

fixed test failure in NodeStatsMonitoringDocTests.testToXContent and …

7eca84c

…make some variable as final

remove debug code

007f684

re-push because cla check failed

e898652

retry for cla check

db1090c

re-push because cla check failed

af20eb1

easyice force-pushed the feature/iotime branch from a2b3900 to 7d6a876 Compare March 25, 2021 14:50

easyice requested a review from andreidan March 28, 2021 08:14

andreidan reviewed Apr 12, 2021

View reviewed changes

elasticmachine and others added 2 commits April 12, 2021 08:28

Merge branch 'master' into feature/iotime

63d303f

fix code formatting issue

2a3034b

easyice requested a review from andreidan April 19, 2021 11:52

pugnascotia added >enhancement v7.14.0 and removed v7.13.0 labels Apr 21, 2021

Merge branch 'master' into feature/iotime

9f27fce

andreidan mentioned this pull request May 5, 2021

Disabe bwc tests #72750

Merged

andreidan added a commit that referenced this pull request May 5, 2021

Disabe bwc tests (#72750)

05baf4c

Disable bwc in preparation for merging #67861

andreidan approved these changes May 5, 2021

View reviewed changes

andreidan merged commit 71a9cfe into elastic:master May 5, 2021

andreidan pushed a commit to andreidan/elasticsearch that referenced this pull request May 5, 2021

Add fs iotime in Nodes Stats API (elastic#67861)

2c2e984

This adds io_time_in_millis to Nodes stats API (cherry picked from commit 71a9cfe) Signed-off-by: Andrei Dan <[email protected]>

andreidan mentioned this pull request May 5, 2021

[7.x] Add fs iotime in Nodes Stats API (#67861) #72754

Merged

stevejgordon mentioned this pull request Jul 1, 2021

7.14.0 Meta Ticket elastic/elasticsearch-net#5776

Closed

14 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

mindw added a commit to mindw/elasticsearch that referenced this pull request Mar 12, 2022

Add documentation for "io_time_in_millis"

a73170f

Seems to be missing from elastic#67861 .

mindw mentioned this pull request Mar 12, 2022

Add documentation for "io_time_in_millis" #84911

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fs iotime in Nodes Stats API #67861

Add fs iotime in Nodes Stats API #67861

easyice commented Jan 22, 2021 •

edited

Loading

elasticmachine commented Jan 26, 2021

mark-vieira commented Feb 3, 2021

andreidan left a comment

andreidan commented Mar 18, 2021

andreidan commented Mar 18, 2021

andreidan left a comment

andreidan Mar 18, 2021

easyice Mar 20, 2021

andreidan Mar 18, 2021

easyice Mar 20, 2021

andreidan left a comment

easyice commented Mar 22, 2021

andreidan left a comment •

edited

Loading

andreidan Mar 22, 2021

easyice Mar 22, 2021

andreidan Mar 22, 2021

easyice Mar 22, 2021

easyice commented Mar 22, 2021

cla-checker-service bot commented Mar 22, 2021 •

edited

Loading

easyice commented Mar 23, 2021

easyice commented Mar 29, 2021

andreidan left a comment

andreidan Apr 12, 2021

andreidan Apr 12, 2021

andreidan commented Apr 12, 2021

easyice commented Apr 12, 2021

andreidan commented May 5, 2021

andreidan left a comment

easyice commented May 6, 2021

		currentIOTime = in.readLong();
		previousIOTime = in.readLong();

Add fs iotime in Nodes Stats API #67861

Add fs iotime in Nodes Stats API #67861

Conversation

easyice commented Jan 22, 2021 • edited Loading

elasticmachine commented Jan 26, 2021

mark-vieira commented Feb 3, 2021

andreidan left a comment

Choose a reason for hiding this comment

andreidan commented Mar 18, 2021

andreidan commented Mar 18, 2021

andreidan 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

andreidan left a comment

Choose a reason for hiding this comment

easyice commented Mar 22, 2021

andreidan 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

Choose a reason for hiding this comment

easyice commented Mar 22, 2021

cla-checker-service bot commented Mar 22, 2021 • edited Loading

easyice commented Mar 23, 2021

easyice commented Mar 29, 2021

andreidan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan commented Apr 12, 2021

easyice commented Apr 12, 2021

andreidan commented May 5, 2021

andreidan left a comment

Choose a reason for hiding this comment

easyice commented May 6, 2021

easyice commented Jan 22, 2021 •

edited

Loading

andreidan left a comment •

edited

Loading

cla-checker-service bot commented Mar 22, 2021 •

edited

Loading