[#1189] feat(server): Add netty used direct memory size metric #1363

lifeSo · 2023-12-11T13:57:11Z

What changes were proposed in this pull request?

ShuffleServer add Direct Memory Metric for monitor

Why are the changes needed?

Fix: # (#1189)

Does this PR introduce any user-facing change?

No.

How was this patch tested?

zuston · 2023-12-12T02:00:24Z

server/src/main/java/org/apache/uniffle/server/DirectMemoryUsageReporter.java

+    service.scheduleAtFixedRate(
+        () -> {
+          try {
+            long usedDirectMemory = PlatformDependent.usedDirectMemory();


What's the difference with the metric of jvm_memory_bytes_used nonheap

We found "jvm_memory_bytes_used nonheap" is not correct, and it is small than direct memory.
When OutOfDirectMemory is throw，"jvm_memory_bytes_used nonheap" is small and not correct.

codecov-commenter · 2023-12-12T03:05:51Z

Codecov Report

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

Comparison is base (ecbf2e7) 53.22% compared to head (7ca74cf) 54.15%.
Report is 4 commits behind head on master.

Files	Patch %	Lines
...pache/uniffle/server/NettyDirectMemoryTracker.java	48.00%	13 Missing ⚠️
.../java/org/apache/uniffle/server/ShuffleServer.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1363      +/-   ##
============================================
+ Coverage     53.22%   54.15%   +0.92%     
- Complexity     2715     2717       +2     
============================================
  Files           418      399      -19     
  Lines         23932    21600    -2332     
  Branches       2043     2043              
============================================
- Hits          12738    11697    -1041     
+ Misses        10408     9189    -1219     
+ Partials        786      714      -72

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

zuston · 2023-12-12T03:25:59Z

server/src/main/java/org/apache/uniffle/server/DirectMemoryUsageReporter.java

+    service.scheduleAtFixedRate(
+        () -> {
+          try {
+            long usedDirectMemory = PlatformDependent.usedDirectMemory();


zuston · 2023-12-12T03:34:30Z

server/src/main/java/org/apache/uniffle/server/DirectMemoryUsageReporter.java

+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public class DirectMemoryUsageReporter {


This class name is not proper, how about NettyMemoryTracker ? And put this class into netty module ?

But the implement is get direct memory , not only netty.
Just only netty use direct memory.

If I change to NettyMemoryTracker, how about I use other component use direct memory

Anyway, I also think the nettyDirectMemoryTracker is better. And it needn't move file position

Ok, I corrented and renamed it .

zuston · 2023-12-12T03:34:56Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java

  public static Gauge.Child gaugeInFlushBufferSize;
  public static Gauge.Child gaugeUsedBufferSize;
  public static Gauge.Child gaugeReadBufferUsedSize;
+  public static Gauge.Child gaugeUsedDirectMemorySize;


Is this dedicated for netty?

For Shuffle Server and monitor Direct memory, but only netty use direct memory 。

I think not.

I think this is only for netty after reviewing the implementation of PlatformDependent.usedDirectMemory()

@zuston Ok，I change the class name later , thanks

zuston · 2023-12-12T09:36:03Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java

  private static final String USED_BUFFER_SIZE = "used_buffer_size";
  private static final String READ_USED_BUFFER_SIZE = "read_used_buffer_size";
-  private static final String USED_DIRECT_MEMORY_SIZE = "used_direct_memory_size";
+  private static final String NETTY_USED_DIRECT_MEMORY_SIZE = "netty_used_direct_memory_size";


This metrics also should be moved into netty metric class

And this should be initialized in the netty metrics class rather than in shuffleServer class

Grpc use Netty, too.

Oh I missed this. Let reserve this. Sorry @lifeSo

@zuston so, I revert the code to early version.

zuston

Left the minor comments.

zuston · 2023-12-13T09:46:54Z

server/src/main/java/org/apache/uniffle/server/DirectMemoryUsageReporter.java

+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public class DirectMemoryUsageReporter {


Anyway, I also think the nettyDirectMemoryTracker is better. And it needn't move file position

server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java

zuston · 2023-12-13T09:50:38Z

server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java

+          .withDescription("rss direct memory usage report initial delay (ms)");
+
+  public static final ConfigOption<Long> SERVER_DIRECT_MEMORY_USAGE_REPORT_INTERVAL =
+      ConfigOptions.key("rss.server.direct.memory.usage.report.interval")


How about rss.server.netty.direct-memory-tracker.metrics.update-interval-ms

Ok, I corrented it.

Sorry for my comment incorrectness. Follows @jerqi suggestion here: #1370 (comment)

Let's use the camel style config key.

How about rss.server.netty.directMemoryTracker.memoryUsage.initialFetchDelayMs and rss.server.netty.directMemoryTracker.memoryUsage.updateMetricsIntervalMs ? cc @jerqi

Ok， I corrected it .

zuston

LGTM. Thanks for your effort. @lifeSo

* master: (23 commits) format use synchronized format modify to singleton format format format format format change Coordinator to singleton like ShuffleServerClientFactory.java [apache#1189] feat(server): Add netty used direct memory size metric (apache#1363) format format format rename file rename [apache#960] fix(dashboard): simplify dependency and correct the startup script (apache#1347) [apache#1348] improvement(metrics): Unify tags generation for shuffle-server metrics reporter (apache#1349) [MINOR] chore: fix kubernetes ci pipeline (apache#1368) [MINOR] fix(spark): Fix NPE for ShuffleWriteClientImpl.unregisterShuffle (apache#1367) ...

…re_1119_new_2023_12_23 * feature_1119_BUFFER_LIMIT_OF_HUGE_PARTITION: add file for commit [apache#1189] feat(server): Add netty used direct memory size metric (apache#1363) format format format rename file rename [apache#960] fix(dashboard): simplify dependency and correct the startup script (apache#1347) [apache#1348] improvement(metrics): Unify tags generation for shuffle-server metrics reporter (apache#1349) [MINOR] chore: fix kubernetes ci pipeline (apache#1368) [MINOR] fix(spark): Fix NPE for ShuffleWriteClientImpl.unregisterShuffle (apache#1367) format code format code 添加堆外内存监控的代码

### What changes were proposed in this pull request? Fix PR [#1363](#1363). NettyDirectMemoryTracker will not be started currently. ### Why are the changes needed? It's a followup PR for [#1363](#1363). ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing UTs.

…pache#1363) ### What changes were proposed in this pull request? ShuffleServer add Direct Memory Metric for monitor ### Why are the changes needed? Fix: # (apache#1189) ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested?

lifeSo added 2 commits December 11, 2023 21:51

添加堆外内存监控的代码

7ab778f

format code

df92f55

zuston reviewed Dec 12, 2023

View reviewed changes

format code

1759f9a

zuston reviewed Dec 12, 2023

View reviewed changes

lifeSo force-pushed the master branch from f9a78cd to 1759f9a Compare December 12, 2023 11:51

zuston reviewed Dec 13, 2023

View reviewed changes

lifeSo added 5 commits December 13, 2023 20:25

file rename

01872d0

rename

a0f5ea6

format

84df9a4

format

0a9a90d

format

7ca74cf

zuston approved these changes Dec 16, 2023

View reviewed changes

zuston changed the title ~~[#1189] improvement(server): ShuffleServer add Direct Memory Metric for monitor~~ [#1189] feat(server): Add netty used direct memory size metric Dec 16, 2023

zuston merged commit 9bd5fd8 into apache:master Dec 16, 2023

zuston mentioned this pull request Dec 16, 2023

[Improvement] ShuffleServer add Direct Memory Metric for monitor #1189

Closed

3 tasks

lifeSo mentioned this pull request Jan 5, 2024

[#333] feat: expose metrics of TOP10 app bytes in one shuffle server #1400

Merged

rickyma mentioned this pull request Jan 10, 2024

[#1189][FOLLOWUP] fix(server): Start NettyDirectMemoryTracker. #1432

Merged


		import org.apache.uniffle.common.util.ThreadUtils;

		public class DirectMemoryUsageReporter {

[#1189] feat(server): Add netty used direct memory size metric #1363

[#1189] feat(server): Add netty used direct memory size metric #1363

Uh oh!

Conversation

lifeSo commented Dec 11, 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!

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!

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

Uh oh!

Codecov Report

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!

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!

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!

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!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Dec 12, 2023 •

edited

Loading