[#333] feat: expose metrics of TOP10 app bytes in one shuffle server by lifeSo · Pull Request #1400 · apache/uniffle

lifeSo · 2023-12-27T15:55:46Z

What changes were proposed in this pull request?

expose metrics of TOP10 app bytes in one shuffle server #333

Why are the changes needed?

Fix:
#333
#681

Does this PR introduce any user-facing change?

No.

How was this patch tested?

unit test

codecov-commenter · 2023-12-28T21:57:16Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (0630dc5) 53.39% compared to head (84ab744) 55.16%.

Files	Patch %	Lines
...org/apache/uniffle/server/ShuffleFlushManager.java	72.72%	2 Missing and 1 partial ⚠️
.../java/org/apache/uniffle/server/ShuffleServer.java	50.00%	0 Missing and 1 partial ⚠️
...ava/org/apache/uniffle/server/ShuffleTaskInfo.java	92.30%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1400      +/-   ##
============================================
+ Coverage     53.39%   55.16%   +1.77%     
- Complexity     2730     2773      +43     
============================================
  Files           422      403      -19     
  Lines         24052    21817    -2235     
  Branches       2051     2057       +6     
============================================
- Hits          12842    12035     -807     
+ Misses        10410     9044    -1366     
+ Partials        800      738      -62

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zuston · 2023-12-29T10:27:24Z

I think I will review this in the next monday

zuston · 2024-01-02T09:50:32Z

    if (executorService != null) {
      executorService.shutdownNow();
    }
+    shuffleTaskManager.stop();


if the shuffleTaskManager is not identified as final, you should check null

shuffleTaskManager is assigned in constructor and initialization() method.
I added the null check to make code more robustness

zuston · 2024-01-02T09:51:26Z

  private StorageManager storageManager;
  private HealthCheck healthCheck;
-  private Set<String> tags = Sets.newHashSet();
+  private final Set<String> tags = Sets.newHashSet();


why changing this?

There is IDEA tips and want to make code better, so I changed it.

Finally, I reverted the changes.

zuston · 2024-01-02T09:52:48Z

                  + "Default is 0, OS will dynamically adjust the buf size.");

+  public static final ConfigOption<Integer> SERVER_SHUFFLE_DATA_METRIC_TOP_NUMBER =
+      ConfigOptions.key("rss.server.shuffle.data.metric.top.number")


The name is not accurate, what's the shuffle data metric? I think here you want to expose the topN of app level total shuffle size.

Ok， I changed the code like this:

public static final ConfigOption<Integer> SERVER_SHUFFLE_DATA_METRIC_TOP_NUMBER = ConfigOptions.key("rss.server.shuffle.data.metric.top.number") .intType() .defaultValue(10) .withDescription( "size of topN when calc max shuffle data in memory、on disk、remote HDFS or total."); public static final ConfigOption<Integer> SERVER_SHUFFLE_DATA_METRIC_REFRESH_INTERVAL = ConfigOptions.key("rss.server.shuffle.data.metric.refresh.interval") .intType() .defaultValue(1000) .withDescription("Shuffle data metric refresh interval in ms");

zuston · 2024-01-02T09:52:56Z

+              "size of topN when calc max shuffle data in memory、on disk、remote HDFS or total.");
+
+  public static final ConfigOption<Integer> SERVER_SHUFFLE_DATA_METRIC_REFRESH_INTERVAL =
+      ConfigOptions.key("rss.server.shuffle.data.metric.refresh.interval")


Ok, I changed it.

zuston · 2024-01-02T09:57:29Z

  private static final String TOTAL_REMOVE_RESOURCE_BY_SHUFFLE_IDS_TIME =
      "total_remove_resource_by_shuffle_ids_time";

+  private static final String TOP_N_TOTAL_SHUFFLE_DATA = "top_N_total_shuffle_data";


this all metrics newly introduced are not consistent with their names.

I think the

topN_of_total_data_size_for_app
topN_of_on_memory_data_size_for_app
topN_of_on_localfile_data_size_for_app
topN_of_on_hadoop_data_size_for_app

How about this?

I'm not sure these metrics are about app?

this metric is from ShuffleTaskManager.shuffleTaskInfos, it maintain the app level shuffle data size info.

this all metrics newly introduced are not consistent with their names.

I think the

topN_of_total_data_size_for_app topN_of_on_memory_data_size_for_app topN_of_on_localfile_data_size_for_app topN_of_on_hadoop_data_size_for_app

How about this?

Ok, I change to it. it is more clear

How about change 'on memory' to 'in memory', for there is no 'on memory'.

zuston · 2024-01-02T09:58:26Z

+  private final AtomicLong totalDataSize = new AtomicLong(0);
+  private final AtomicLong totalDataSizeInMemory = new AtomicLong(0);
+  private final AtomicLong totalDataSizeOnDisk = new AtomicLong(0);
+  private final AtomicLong totalDataSizeRemote = new AtomicLong(0);


remote is not accurate. I hope the naming should be careful, hadoop is more accurate

change to hadoop is ok and I changed, but if use obs file system.

zuston · 2024-01-02T10:01:21Z

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ShuffleDataExporterTask {


The class name is not accurate.

Ok, I changed to TopNShuffleDataSizeOfAppCalcTask

zuston

Please attach the metrics shown in the prometheus. And the tests cases are needed.

zuston · 2024-01-05T02:04:16Z

+          .withDescription("size of TopN total shuffle data size of app level.");
+
+  public static final ConfigOption<Integer> TOP_N_SHUFFLE_DATA_SIZE_OF_APP_LEVEL_REFRESH_INTERVAL =
+      ConfigOptions.key("rss.server.topN.shuffle.data.size.of.app-level.refresh.interval")


How about rss.server.topN.appShuffleDataSize.refresh.interval ?

How about this refer to: #1363 (comment)

rss.server.topN.appShuffleDataSize.refreshIntervalMs

And it depends on you.

zuston · 2024-01-05T02:04:58Z

                  + "Default is 0, OS will dynamically adjust the buf size.");

+  public static final ConfigOption<Integer> TOP_N_SHUFFLE_DATA_SIZE_OF_APP_LEVEL =
+      ConfigOptions.key("rss.server.topN.shuffle.data.size.of.app-level")


How about rss.server.topN.appShuffleDataSize.number ?

ok, it is better.

zuston · 2024-01-05T02:07:01Z

            .build();

+    startClearResourceThread();
+    memoryExporter =


memoryExporter is not accurate. Please rename this.

ok, I rename it to: topNShuffleDataSizeOfAppCalcTask, I think it should be ok.

zuston · 2024-01-05T02:09:49Z

+        Gauge.build()
+            .name(TOPN_OF_TOTAL_DATA_SIZE_FOR_APP)
+            .help("top N of total shuffle data size for app level")
+            .labelNames("topn_of_total_data_size_for_app")


the label name is wrong. If we hope this could be shown to find out the app and sorted number, these labels should be taken.

@zuston

This is the first initialization in setUpMetrics() in ShuffleServerMetrics.java, and to make gaugeDataSizeTotalUsage not be null.

The real metric cals is in incStorageSuccessCounter() in TopNShuffleDataSizeOfAppCalcTask.java.
eg:

List<Map.Entry<String, ShuffleTaskInfo>> topNTaskInfo = calcTopNTotalDataSizeTaskInfo(); gaugeTotalDataSize.clear(); for (Map.Entry<String, ShuffleTaskInfo> taskInfo : topNTaskInfo) { gaugeTotalDataSize.labels(taskInfo.getKey()).set(taskInfo.getValue().getTotalDataSize()); }

In order to no make misunderstand， how about I delete the init and add null check ?

Or How about code:

Gauge.build() .name(TOPN_OF_TOTAL_DATA_SIZE_FOR_APP) .help("top N of total shuffle data size for app level") .labelNames("")

labelNames is set to "".

Sorry, I tried this, and get error when run the code.

zuston · 2024-01-05T02:11:09Z

    }
  }
+
+  public List<Map.Entry<String, ShuffleTaskInfo>> calcTopNTotalDataSizeTaskInfo() {


How about putting these calcTopN methods into TopNShuffleDataSizeOfAppCalcTask

I original want to encapsulate shuffleTaskInfos in ShuffleTaskManager.
If do as what you say, shuffleTaskInfos will be public through get method().

I will modify the code to your idea.

When RSS is started and there is no app:

when run some app:

I added test case TopNShuffleDataSizeOfAppCalcTaskTest.java

zuston

Overall LGTM. Sorry for the later review @lifeSo

I only left the minor comment about the metrics label. Please help correct it. Thanks

zuston · 2024-01-12T02:19:14Z

+        Gauge.build()
+            .name(TOPN_OF_TOTAL_DATA_SIZE_FOR_APP)
+            .help("top N of total shuffle data size for app level")
+            .labelNames("TOPN_OF_TOTAL_DATA_SIZE_FOR_APP")


rename to app_id

zuston · 2024-01-12T02:19:21Z

+        Gauge.build()
+            .name(TOPN_OF_IN_MEMORY_DATA_SIZE_FOR_APP)
+            .help("top N of in memory shuffle data size for app level")
+            .labelNames("TOPN_OF_IN_MEMORY_DATA_SIZE_FOR_APP")


zuston · 2024-01-12T02:19:27Z

+        Gauge.build()
+            .name(TOPN_OF_ON_LOCALFILE_DATA_SIZE_FOR_APP)
+            .help("top N of on disk shuffle data size for app level")
+            .labelNames("TOPN_OF_ON_LOCALFILE_DATA_SIZE_FOR_APP")


zuston · 2024-01-12T02:19:33Z

+        Gauge.build()
+            .name(TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP)
+            .help("top N of on hadoop shuffle data size for app level")
+            .labelNames("TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP")


zuston

LGTM. Thanks @lifeSo

…uffle server (apache#1400) expose metrics of TOP10 app bytes in one shuffle server apache#333 Fix: apache#333 apache#681 No. unit test

lifeSo added 30 commits December 11, 2023 21:51

添加堆外内存监控的代码

7ab778f

format code

df92f55

format code

1759f9a

file rename

01872d0

rename

a0f5ea6

format

84df9a4

format

0a9a90d

format

7ca74cf

Merge branch 'apache:master' into master

47de654

change Coordinator to singleton like ShuffleServerClientFactory.java

1badd71

format

ece53d4

format

9f63b32

format

e211fdd

format

850f432

format

4c84928

modify to singleton

34834a8

format

71a7e5c

use synchronized

5dcd06d

format

31c0128

for test

3a34b4e

fix

efdb4e1

fix

a7e7e64

fix

39539d0

for test

c871e63

for test

51310d3

test

a90f9b7

fix and test

667f54c

fix

caaac6f

format

e61da50

for test

99dc725

lifeSo requested a review from zuston December 28, 2023 04:48

lifeSo marked this pull request as ready for review December 29, 2023 00:36

roryqi changed the title ~~[#333 ] feat: expose metrics of TOP10 app bytes in one shuffle server #333~~ [#333 ] feat: expose metrics of TOP10 app bytes in one shuffle server Dec 29, 2023

lifeSo changed the title ~~[#333 ] feat: expose metrics of TOP10 app bytes in one shuffle server~~ [#333] feat: expose metrics of TOP10 app bytes in one shuffle server Dec 29, 2023

zuston reviewed Jan 2, 2024

View reviewed changes

lifeSo requested a review from zuston January 3, 2024 08:26

zuston reviewed Jan 5, 2024

View reviewed changes

lifeSo force-pushed the feature_333_expose_app_metrics branch 10 times, most recently from e38e518 to 39a2fc8 Compare January 7, 2024 14:58

expose metrics of TOP10 app bytes in one shuffle server

b039800

lifeSo force-pushed the feature_333_expose_app_metrics branch from 39a2fc8 to b039800 Compare January 7, 2024 15:03

zuston reviewed Jan 12, 2024

View reviewed changes

modify label name to 'app_id'

d17d1a4

lifeSo force-pushed the feature_333_expose_app_metrics branch from a998b80 to d17d1a4 Compare January 12, 2024 04:32

lifeSo added 3 commits January 12, 2024 12:38

fix merge conflict

32d0208

Merge branch 'master' into feature_333_expose_app_metrics

ad90763

fix

84ab744

zuston approved these changes Jan 12, 2024

View reviewed changes

zuston merged commit b43bbd8 into apache:master Jan 12, 2024

Uh oh!

Conversation

lifeSo commented Dec 27, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

codecov-commenter commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuston commented Dec 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifeSo Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

codecov-commenter commented Dec 28, 2023 •

edited

Loading

lifeSo Jan 4, 2024 •

edited

Loading

lifeSo Jan 7, 2024 •

edited

Loading

lifeSo Jan 2, 2024 •

edited

Loading

lifeSo Jan 5, 2024 •

edited

Loading

lifeSo Jan 5, 2024 •

edited

Loading

lifeSo Jan 5, 2024 •

edited

Loading

lifeSo Jan 5, 2024 •

edited

Loading

lifeSo Jan 5, 2024 •

edited

Loading