[Feature][Metrics] Add resource download related metrics for workers by EricGao888 · Pull Request #10749 · apache/dolphinscheduler

EricGao888 · 2022-07-03T16:41:20Z

Purpose of the pull request

Added resource download related metrics for workers.
Added related docs and grafana-demos
related: [DSIP-8][Metrics] Improve DolphinScheduler Monitoring #9324

Brief change log

Added following metrics:

ds.worker.resource.download.count, sliced by tag status
ds.worker.resource.download.duration
ds.worker.resource.download.size

Verify this pull request

Verified by manual test.

codecov-commenter · 2022-07-03T16:55:44Z

Codecov Report

Merging #10749 (534c647) into dev (aa8b88a) will increase coverage by 0.05%.
The diff coverage is 83.78%.

❗ Current head 534c647 differs from pull request most recent head d444411. Consider uploading reports for the commit d444411 to get more accurate results

@@             Coverage Diff              @@
##                dev   #10749      +/-   ##
============================================
+ Coverage     40.13%   40.19%   +0.05%     
- Complexity     4843     4844       +1     
============================================
  Files           940      940              
  Lines         36945    36982      +37     
  Branches       4033     4033              
============================================
+ Hits          14828    14864      +36     
+ Misses        20630    20625       -5     
- Partials       1487     1493       +6

Impacted Files	Coverage Δ
...eduler/server/worker/runner/TaskExecuteThread.java	`19.25% <66.66%> (+1.42%)`	⬆️
...ler/server/worker/metrics/WorkerServerMetrics.java	`72.91% <87.09%> (+72.91%)`	⬆️
...er/master/dispatch/host/assign/RandomSelector.java	`77.77% <0.00%> (-5.56%)`	⬇️
...e/dolphinscheduler/remote/NettyRemotingClient.java	`50.00% <0.00%> (-1.39%)`	⬇️
...org/apache/dolphinscheduler/remote/utils/Host.java	`45.65% <0.00%> (+2.17%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa8b88a...d444411. Read the comment docs.

EricGao888 · 2022-07-04T02:33:01Z

Codecov Report

Merging #10749 (16b7bb4) into dev (4ac3349) will increase coverage by 0.08%.
The diff coverage is 88.63%.
@@             Coverage Diff              @@
##                dev   #10749      +/-   ##
============================================
+ Coverage     40.71%   40.79%   +0.08%     
- Complexity     4820     4825       +5     
============================================
  Files           900      900              
  Lines         36143    36187      +44     
  Branches       3982     3982              
============================================
+ Hits          14717    14764      +47     
+ Misses        19972    19969       -3     
  Partials       1454     1454              
Impacted Files Coverage Δ
...pache/dolphinscheduler/common/utils/FileUtils.java 51.42% <0.00%> (-1.52%) ⬇️
...eduler/server/worker/runner/TaskExecuteThread.java 21.32% <85.71%> (+3.49%) ⬆️
...ler/server/worker/metrics/WorkerServerMetrics.java 78.84% <94.28%> (+78.84%) ⬆️
Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ac3349...16b7bb4. Read the comment docs.

I didn't add a line of UT, not sure why code coverage increased 🤣 .

dolphinscheduler-meter/src/main/resources/grafana/DolphinSchedulerWorker.json

dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java

...ker/src/main/java/org/apache/dolphinscheduler/server/worker/metrics/WorkerServerMetrics.java

...worker/src/main/java/org/apache/dolphinscheduler/server/worker/runner/TaskExecuteThread.java

dolphinscheduler-meter/src/main/resources/grafana/DolphinSchedulerWorker.json

...ker/src/main/java/org/apache/dolphinscheduler/server/worker/metrics/WorkerServerMetrics.java

EricGao888 · 2022-07-11T05:49:42Z

I will submit another PR some time this week to switch to use tags to indicate status instead of putting them in names for all the previous metrics in master and worker. #10867

...ker/src/main/java/org/apache/dolphinscheduler/server/worker/metrics/WorkerServerMetrics.java

…pache#9324)

… download metrics (apache#9324)

…9324)

…e size in metrics (apache#9324)

kezhenxu94

Generally LGTM. Some nits

kezhenxu94 · 2022-07-11T09:15:55Z

dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java

+    public static long getFileSizeInByte(String filename) {
+        File file = new File(filename);
+        return file.length();
+    }


This is one-line code, just inline this util method and remove this method?

new File(filename).length()

kezhenxu94 · 2022-07-11T09:18:27Z

...worker/src/main/java/org/apache/dolphinscheduler/server/worker/runner/TaskExecuteThread.java

+                WorkerServerMetrics.recordWorkerResourceDownloadSize(
+                        FileUtils.getFileSizeInByte(execLocalPath + File.separator + fullName));


Suggested change

WorkerServerMetrics.recordWorkerResourceDownloadSize(

FileUtils.getFileSizeInByte(execLocalPath + File.separator + fullName));

WorkerServerMetrics.recordWorkerResourceDownloadSize(

Files.size(Paths.get(execLocalPath, fullName)));

Pretty and neat! Have fixed it in the latest commit. Thx

sonarqubecloud · 2022-07-11T11:47:17Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

83.8% Coverage
0.0% Duplication

EricGao888 · 2022-07-11T11:58:56Z

CI seems somehow not stable today. I've pushed multiple times and it finally passes.

EricGao888 · 2022-07-12T02:15:16Z

@ruanwenjun @zhongjiajie Could you please take another look when available? Thx~

ruanwenjun

LGTM

kezhenxu94 · 2022-07-12T03:48:10Z

Hi @EricGao888 a hint. When merging pull request please take a minute to cleanup/rewrite the commit message so that we can have a more clean git history

EricGao888 · 2022-07-12T03:55:48Z

Hi @EricGao888 a hint. When merging pull request please take a minute to cleanup/rewrite the commit message so that we can have a more clean git history

@kezhenxu94 Thanks for the hint! I will follow this guide next time when merging commits https://dolphinscheduler.apache.org/en-us/community/development/commit-message.html

ruanwenjun · 2022-07-12T04:04:37Z

Yes, we need to clean up some messages,
e.g.

Fix demos, docs and remove redundant code
Fix style check
Make code neat
...

EricGao888 · 2022-07-12T05:10:42Z

Yes, we need to clean up some messages, e.g.
Fix demos, docs and remove redundant code
Fix style check
Make code neat
...

Thx for the explanation. I will remove those redundant commit msgs next time. : )

…pache#10749) * [Feature][Metrics] Add resource download related metrics for workers (apache#9324) * [Feature][Metrics] Fix bugs and add grafana demos for worker resource download metrics (apache#9324) * [Feature][Metrics] Add docs to resource related metrics (apache#9324) * [Feature][Metrics] Use tags to indicate status in metrics (apache#9324) * [Feature][Metrics] Fix demos, docs and remove redundant code (apache#9324) * [Feature][Metrics] Remove .pnpm-debug.log (apache#9324) * [Feature][Metrics] Fix style check (apache#9324) * [Feature][Metrics] Replace KB with bytes for the unit of resource file size in metrics (apache#9324) * [Feature][Metrics] Make code neat (apache#9324)

SbloodyS assigned EricGao888 Jul 4, 2022

SbloodyS added backend metrics labels Jul 4, 2022

SbloodyS added this to the 3.1.0-alpha milestone Jul 4, 2022

EricGao888 marked this pull request as ready for review July 5, 2022 00:49

EricGao888 requested review from SbloodyS, Tianqi-Dotes, caishunfeng, kezhenxu94, ruanwenjun and zhongjiajie as code owners July 5, 2022 00:49

EricGao888 commented Jul 5, 2022

View reviewed changes

dolphinscheduler-meter/src/main/resources/grafana/DolphinSchedulerWorker.json Outdated Show resolved Hide resolved

EricGao888 force-pushed the Fix-9324-add-resource-related-metrics branch from 1580074 to a4e2bc7 Compare July 5, 2022 02:17

zhongjiajie reviewed Jul 6, 2022

View reviewed changes

dolphinscheduler-common/src/main/java/org/apache/dolphinscheduler/common/utils/FileUtils.java Outdated Show resolved Hide resolved

...ker/src/main/java/org/apache/dolphinscheduler/server/worker/metrics/WorkerServerMetrics.java Outdated Show resolved Hide resolved

ruanwenjun requested changes Jul 6, 2022

View reviewed changes

...worker/src/main/java/org/apache/dolphinscheduler/server/worker/runner/TaskExecuteThread.java Outdated Show resolved Hide resolved

kezhenxu94 reviewed Jul 6, 2022

View reviewed changes

dolphinscheduler-meter/src/main/resources/grafana/DolphinSchedulerWorker.json Outdated Show resolved Hide resolved

...ker/src/main/java/org/apache/dolphinscheduler/server/worker/metrics/WorkerServerMetrics.java Outdated Show resolved Hide resolved

EricGao888 mentioned this pull request Jul 9, 2022

[Improvement][Metrics] Use tags to indicate task / workflow execution status for metrics #10867

Closed

3 tasks

EricGao888 force-pushed the Fix-9324-add-resource-related-metrics branch from a4e2bc7 to a0dfc58 Compare July 11, 2022 05:45

github-actions bot added the document label Jul 11, 2022

EricGao888 force-pushed the Fix-9324-add-resource-related-metrics branch from a0dfc58 to 73cd48f Compare July 11, 2022 05:51

kezhenxu94 reviewed Jul 11, 2022

View reviewed changes

...ker/src/main/java/org/apache/dolphinscheduler/server/worker/metrics/WorkerServerMetrics.java Outdated Show resolved Hide resolved

EricGao888 added 6 commits July 11, 2022 17:15

[Feature][Metrics] Add resource download related metrics for workers (a…

2fbe26c

…pache#9324)

[Feature][Metrics] Fix bugs and add grafana demos for worker resource…

ff70c7a

… download metrics (apache#9324)

[Feature][Metrics] Add docs to resource related metrics (apache#9324)

dcd3bb1

[Feature][Metrics] Use tags to indicate status in metrics (apache#9324)

b273981

[Feature][Metrics] Fix demos, docs and remove redundant code (apache#…

8158741

…9324)

[Feature][Metrics] Remove .pnpm-debug.log (apache#9324)

02ce24b

EricGao888 added 2 commits July 11, 2022 17:15

[Feature][Metrics] Fix style check (apache#9324)

f86b4b4

[Feature][Metrics] Replace KB with bytes for the unit of resource fil…

4ec5890

…e size in metrics (apache#9324)

EricGao888 force-pushed the Fix-9324-add-resource-related-metrics branch from 7c0d4e1 to 4ec5890 Compare July 11, 2022 09:15

kezhenxu94 previously approved these changes Jul 11, 2022

View reviewed changes

EricGao888 dismissed kezhenxu94’s stale review via 5171f34 July 11, 2022 09:29

[Feature][Metrics] Make code neat (apache#9324)

d444411

EricGao888 force-pushed the Fix-9324-add-resource-related-metrics branch from 5171f34 to d444411 Compare July 11, 2022 11:06

EricGao888 requested review from kezhenxu94 and ruanwenjun July 11, 2022 11:58

kezhenxu94 approved these changes Jul 11, 2022

View reviewed changes

EricGao888 requested a review from zhongjiajie July 11, 2022 12:02

ruanwenjun approved these changes Jul 12, 2022

View reviewed changes

EricGao888 merged commit 2f7281c into apache:dev Jul 12, 2022

EricGao888 mentioned this pull request Jul 25, 2022

[DSIP-8][Metrics] Improve DolphinScheduler Monitoring #9324

Closed

14 tasks

		WorkerServerMetrics.recordWorkerResourceDownloadSize(
		FileUtils.getFileSizeInByte(execLocalPath + File.separator + fullName));

Conversation

EricGao888 commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

Brief change log

Verify this pull request

Uh oh!

codecov-commenter commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

EricGao888 commented Jul 4, 2022

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EricGao888 commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kezhenxu94 left a comment

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

EricGao888 Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 11, 2022

Uh oh!

EricGao888 commented Jul 11, 2022

Uh oh!

EricGao888 commented Jul 12, 2022

Uh oh!

ruanwenjun left a comment

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricGao888 commented Jul 12, 2022

Uh oh!

ruanwenjun commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricGao888 commented Jul 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EricGao888 commented Jul 3, 2022 •

edited

Loading

codecov-commenter commented Jul 3, 2022 •

edited

Loading

EricGao888 commented Jul 11, 2022 •

edited

Loading

kezhenxu94 commented Jul 12, 2022 •

edited

Loading

ruanwenjun commented Jul 12, 2022 •

edited

Loading