[#3152] feat(core): using time sliding window to record timer metrics #3341

FANNG1 · 2024-05-11T10:13:45Z

What changes were proposed in this pull request?

using time sliding window to record timer metrics

Why are the changes needed?

Fix: #3152

Does this PR introduce any user-facing change?

no

How was this patch tested?

test in local env to check whether Pxx comes to 0 after a while

FANNG1 · 2024-05-13T08:04:35Z

Time sliding window based metrics is more suitable for our scene, no need to make it configurable.
60s time window seems enough , @jerryshao do you think is it neccessary to make it configurationable ?

jerryshao · 2024-05-15T02:07:19Z

I think it would be better to make the time window configurable.

FANNG1 · 2024-05-15T03:38:17Z

I think it would be better to make the time window configurable.

ok

FANNG1 · 2024-05-16T09:58:27Z

core/src/main/java/com/datastrato/gravitino/metrics/source/MetricsSource.java

+  }
+
+  protected int getTimeSlidingWindowSeconds() {
+    MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();


It's a little hacky here, couldn't find a more general way to inject configurations.

Why can't you pass the config value from metrics system to metrics source?

currently, creating metrics source is depend of MetricsSystem, so couldn't pass config value to metrics source.
One possible solution is creating MetricsSource using MetricsSystem or Config.

passing Config to MetricsSource, please review again

FANNG1 · 2024-05-16T10:49:48Z

@jerryshao , please help to review when you are free

jerryshao · 2024-05-17T09:26:41Z

@TEOTEO520 can you please help to check if the fix here satisfy your needs?

jerryshao · 2024-05-17T09:31:42Z

core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java

@@ -139,7 +139,8 @@ public void initialize(Config config) {
    LOG.info("Initializing Gravitino Environment...");

    this.config = config;
-    this.metricsSystem = new MetricsSystem();
+    int slidingWindowSeconds = config.get(Configs.METRICS_TIME_SLIDING_WINDOW_SECONDS).intValue();
+    this.metricsSystem = new MetricsSystem(slidingWindowSeconds);


Why don't you pass in a Config object as a class parameter and get the specific configuration in MetricsSystem? It is a little strange to get the configuration here in GravitinoEnv.

core/src/main/java/com/datastrato/gravitino/Configs.java

jerqi · 2024-05-17T09:34:48Z

common/src/main/java/com/datastrato/gravitino/config/ConfigConstants.java

@@ -32,4 +32,6 @@ private ConfigConstants() {}
  public static final String VERSION_0_4_0 = "0.4.0";
  /** The version number for the 0.5.0 release. */
  public static final String VERSION_0_5_0 = "0.5.0";
+  /** The version number for the 0.6.0 release. */
+  public static final String VERSION_0_6_0 = "0.6.0";


Maybe we would better use 0.5.1.

FANNG1 · 2024-05-20T06:36:02Z

@jerryshao , @jerqi , please help to review again when you are free

jerryshao · 2024-05-21T06:31:35Z

core/src/main/java/com/datastrato/gravitino/metrics/MetricsSystem.java


-  public MetricsSystem() {
-    this("");
+  public MetricsSystem(Config config) {


Now, you directly pass the Config to MetricsSource, this is not needed, why do you still need to change this?

We couldn't get ServerConfig in IcebergRESTService, so saving Config to MetricsSystem to get it in IcebergRESTService

It's a bit hacky, can you please figure out a better way. Also why don't you pass in as a Iceberg rest service config, so that it can be used here.

You mean retrieve the metrics config from ServerConfig and pass in as Iceberg REST service config? this also a little hacky

I'm you can pass the aux configuration "aux.xxx" to iceberg rest service, so the rest service can get and use it, do you think it is hacky?

the properties passed to Iceberg REST service are properties with gravitino.auxService.iceberg-rest. prefix, I think it's hacky to pass gravitino.metrics.timeSlidingWindowSecs to the properties. Maybe we could pass extra Config object to aux service to pass generic configurations.

jerryshao · 2024-05-21T06:33:36Z

core/src/main/java/com/datastrato/gravitino/metrics/source/MetricsSource.java

+                    getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
+  }
+
+  protected int getTimeSlidingWindowSeconds() {


Why do you need this, can you directly use the class variable?

HttpServerMetricsSource should use this method, you suggest to make the variable protected and use it directly?

it's fine leave as it is.

jerryshao · 2024-05-21T06:40:31Z

server/src/main/java/com/datastrato/gravitino/server/GravitinoServer.java

+            MetricsSource.GRAVITINO_SERVER_METRIC_NAME,
+            this,
+            server,
+            metricsSystem.getServerConfig());


GravitinoServer has a variable serverConfig can be used, don't need to do this.

jerryshao · 2024-05-21T06:42:13Z

docs/gravitino-server-config.md

+
+| Property name                             | Description                                          | Default value | Required | Since Version |
+|-------------------------------------------|------------------------------------------------------|---------------|----------|---------------|
+| `gravitino.metrics.timeSlidingWindowSecs` | The seconds of Gravitino metrics time sliding window | 60            | No       | 0.5.1         |


Did you try that this can be worked in iceberg rest catalog service, don't need to change the config prefix?

I'm not verified in Iceberg REST service, but from the code, it should be worked because it's a generic configuration not bind to server or Iceberg server.

jerryshao · 2024-05-27T07:22:42Z

...erg/src/main/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/IcebergRESTService.java

+            MetricsSource.ICEBERG_REST_SERVER_METRIC_NAME,
+            resourceConfig,
+            server,
+            metricsSystem.getServerConfig());


One thing I was thinking is that we don't have to change the interface a lot, using GravitinoEnv.config() can get the global configuration, do you think it is a better solution?

if using GravitinoEnv.config(), seems no need to pass config to MetricsSource constructor too, WDYT?

Yes, we can roll back everything.

…#3341) ### What changes were proposed in this pull request? using time sliding window to record timer metrics ### Why are the changes needed? Fix: #3152 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? test in local env to check whether Pxx comes to 0 after a while

…etrics (apache#3341) ### What changes were proposed in this pull request? using time sliding window to record timer metrics ### Why are the changes needed? Fix: apache#3152 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? test in local env to check whether Pxx comes to 0 after a while

FANNG1 self-assigned this May 11, 2024

FANNG1 marked this pull request as draft May 11, 2024 10:14

FANNG1 mentioned this pull request May 11, 2024

[Improvement] Suggestion for Handling Stale Metrics in Gravitino #3152

Closed

FANNG1 marked this pull request as ready for review May 14, 2024 00:30

FANNG1 requested a review from jerryshao May 14, 2024 00:30

jerryshao added the branch-0.5 label May 15, 2024

FANNG1 force-pushed the metrics branch from 30907d9 to 0517a34 Compare May 16, 2024 09:56

FANNG1 commented May 16, 2024

View reviewed changes

jerryshao reviewed May 17, 2024

View reviewed changes

jerqi reviewed May 17, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/Configs.java Show resolved Hide resolved

jerqi reviewed May 17, 2024

View reviewed changes

FANNG1 force-pushed the metrics branch from c06a1ba to b26a42b Compare May 20, 2024 01:37

FANNG1 changed the title ~~[#3152] fix(core): using time sliding window to record timer metrics~~ [#3152] feat(core): using time sliding window to record timer metrics May 21, 2024

jerryshao reviewed May 21, 2024

View reviewed changes

jerryshao reviewed May 27, 2024

View reviewed changes

FANNG1 added 6 commits May 27, 2024 19:49

fix comment

32efa0a

xx

31f0809

fix comment

b536da6

fix erro

270b46e

xx

674323d

fix comment

a10cbe0

use gravitinoenv.config

e348994

FANNG1 force-pushed the metrics branch from 69a093a to e348994 Compare May 27, 2024 11:50

FANNG1 added 2 commits May 27, 2024 19:55

xx

0351113

xx

3348aec

jerryshao approved these changes May 27, 2024

View reviewed changes

jerryshao merged commit 048f114 into apache:main May 27, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3152] feat(core): using time sliding window to record timer metrics #3341

[#3152] feat(core): using time sliding window to record timer metrics #3341

FANNG1 commented May 11, 2024

FANNG1 commented May 13, 2024

jerryshao commented May 15, 2024

FANNG1 commented May 15, 2024

FANNG1 May 16, 2024

jerryshao May 20, 2024

FANNG1 May 20, 2024

FANNG1 May 21, 2024

FANNG1 commented May 16, 2024

jerryshao commented May 17, 2024

jerryshao May 17, 2024 •

edited

Loading

jerqi May 17, 2024

FANNG1 May 20, 2024

FANNG1 commented May 20, 2024

jerryshao May 21, 2024

FANNG1 May 21, 2024

jerryshao May 21, 2024

FANNG1 May 21, 2024

jerryshao May 22, 2024

FANNG1 May 22, 2024

jerryshao May 21, 2024

FANNG1 May 21, 2024

jerryshao May 21, 2024

jerryshao May 21, 2024

FANNG1 May 21, 2024

jerryshao May 21, 2024

FANNG1 May 21, 2024

jerryshao May 27, 2024

FANNG1 May 27, 2024

jerryshao May 27, 2024

[#3152] feat(core): using time sliding window to record timer metrics #3341

[#3152] feat(core): using time sliding window to record timer metrics #3341

Conversation

FANNG1 commented May 11, 2024

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?

FANNG1 commented May 13, 2024

jerryshao commented May 15, 2024

FANNG1 commented May 15, 2024

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

FANNG1 commented May 16, 2024

jerryshao commented May 17, 2024

jerryshao May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented May 20, 2024

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

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

jerryshao May 17, 2024 •

edited

Loading