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.
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
base: main
Are you sure you want to change the base?
Conversation
Time sliding window based metrics is more suitable for our scene, no need to make it configurable. |
I think it would be better to make the time window configurable. |
ok |
} | ||
|
||
protected int getTimeSlidingWindowSeconds() { | ||
MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little hacky here, couldn't find a more general way to inject configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you pass the config value from metrics system to metrics source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passing Config
to MetricsSource
, please review again
@jerryshao , please help to review when you are free |
@TEOTEO520 can you please help to check if the fix here satisfy your needs? |
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need Secs
or Sec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to using Secs
to keep consistent with current gravitino.lock.cleanIntervalInSecs
, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we would better use 0.5.1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@jerryshao , @jerqi , please help to review again when you are free |
|
||
public MetricsSystem() { | ||
this(""); | ||
public MetricsSystem(Config config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, you directly pass the Config
to MetricsSource, this is not needed, why do you still need to change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We couldn't get ServerConfig
in IcebergRESTService
, so saving Config
to MetricsSystem
to get it in IcebergRESTService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean retrieve the metrics config from ServerConfig
and pass in as Iceberg REST service
config? this also a little hacky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
getTimeSlidingWindowSeconds(), TimeUnit.SECONDS))); | ||
} | ||
|
||
protected int getTimeSlidingWindowSeconds() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
MetricsSource.GRAVITINO_SERVER_METRIC_NAME, | ||
this, | ||
server, | ||
metricsSystem.getServerConfig()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GravitinoServer
has a variable serverConfig
can be used, don't need to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
||
| Property name | Description | Default value | Required | Since Version | | ||
|-------------------------------------------|------------------------------------------------------|---------------|----------|---------------| | ||
| `gravitino.metrics.timeSlidingWindowSecs` | The seconds of Gravitino metrics time sliding window | 60 | No | 0.5.1 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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