[#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.

jerqi · 2024-05-17T09:33:46Z

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

@@ -267,4 +267,11 @@ public interface Configs {
          .version(ConfigConstants.VERSION_0_5_0)
          .longConf()
          .createWithDefault(60 * 60 * 1000L);
+
+  ConfigEntry<Long> METRICS_TIME_SLIDING_WINDOW_SECONDS =
+      new ConfigBuilder("gravitino.metrics.timeSlidingWindowSecs")


Do we need Secs or Sec?

I prefer to using Secs to keep consistent with current gravitino.lock.cleanIntervalInSecs, WDYT?

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.

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

Open

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

FANNG1 added 3 commits May 20, 2024 09:37

fix comment

0a26786

xx

aa537e4

fix comment

b26a42b

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

fix erro

b87aac3

xx

6233946

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

fix comment

69a093a

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

jerqi May 17, 2024

FANNG1 May 20, 2024

jerqi May 20, 2024

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

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

Are you sure you want to change the base?

[#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

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

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

jerryshao May 17, 2024 •

edited