Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
May 27, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
32efa0a
fix comment
FANNG1 May 11, 2024
31f0809
xx
FANNG1 May 16, 2024
b536da6
fix comment
FANNG1 May 20, 2024
270b46e
fix erro
FANNG1 May 20, 2024
674323d
xx
FANNG1 May 21, 2024
a10cbe0
fix comment
FANNG1 May 21, 2024
e348994
use gravitinoenv.config
FANNG1 May 27, 2024
0351113
xx
FANNG1 May 27, 2024
3348aec
xx
FANNG1 May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions common/src/main/java/com/datastrato/gravitino/config/ConfigConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.5.1 release. */
public static final String VERSION_0_5_1 = "0.5.1";
}
8 changes: 8 additions & 0 deletions core/src/main/java/com/datastrato/gravitino/Configs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,4 +276,12 @@ private Configs() {}
.version(ConfigConstants.VERSION_0_5_0)
.longConf()
.createWithDefault(60 * 60 * 1000L);

public static final int DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS = 60;
public static final ConfigEntry<Integer> METRICS_TIME_SLIDING_WINDOW_SECONDS =
new ConfigBuilder("gravitino.metrics.timeSlidingWindowSecs")
.doc("The seconds of Gravitino metrics time sliding window")
.version(ConfigConstants.VERSION_0_5_1)
.intConf()
.createWithDefault(DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS);
}
32 changes: 30 additions & 2 deletions core/src/main/java/com/datastrato/gravitino/metrics/source/MetricsSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@
import com.codahale.metrics.Gauge;
import com.codahale.metrics.Histogram;
import com.codahale.metrics.MetricRegistry;
import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
import com.codahale.metrics.Timer;
import com.datastrato.gravitino.Config;
import com.datastrato.gravitino.Configs;
import com.datastrato.gravitino.GravitinoEnv;
import java.util.concurrent.TimeUnit;

/**
* MetricsSource provides utilities to collect specified kind metrics, all metrics must create with
Expand All @@ -24,10 +29,19 @@ public abstract class MetricsSource {
public static final String JVM_METRIC_NAME = "jvm";
private final MetricRegistry metricRegistry;
private final String metricsSourceName;
private final int timeSlidingWindowSeconds;

protected MetricsSource(String name) {
this.metricsSourceName = name;
metricRegistry = new MetricRegistry();
Config config = GravitinoEnv.getInstance().config();
if (config != null) {
this.timeSlidingWindowSeconds =
config.get(Configs.METRICS_TIME_SLIDING_WINDOW_SECONDS).intValue();
} else {
// Couldn't get config when testing
this.timeSlidingWindowSeconds = Configs.DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS;
}
}

/**
Expand Down Expand Up @@ -75,7 +89,12 @@ public Counter getCounter(String name) {
* @return a new or pre-existing Histogram
*/
public Histogram getHistogram(String name) {
return this.metricRegistry.histogram(name);
return this.metricRegistry.histogram(
name,
() ->
new Histogram(
new SlidingTimeWindowArrayReservoir(
getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
}

/**
Expand All @@ -85,6 +104,15 @@ public Histogram getHistogram(String name) {
* @return a new or pre-existing Timer
*/
public Timer getTimer(String name) {
return this.metricRegistry.timer(name);
return this.metricRegistry.timer(
name,
() ->
new Timer(
new SlidingTimeWindowArrayReservoir(
getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
}

protected int getTimeSlidingWindowSeconds() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's fine leave as it is.

return timeSlidingWindowSeconds;
}
}
10 changes: 5 additions & 5 deletions core/src/test/java/com/datastrato/gravitino/metrics/TestMetricsSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,7 @@
import org.junit.jupiter.api.Test;

public class TestMetricsSystem {
MetricsSystem metricsSystem = new MetricsSystem();

private long getCounterValue(String metricsSourceName, String name) {
return metricsSystem.getMetricRegistry().counter(metricsSourceName + "." + name).getCount();
}
private MetricsSystem metricsSystem = new MetricsSystem();

@Test
void testRegisterMetricsSource() {
Expand Down Expand Up @@ -53,4 +49,8 @@ void testRegisterMetricsSource() {
.getCounters()
.containsKey(metricsSource.getMetricsSourceName() + "a.b"));
}

private long getCounterValue(String metricsSourceName, String name) {
return metricsSystem.getMetricRegistry().counter(metricsSourceName + "." + name).getCount();
}
}
2 changes: 1 addition & 1 deletion core/src/test/java/com/datastrato/gravitino/metrics/source/TestMetricsSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public TestMetricsSource() {

@BeforeAll
void init() {
metricsSystem = new MetricsSystem();
this.metricsSystem = new MetricsSystem();
metricsSystem.register(this);
}

Expand Down
6 changes: 6 additions & 0 deletions docs/gravitino-server-config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ For more details, please refer to the definition of the plugin.

Refer to [security](security.md) for HTTPS and authentication configurations.

### Metrics configuration

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Gravitino catalog properties configuration

There are three types of catalog properties:
Expand Down
12 changes: 11 additions & 1 deletion server-common/src/main/java/com/datastrato/gravitino/server/web/HttpServerMetricsSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,25 @@

package com.datastrato.gravitino.server.web;

import com.codahale.metrics.Clock;
import com.codahale.metrics.SlidingTimeWindowArrayReservoir;
import com.codahale.metrics.jersey2.InstrumentedResourceMethodApplicationListener;
import com.datastrato.gravitino.metrics.MetricNames;
import com.datastrato.gravitino.metrics.source.MetricsSource;
import java.util.concurrent.TimeUnit;
import org.glassfish.jersey.server.ResourceConfig;

public class HttpServerMetricsSource extends MetricsSource {
public HttpServerMetricsSource(String name, ResourceConfig resourceConfig, JettyServer server) {
super(name);
resourceConfig.register(new InstrumentedResourceMethodApplicationListener(getMetricRegistry()));
resourceConfig.register(
new InstrumentedResourceMethodApplicationListener(
getMetricRegistry(),
Clock.defaultClock(),
false,
() ->
new SlidingTimeWindowArrayReservoir(
getTimeSlidingWindowSeconds(), TimeUnit.SECONDS)));
registerGauge(
MetricNames.SERVER_IDLE_THREAD_NUM, () -> server.getThreadPool().getIdleThreads());
}
Expand Down
Loading