Skip to content

Commit

Permalink
review fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@Vladsz83
Vladsz83 committed Mar 16, 2024
1 parent 85472c2 commit 0ac81b0
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 68 deletions.
4 changes: 0 additions & 4 deletions ...es/core/src/main/java/org/apache/ignite/internal/processors/metric/GridMetricManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.function.Consumer;
import java.util.function.Supplier;
import java.util.regex.Pattern;
import org.apache.ignite.IgniteCheckedException;
import org.apache.ignite.IgniteException;
import org.apache.ignite.internal.GridKernalContext;
Expand Down Expand Up @@ -144,9 +143,6 @@ public class GridMetricManager extends GridManagerAdapter<MetricExporterSpi> imp
/** Custom metrics registry name. */
public static final String CUSTOM_METRICS_PREF = CUSTOM_METRICS + SEPARATOR;

/** */
private static final Pattern SEPARATOR_PATTERN = Pattern.compile("\\" + SEPARATOR);

/** JVM interface to memory consumption info */
private static final MemoryMXBean mem = ManagementFactory.getMemoryMXBean();

Expand Down
6 changes: 3 additions & 3 deletions modules/core/src/main/java/org/apache/ignite/internal/processors/metric/MetricRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,8 @@ private <T extends Metric> T addMetric(String name, T metric) throws IgniteExcep
return (T)old;
}

configureMetrics(metric);
if (!name().startsWith(CUSTOM_METRICS))
configureMetrics(metric);

return metric;
}
Expand All @@ -296,8 +297,7 @@ private <T extends Metric> T addMetric(String name, T metric) throws IgniteExcep
* Assigns metric settings if {@code metric} is configurable.
*/
private void configureMetrics(Metric metric) {
if (name().startsWith(CUSTOM_METRICS))
return;
assert !name().startsWith(CUSTOM_METRICS) : "Custom metrics cannot be configured yet";

if (metric instanceof HistogramMetricImpl) {
long[] cfgBounds = histogramCfgProvider.apply(metric.name());
Expand Down
41 changes: 8 additions & 33 deletions ...les/core/src/main/java/org/apache/ignite/internal/processors/metric/impl/MetricUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@

import static org.apache.ignite.internal.processors.cache.CacheGroupMetricsImpl.CACHE_GROUP_METRICS_PREFIX;
import static org.apache.ignite.internal.processors.cache.CacheMetricsImpl.CACHE_METRICS;
import static org.apache.ignite.internal.processors.metric.GridMetricManager.CUSTOM_METRICS;

/**
* Utility class to build or parse metric name in dot notation.
Expand All @@ -47,38 +46,26 @@ public class MetricUtils {
/** Histogram name divider. */
public static final char HISTOGRAM_NAME_DIVIDER = '_';

/** Metric name pattern. */
private static final Pattern SPACES_PATTERN = Pattern.compile(".*[\\s]+.*");

/** Metric name pattern. */
//Dont allow: '.' at the start or end, consiquent '.', non alphadigits except hyphen and column.
private static final Pattern STRICT_NAME_PATTERN = Pattern.compile("^(?!\\.)(?!.*\\.$)(?!.*\\.\\.)(?!.*-$)(?!.*\\.\\.)[\\w-.:]+$");
/** Metric name pattern. Permits empty string, spaces, tabs, dot at the start or end, consiquent dots. */
private static final Pattern NAME_PATTERN = Pattern.compile("(?!\\.)(?!.*\\.$)(?!.*\\.\\.)(?!.*[\\s]+.*).+");

/**
* Chechs and builds metric name. Each parameter will separated by '.'.
* Chechs and builds metric name.
*
* @param names Metric name parts.
* @return Metric name.
*/
public static String metricName(String... names) {
assert names != null;
assert names != null && names.length > 0 : "Metric name must consist of at least one element.";

for (int i = 0; i < names.length; i++) {
if (names[i] == null || names[i].isEmpty() || SPACES_PATTERN.matcher(names[i]).matches()) {
throw new IllegalArgumentException("Illegal metric or registry name. Spaces, nulls or empty name parts " +
"are not allowed.");
if (names[i] == null || names[i].isEmpty() || !NAME_PATTERN.matcher(names[i]).matches()) {
throw new IllegalArgumentException("Illegal metric or registry name. Spaces, nulls or empty name " +
"parts are not allowed.");
}
}

String res = String.join(SEPARATOR, names);

if (res.startsWith(CUSTOM_METRICS) && !STRICT_NAME_PATTERN.matcher(res).matches()) {
throw new IllegalArgumentException("Metric or registry name '" + res + "' is illegal. It cannot have " +
"spaces, sequenced dots, start or end with dots or hyphen. Allowed: characters, digits, undersore, " +
"hypen, column and dot-separators.");
}

return res;
return String.join(SEPARATOR, names);
}

/**
Expand Down Expand Up @@ -165,18 +152,6 @@ public static void setIfGreater(AtomicLongMetric m, long update) {
v = m.value();
}

/**
* Asserts all arguments are not empty and contains no spaces.
*
* @param names Names.
*/
private static void ensureNotEmptyAndHasNoSpaces(String... names) {
for (int i = 0; i < names.length; i++) {
if (names[i] == null || names[i].isEmpty() || SPACES_PATTERN.matcher(names[i]).matches())
throw new IllegalArgumentException("Metric name element " + (i + 1) + " is empty or contains spaces.");
}
}

/**
* Generates histogram bucket names.
*
Expand Down
2 changes: 1 addition & 1 deletion modules/core/src/main/java/org/apache/ignite/metric/IgniteMetrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
* <p>
* Note: Custom metric are registered on demand and aren't stored. If node restarts, the metrics require registration anew.
* <p>
* Any name or dot-separated name part must not be empty, can contain only characters, numbers, '-' and '_'.
* Any name or dot-separated name part must not be empty and cannot have spaces.
* Examples of custom metric registry names: "custom.admin.sessions", "custom.processes", "custom.processes.proc_1", etc.
*
* @see ReadOnlyMetricRegistry
Expand Down
39 changes: 12 additions & 27 deletions modules/core/src/test/java/org/apache/ignite/internal/metric/CustomMetricsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,56 +216,55 @@ public void testMetricName() {

AtomicInteger val = new AtomicInteger();

String errTxt1 = "Illegal metric or registry name. Spaces, nulls or empty name parts are not allowed";
String errTxt2 = "is illegal. It cannot have spaces, sequenced dots, start or end with dots or hyphen";
String errTxt = "Illegal metric or registry name. Spaces, nulls or empty name parts are not allowed";

assertThrows(
null,
() -> metrics.customRegistry("").register("intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry(null).register("intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry("myreg").register(null, val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry(CUSTOM_METRICS + ' ').register("intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry(" \t myreg ").register("intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry(" \t ").register("intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry("a.b").register(" \t c . \t d \t . \t intMetric", val::get, null),
IllegalArgumentException.class,
errTxt1
errTxt
);

assertNotNull(metrics.findRegistry(CUSTOM_METRICS + ".a.b"));
Expand All @@ -274,42 +273,28 @@ public void testMetricName() {
null,
() -> metrics.customRegistry("myreg").register("intValues..intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt2
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry("myreg.").register("intValues.intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt2
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry(".myreg.").register("intValues.intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt2
errTxt
);

assertThrows(
null,
() -> metrics.customRegistry("myreg..reg2").register("intValues.intMetric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt2
);

assertThrows(
null,
() -> metrics.customRegistry("myreg.reg2").register("intValues.int@Metric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt2
);

assertThrows(
null,
() -> metrics.customRegistry("myreg.reg2").register("intValues.int+Metric1", val::get, "intMetric1Desc"),
IllegalArgumentException.class,
errTxt2
errTxt
);

metrics.customRegistry(CUSTOM_METRICS).register("intMetric1", val::get, "intMetric1Desc");
Expand Down

0 comments on commit 0ac81b0

Please sign in to comment.