Skip to content

[MINOR] Add configuration to enable/disable common prefix for all met… #4741

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

Closed
low-on-mana wants to merge 1 commit into from
Closed

[MINOR] Add configuration to enable/disable common prefix for all met… #4741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1561,6 +1561,10 @@ public boolean isExecutorMetricsEnabled() {
getStringOrDefault(HoodieMetricsConfig.EXECUTOR_METRICS_ENABLE, "false"));
}

public boolean isMetricsCommonPrefixEnabled() {
Copy link
Contributor

@rmahindra123 rmahindra123 Feb 4, 2022

Choose a reason for hiding this comment

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

Why do you need another config. Check this PR: https://github.com/apache/hudi/pull/4274/files. If you want remove the infer function for METRICS_REPORTER_PREFIX, so it does not use the table name as default and the user can set a prefix using METRICS_REPORTER_PREFIX. Does it make sense?

Copy link
Contributor Author

@low-on-mana low-on-mana Feb 4, 2022

Choose a reason for hiding this comment

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

Ohh, I didnt notice while rebasing changes. I think I can use #4274. I will close this PR.

Copy link
Contributor

@rmahindra123 rmahindra123 Feb 4, 2022

Choose a reason for hiding this comment

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

No problem. Thanks @t0il3ts0ap

return getBooleanOrDefault(HoodieMetricsConfig.ENABLE_COMMON_PREFIX);
}

public MetricsReporterType getMetricsReporterType() {
return MetricsReporterType.valueOf(getString(HoodieMetricsConfig.METRICS_REPORTER_TYPE_VALUE));
}
Expand Down
12 changes: 12 additions & 0 deletions .../hudi-client-common/src/main/java/org/apache/hudi/config/metrics/HoodieMetricsConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ public class HoodieMetricsConfig extends HoodieConfig {
.sinceVersion("0.7.0")
.withDocumentation("");

public static final ConfigProperty<Boolean> ENABLE_COMMON_PREFIX = ConfigProperty
.key(METRIC_PREFIX + ".common_prefix.enable")
.defaultValue(true)
.sinceVersion("0.10.0")
Copy link
Member

@codope codope Feb 3, 2022

Choose a reason for hiding this comment

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

version should be 0.11.0

.withDocumentation("Turn on/off common prefix for all metrics. Table name is used as the common prefix and its "
+ "on by default");

/**
* @deprecated Use {@link #TURN_METRICS_ON} and its methods instead
*/
Expand Down Expand Up @@ -164,6 +171,11 @@ public Builder withExecutorMetrics(boolean enable) {
return this;
}

public Builder withCommonPrefix(boolean enable) {
hoodieMetricsConfig.setValue(ENABLE_COMMON_PREFIX, String.valueOf(enable));
return this;
}

public HoodieMetricsConfig build() {

hoodieMetricsConfig.setDefaults(HoodieMetricsConfig.class.getName());
Expand Down
1 change: 1 addition & 0 deletions ...client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ private HoodieWriteConfig createMetadataWriteConfig(HoodieWriteConfig writeConfi
builder.withMetricsConfig(HoodieMetricsConfig.newBuilder()
.withReporterType(writeConfig.getMetricsReporterType().toString())
.withExecutorMetrics(writeConfig.isExecutorMetricsEnabled())
.withCommonPrefix(writeConfig.isMetricsCommonPrefixEnabled())
.on(true).build());
switch (writeConfig.getMetricsReporterType()) {
case GRAPHITE:
Expand Down
8 changes: 7 additions & 1 deletion hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/HoodieMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,13 @@ public void updateIndexMetrics(final String action, final long durationInMs) {
}

String getMetricsName(String action, String metric) {
return config == null ? null : String.format("%s.%s.%s", config.getMetricReporterMetricsNamePrefix(), action, metric);
if (config == null) {
return null;
}
if (config.isMetricsCommonPrefixEnabled()) {
return String.format("%s.%s.%s", tableName, action, metric);
}
return String.format("%s.%s", action, metric);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/Metrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class Metrics {

private Metrics(HoodieWriteConfig metricConfig) {
registry = new MetricRegistry();
commonMetricPrefix = metricConfig.getMetricReporterMetricsNamePrefix();
commonMetricPrefix = metricConfig.isMetricsCommonPrefixEnabled() ? metricConfig.getTableName() : "";
reporter = MetricsReporterFactory.createReporter(metricConfig, registry);
if (reporter == null) {
throw new RuntimeException("Cannot initialize Reporter.");
Expand Down Expand Up @@ -116,7 +116,7 @@ public static synchronized void flush() {
}

public static void registerGauges(Map<String, Long> metricsMap, Option<String> prefix) {
String metricPrefix = prefix.isPresent() ? prefix.get() + "." : "";
String metricPrefix = prefix.isPresent() && !prefix.get().isEmpty() ? prefix.get() + "." : "";
metricsMap.forEach((k, v) -> registerGauge(metricPrefix + k, v));
}

Expand Down
8 changes: 7 additions & 1 deletion ...ies/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamerMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,13 @@ private Timer createTimer(String name) {
}

String getMetricsName(String action, String metric) {
return config == null ? null : String.format("%s.%s.%s", config.getMetricReporterMetricsNamePrefix(), action, metric);
if (config == null) {
return null;
}
if (config.isMetricsCommonPrefixEnabled()) {
return String.format("%s.%s.%s", tableName, action, metric);
}
return String.format("%s.%s", action, metric);
}

public void updateDeltaStreamerMetrics(long durationInNs) {
Expand Down