[#1973] feat(server,coordinator): Refactor metrics system to reduce periodic reporting load #1991

kuszz · 2024-07-31T07:20:45Z

What changes were proposed in this pull request?

Add another method to add gauge metric, we can use lambda to describe a gauge metric.

Why are the changes needed?

Fix: #1973

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UTs.

github-actions · 2024-07-31T10:13:22Z

Test Results

2 792 files ±0 2 792 suites ±0 5h 51m 4s ⏱️ +47s
988 tests ±0 987 ✅ ±0 1 💤 ±0 0 ❌ ±0
12 403 runs ±0 12 388 ✅ ±0 15 💤 ±0 0 ❌ ±0

Results for commit 29aec85. ± Comparison against base commit 3673b55.

♻️ This comment has been updated with latest results.

maobaolong

left comment

maobaolong · 2024-08-01T18:24:42Z

client-spark/spark2-shaded/pom.xml

@@ -216,6 +216,10 @@
                  <pattern>picocli</pattern>
                  <shadedPattern>${rss.shade.packageName}.picocli</shadedPattern>
                </relocation>
+                <relocation>
+                  <pattern>com.codahale.metrics</pattern>
+                  <shadedPattern>${rss.shade.packageName}.com.codahale.metrics</shadedPattern>


check the dependencies of this is shaded too

maobaolong

Would you like to add /api/xxx/metrics /api/xxx/prometheus/metrics result to the Description of this PR.

maobaolong · 2024-08-02T00:45:47Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

  public Gauge addGauge(String name, String help, String[] labels) {
    return Gauge.build().name(name).labelNames(labels).help(help).register(collectorRegistry);
  }

+  public synchronized <T> void addGauge(String name, com.codahale.metrics.Gauge<T> metric) {


registerGaugeIfAbsent

maobaolong · 2024-08-02T00:46:49Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

+    }
+  }
+
+  public synchronized <T> void addGauge(


registerCachedGaugeIfAbsent

pom.xml

maobaolong · 2024-08-02T00:54:28Z

pom.xml

@@ -105,6 +105,7 @@
    <skipITs>${skipTests}</skipITs>
    <skipBuildImage>true</skipBuildImage>
    <snakeyaml.version>2.2</snakeyaml.version>
+    <prometheus.version>0.8.0</prometheus.version>


Why not use ${prometheus.simpleclient.version}?

maobaolong

@kuszz It looks a great feature, with this, we can easy to add Gauge to report size of collection, left a last question inline, besides this, there are no any other suggestion from me.

maobaolong

LGTM. @kuszz Thanks for your contribution!

advancedxy

Two high-level questions:

how will this new metrics system integrate with existing systems? I think a lot of people are already using prometheus to collect metrics data. It would be great if we can make a drop-in replacement for that. cc @xianjingfeng maybe you have some input about this part
is there a plan to replace other existing metrics with the new one?

advancedxy · 2024-08-02T12:25:04Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

@@ -44,6 +51,8 @@ public MetricsManager(CollectorRegistry collectorRegistry, Map<String, String> d
    } else {
      this.collectorRegistry = collectorRegistry;
    }
+    metricRegistry = new MetricRegistry();


Nit: -> this.metricRegistry = xxx.

It's clear to explicitly use this.xx when updating the instance field.

advancedxy · 2024-08-02T12:52:45Z

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

@@ -513,4 +503,8 @@ private static void setUpMetrics(ShuffleServerConf serverConf) {
            .labelNames("app_id")
            .register(metricsManager.getCollectorRegistry());
  }
+
+  public static MetricsManager getMetricsManager() {


I don't think we should expose metrics manager directly.
Instead, you could add methods in ShuffleServerMetrics to register new metrics, such as:

public static void registerEventQueueSize(com.codahale.metrics.Gauge<Integer> eventQueueSize) { // do all kinds of validation if needed. metricsManager.registerGaugeIfAbsent(EVENT_QUEUE_SIZE, eventQueueSize); }

Or we can expose the metrics names and the registerMetrics method combined.

thanks, I removed this func

advancedxy · 2024-08-02T12:53:40Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

+  private static final String LABEL_SEPARATOR = ":";
+  private static final String NAME_SEPARATOR = "_";


Seems these two are never used?

advancedxy · 2024-08-02T12:54:01Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

@@ -112,4 +150,8 @@ public Summary.Child addLabeledSummary(String name) {
    }
    return builder.register(collectorRegistry).labels(defaultLabelValues);
  }
+
+  public com.codahale.metrics.Gauge getGauge(String name) {


Never used?

yes, this func display how to obtain the value of a gauge registered in new way

advancedxy · 2024-08-02T12:55:11Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

@@ -66,14 +75,43 @@ public Counter.Child addLabeledCounter(String name) {
    return c.labels(this.defaultLabelValues);
  }

+  @Deprecated


I would like to add a javadoc to indicate which version this method will be removed and what alternative should be used.

I removed the annotation, the new way can not add labels, so it cannot completely replace the old method

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

xianjingfeng · 2024-08-05T06:15:14Z

how will this new metrics system integrate with existing systems? I think a lot of people are already using prometheus to collect metrics data. It would be great if we can make a drop-in replacement for that. cc @xianjingfeng maybe you have some input about this part

This PR does not affect the original collection logic. https://www.robustperception.io/exposing-dropwizard-metrics-to-prometheus/

advancedxy · 2024-08-05T08:51:38Z

how will this new metrics system integrate with existing systems? I think a lot of people are already using prometheus to collect metrics data. It would be great if we can make a drop-in replacement for that. cc @xianjingfeng maybe you have some input about this part

This PR does not affect the original collection logic. https://www.robustperception.io/exposing-dropwizard-metrics-to-prometheus/

This is good to know. But it seems it doesn't support labels? Or it could be supported in another way?

xianjingfeng · 2024-08-05T10:24:29Z

This is good to know. But it seems it doesn't support labels? Or it could be supported in another way?

It seems that there is no way.
https://groups.google.com/g/prometheus-users/c/6SwcjeAYkiE/m/VvOSd7nSAgAJ

advancedxy

Thanks @kuszz, I think this is in the right direction. Just some coding style issues to be addressed.

It would be great if you can add some UTs for the new gauge metric as well.

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

advancedxy · 2024-08-09T02:57:00Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

@@ -47,6 +50,7 @@ public MetricsManager(CollectorRegistry collectorRegistry, Map<String, String> d
    this.defaultLabelNames = defaultLabels.keySet().toArray(new String[0]);
    this.defaultLabelValues =
        Arrays.stream(defaultLabelNames).map(defaultLabels::get).toArray(String[]::new);
+    this.gaugeMap = new ConcurrentHashMap<>();


Use this.gaugeMap = JavaUtils.newConcurrentMap();

This is a performance issue in JDK8 for concurrent hash map, so we have to wrap it with a special one.

thanks, I have modified it

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

advancedxy · 2024-08-09T03:01:45Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

@@ -112,4 +142,10 @@ public Summary.Child addLabeledSummary(String name) {
    }
    return builder.register(collectorRegistry).labels(defaultLabelValues);
  }
+
+  public void unRegister() {


hmm. This signature doesn't seem right.

It should be something like:

public void unregisterSupplierGauge(String name)

?

common/src/main/java/org/apache/uniffle/common/metrics/SupplierGauge.java

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

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

advancedxy

Generally lgtm, except one minor comment.

@maobaolong please take a look as well in case you have more comments.

codecov-commenter · 2024-08-09T06:47:38Z

Codecov Report

Attention: Patch coverage is 38.88889% with 22 lines in your changes missing coverage. Please review.

Project coverage is 52.53%. Comparing base (5ddcc28) to head (e42cd9f).
Report is 37 commits behind head on master.

Files	Patch %	Lines
...g/apache/uniffle/common/metrics/SupplierGauge.java	0.00%	15 Missing ⚠️
.../apache/uniffle/common/metrics/MetricsManager.java	12.50%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1991      +/-   ##
============================================
- Coverage     52.77%   52.53%   -0.24%     
- Complexity     2498     2925     +427     
============================================
  Files           398      447      +49     
  Lines         18135    23546    +5411     
  Branches       1660     2196     +536     
============================================
+ Hits           9570    12370    +2800     
- Misses         7981    10383    +2402     
- Partials        584      793     +209

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

advancedxy · 2024-08-09T08:16:15Z

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java

+  public void unregisterAllSupplierGauge() {
+    for (SupplierGauge gauge : supplierGaugeMap.values()) {
+      collectorRegistry.unregister(gauge);
+    }


hmm, I think we should clear the supplierGaugeMap as well?

maobaolong

@kuszz @advancedxy It LGTM, I've give some suggestion off the github, and I checked all of my suggestion are addressed.

advancedxy · 2024-08-12T08:06:32Z

LGTM on my side too. @kuszz would you mind to rebase the latest master and make the CI happy.

kuszz · 2024-08-13T02:26:27Z

LGTM on my side too. @kuszz would you mind to rebase the latest master and make the CI happy.

@advancedxy rust ci reports 404 error

advancedxy · 2024-08-13T02:52:06Z

hmmm, it seems all the CI are failing? @zuston would you mind to take a look at the rust CI failure?

maobaolong · 2024-08-13T12:45:51Z

@advancedxy If it is difficult to fix the rust related CI checks, we can ignore it by this for now？ #2041

advancedxy · 2024-08-13T13:48:59Z

I'm fine to ignore that first, cc @zuston

advancedxy · 2024-08-14T03:30:13Z

Thanks to @maobaolong, the rust ci checked is disabled temporary. @kuszz please rebase the master and retry CI, sorry for the inconvenience.

kuszz · 2024-08-14T07:37:20Z

Thanks to @maobaolong, the rust ci checked is disabled temporary. @kuszz please rebase the master and retry CI, sorry for the inconvenience.

@advancedxy done

advancedxy · 2024-08-14T12:39:35Z

Merged, thanks @kuszz.

…ric type and add requireBufferCount metrics (#2113) ### What changes were proposed in this pull request? Refactor SupplierGauge to support generic type and add requireBufferCount metrics ### Why are the changes needed? Fix: #1991 ### Does this PR introduce _any_ user-facing change? Add new metrics named `require_buffer_count` ### How was this patch tested? Tested through dashboard server metrics popup page. <img width="945" alt="image" src="https://github.com/user-attachments/assets/2fbbb1d1-7f1f-41ac-ab41-adba55790005"> --------- Co-authored-by: xianjingfeng <[email protected]>

### What changes were proposed in this pull request? Add another method to add gauge metric, we can use lambda to describe a gauge metric. ### Why are the changes needed? Fix: apache#1973 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? UTs. (cherry picked from commit 8dfa2a7)

…t generic type and add requireBufferCount metrics (apache#2113) ### What changes were proposed in this pull request? Refactor SupplierGauge to support generic type and add requireBufferCount metrics ### Why are the changes needed? Fix: apache#1991 ### Does this PR introduce _any_ user-facing change? Add new metrics named `require_buffer_count` ### How was this patch tested? Tested through dashboard server metrics popup page. <img width="945" alt="image" src="https://github.com/user-attachments/assets/2fbbb1d1-7f1f-41ac-ab41-adba55790005"> --------- Co-authored-by: xianjingfeng <[email protected]> (cherry picked from commit d1aa51b)

rickyma changed the title ~~[#1973] feat(server,coordinator): Refactor metrics system to reduce periodically report metrics heavy behavior~~ [#1973] feat(server,coordinator): Refactor metrics system to reduce periodic reporting load Jul 31, 2024

kuszz closed this Aug 1, 2024

kuszz reopened this Aug 1, 2024

maobaolong reviewed Aug 1, 2024

View reviewed changes

maobaolong reviewed Aug 2, 2024

View reviewed changes

maobaolong approved these changes Aug 2, 2024

View reviewed changes

advancedxy reviewed Aug 2, 2024

View reviewed changes

kuszz force-pushed the fixMetricSystem branch 2 times, most recently from 4a22c8e to 5c018b3 Compare August 7, 2024 10:46

kuszz closed this Aug 8, 2024

kuszz reopened this Aug 8, 2024

kuszz closed this Aug 8, 2024

kuszz reopened this Aug 8, 2024

kuszz closed this Aug 8, 2024

kuszz reopened this Aug 8, 2024

advancedxy reviewed Aug 9, 2024

View reviewed changes

common/src/main/java/org/apache/uniffle/common/metrics/MetricsManager.java Outdated Show resolved Hide resolved

advancedxy reviewed Aug 9, 2024

View reviewed changes

kuszz force-pushed the fixMetricSystem branch from e42cd9f to a893f50 Compare August 9, 2024 07:29

advancedxy reviewed Aug 9, 2024

View reviewed changes

kuszz force-pushed the fixMetricSystem branch from c5bc70f to e03a243 Compare August 9, 2024 08:44

maobaolong approved these changes Aug 12, 2024

View reviewed changes

kuszz closed this Aug 12, 2024

kuszz reopened this Aug 12, 2024

kuszz closed this Aug 12, 2024

kuszz reopened this Aug 12, 2024

kuszz closed this Aug 13, 2024

kuszz reopened this Aug 13, 2024

Refactor metrics system to reduce periodic reporting load

29aec85

kuszz force-pushed the fixMetricSystem branch from bbff134 to 29aec85 Compare August 14, 2024 07:05

advancedxy approved these changes Aug 14, 2024

View reviewed changes

advancedxy merged commit 8dfa2a7 into apache:master Aug 14, 2024
41 checks passed

maobaolong mentioned this pull request Sep 12, 2024

[#1991][FOLLOWUP] Refactor SupplierGauge to support generic type and add requireBufferCount metrics #2113

Merged

		private static final String LABEL_SEPARATOR = ":";
		private static final String NAME_SEPARATOR = "_";

[#1973] feat(server,coordinator): Refactor metrics system to reduce periodic reporting load #1991

[#1973] feat(server,coordinator): Refactor metrics system to reduce periodic reporting load #1991

Conversation

kuszz commented Jul 31, 2024 • edited Loading

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?

github-actions bot commented Jul 31, 2024 • edited Loading

Test Results

maobaolong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong left a comment

Choose a reason for hiding this comment

maobaolong left a comment

Choose a reason for hiding this comment

advancedxy 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

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

Choose a reason for hiding this comment

xianjingfeng commented Aug 5, 2024

advancedxy commented Aug 5, 2024

xianjingfeng commented Aug 5, 2024

advancedxy 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

advancedxy left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 9, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maobaolong left a comment

Choose a reason for hiding this comment

advancedxy commented Aug 12, 2024

kuszz commented Aug 13, 2024

advancedxy commented Aug 13, 2024

maobaolong commented Aug 13, 2024

advancedxy commented Aug 13, 2024

advancedxy commented Aug 14, 2024

kuszz commented Aug 14, 2024

advancedxy commented Aug 14, 2024

kuszz commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading