You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by bt...@apache.org on 2019/12/19 05:40:41 UTC
[james-project] 07/08: JAMES-3007 MetricContract enforce getCount()
return non-negative value
This is an automated email from the ASF dual-hosted git repository.
btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git
commit 09dd546216307dc84ab5db8262d4c5ef47f1e1ff
Author: Tran Tien Duc <dt...@linagora.com>
AuthorDate: Tue Dec 17 10:06:52 2019 +0700
JAMES-3007 MetricContract enforce getCount() return non-negative value
---
.../java/org/apache/james/metrics/api/Metric.java | 5 ++++
.../apache/james/metrics/api/MetricContract.java | 27 ++++++++++++++++------
.../james/metrics/api/MetricFactoryContract.java | 6 ++---
.../james/metrics/dropwizard/DropWizardMetric.java | 16 +++++++++++--
.../dropwizard/DropWizardMetricFactory.java | 2 +-
.../metrics/dropwizard/DropWizardMetricTest.java | 13 ++++++++++-
.../apache/james/metrics/logger/DefaultMetric.java | 18 ++++++++++++---
.../james/metrics/logger/DefaultMetricFactory.java | 2 +-
.../james/metrics/logger/DefaultMetricTest.java | 13 ++++++++++-
.../james/metrics/tests/RecordingMetric.java | 15 +++++++++---
.../james/metrics/tests/RecordingMetricTest.java | 18 +++++++++++++++
11 files changed, 113 insertions(+), 22 deletions(-)
diff --git a/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java b/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java
index 1b2e879..8f35998 100644
--- a/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java
+++ b/metrics/metrics-api/src/main/java/org/apache/james/metrics/api/Metric.java
@@ -29,5 +29,10 @@ public interface Metric {
void remove(int value);
+ /**
+ * @return A long guaranteed to be positive.
+ *
+ * Implementation might be doing strict validation (throwing on negative value) or lenient (log and sanitize to 0)
+ */
long getCount();
}
diff --git a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java
index 4c6bf07..59a975d 100644
--- a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java
+++ b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricContract.java
@@ -47,20 +47,24 @@ public interface MetricContract {
@Test
default void decrementShouldDecreaseCounter() {
+ testee().add(2);
+
testee().decrement();
assertThat(testee().getCount())
- .isEqualTo(-1);
+ .isEqualTo(1);
}
@Test
default void decrementShouldDecreaseCounterAfterMultipleCalls() {
+ testee().add(6);
+
testee().decrement();
testee().decrement();
testee().decrement();
assertThat(testee().getCount())
- .isEqualTo(-3);
+ .isEqualTo(3);
}
@Test
@@ -73,10 +77,11 @@ public interface MetricContract {
@Test
default void addShouldDecreaseCounterWhenNegativeNumber() {
+ testee().add(10);
testee().add(-9);
assertThat(testee().getCount())
- .isEqualTo(-9);
+ .isEqualTo(1);
}
@Test
@@ -90,10 +95,11 @@ public interface MetricContract {
@Test
default void removeShouldDecreaseCounter() {
- testee().remove(10);
+ testee().add(10);
+ testee().remove(9);
assertThat(testee().getCount())
- .isEqualTo(-10);
+ .isEqualTo(1);
}
@Test
@@ -106,11 +112,11 @@ public interface MetricContract {
@Test
default void removeShouldKeepCounterBeTheSameWhenZero() {
- testee().remove(888);
+ testee().add(888);
testee().remove(0);
assertThat(testee().getCount())
- .isEqualTo(-888);
+ .isEqualTo(888);
}
@Test
@@ -118,4 +124,11 @@ public interface MetricContract {
assertThat(testee().getCount())
.isEqualTo(0);
}
+
+ @Test
+ default void getCountShouldReturnValueWhenCounterIsPositive() {
+ testee().add(19);
+ assertThat(testee().getCount())
+ .isEqualTo(19);
+ }
}
\ No newline at end of file
diff --git a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java
index 5db31fa..1cbc06d 100644
--- a/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java
+++ b/metrics/metrics-api/src/test/java/org/apache/james/metrics/api/MetricFactoryContract.java
@@ -47,12 +47,12 @@ public interface MetricFactoryContract {
Metric metric1 = testee().generate(NAME_1);
Metric metric2 = testee().generate(NAME_2);
- metric1.increment();
- metric2.decrement();
+ metric1.add(1);
+ metric2.add(2);
SoftAssertions.assertSoftly(softly -> {
softly.assertThat(metric1.getCount()).isEqualTo(1);
- softly.assertThat(metric2.getCount()).isEqualTo(-1);
+ softly.assertThat(metric2.getCount()).isEqualTo(2);
});
}
}
\ No newline at end of file
diff --git a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java
index 21a192d..58a0332 100644
--- a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java
+++ b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetric.java
@@ -20,15 +20,21 @@
package org.apache.james.metrics.dropwizard;
import org.apache.james.metrics.api.Metric;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import com.codahale.metrics.Counter;
public class DropWizardMetric implements Metric {
+ private static final Logger LOGGER = LoggerFactory.getLogger(DropWizardMetric.class);
+
private final Counter counter;
+ private final String metricName;
- public DropWizardMetric(Counter counter) {
+ public DropWizardMetric(Counter counter, String metricName) {
this.counter = counter;
+ this.metricName = metricName;
}
@Override
@@ -53,6 +59,12 @@ public class DropWizardMetric implements Metric {
@Override
public long getCount() {
- return counter.getCount();
+ long value = counter.getCount();
+ if (value < 0) {
+ LOGGER.error("counter value({}) of the metric '{}' should not be a negative number", value, metricName);
+ return 0;
+ }
+
+ return value;
}
}
diff --git a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java
index 015c21d..2d040d4 100644
--- a/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java
+++ b/metrics/metrics-dropwizard/src/main/java/org/apache/james/metrics/dropwizard/DropWizardMetricFactory.java
@@ -45,7 +45,7 @@ public class DropWizardMetricFactory implements MetricFactory, Startable {
@Override
public Metric generate(String name) {
- return new DropWizardMetric(metricRegistry.counter(name));
+ return new DropWizardMetric(metricRegistry.counter(name), name);
}
@Override
diff --git a/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java b/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java
index 1183bc4..f7ab436 100644
--- a/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java
+++ b/metrics/metrics-dropwizard/src/test/java/org/apache/james/metrics/dropwizard/DropWizardMetricTest.java
@@ -19,9 +19,12 @@
package org.apache.james.metrics.dropwizard;
+import static org.assertj.core.api.Assertions.assertThat;
+
import org.apache.james.metrics.api.Metric;
import org.apache.james.metrics.api.MetricContract;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
import com.codahale.metrics.MetricRegistry;
@@ -34,11 +37,19 @@ class DropWizardMetricTest implements MetricContract {
@BeforeEach
void setUp() {
MetricRegistry registry = new MetricRegistry();
- testee = new DropWizardMetric(registry.counter(METRIC_NAME));
+ testee = new DropWizardMetric(registry.counter(METRIC_NAME), METRIC_NAME);
}
@Override
public Metric testee() {
return testee;
}
+
+ @Test
+ void getCountShouldReturnZeroWhenCounterIsNegative() {
+ testee().remove(9);
+
+ assertThat(testee().getCount())
+ .isEqualTo(0);
+ }
}
\ No newline at end of file
diff --git a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java
index ed4d7d3..d9b11db 100644
--- a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java
+++ b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetric.java
@@ -21,12 +21,18 @@ package org.apache.james.metrics.logger;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.james.metrics.api.Metric;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class DefaultMetric implements Metric {
- private AtomicInteger value;
+ private static final Logger LOGGER = LoggerFactory.getLogger(DefaultMetric.class);
- public DefaultMetric() {
+ private final AtomicInteger value;
+ private final String metricName;
+
+ public DefaultMetric(String metricName) {
+ this.metricName = metricName;
value = new AtomicInteger();
}
@@ -52,6 +58,12 @@ public class DefaultMetric implements Metric {
@Override
public long getCount() {
- return value.get();
+ long counter = value.longValue();
+ if (counter < 0) {
+ LOGGER.error("counter value({}) of the metric '{}' should not be a negative number", value, metricName);
+ return 0;
+ }
+
+ return counter;
}
}
diff --git a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java
index b00e3f3..db64916 100644
--- a/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java
+++ b/metrics/metrics-logger/src/main/java/org/apache/james/metrics/logger/DefaultMetricFactory.java
@@ -30,7 +30,7 @@ public class DefaultMetricFactory implements MetricFactory {
@Override
public Metric generate(String name) {
- return new DefaultMetric();
+ return new DefaultMetric(name);
}
@Override
diff --git a/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java b/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java
index 3ab0d6d..8e8ccd2 100644
--- a/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java
+++ b/metrics/metrics-logger/src/test/java/org/apache/james/metrics/logger/DefaultMetricTest.java
@@ -19,9 +19,12 @@
package org.apache.james.metrics.logger;
+import static org.assertj.core.api.Assertions.assertThat;
+
import org.apache.james.metrics.api.Metric;
import org.apache.james.metrics.api.MetricContract;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
class DefaultMetricTest implements MetricContract {
@@ -29,11 +32,19 @@ class DefaultMetricTest implements MetricContract {
@BeforeEach
void setUp() {
- testee = new DefaultMetric();
+ testee = new DefaultMetric("metricName");
}
@Override
public Metric testee() {
return testee;
}
+
+ @Test
+ void getCountShouldReturnZeroWhenCounterIsNegative() {
+ testee().remove(9);
+
+ assertThat(testee().getCount())
+ .isEqualTo(0);
+ }
}
\ No newline at end of file
diff --git a/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java b/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java
index e004e86..5c08c76 100644
--- a/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java
+++ b/metrics/metrics-tests/src/main/java/org/apache/james/metrics/tests/RecordingMetric.java
@@ -41,7 +41,7 @@ public class RecordingMetric implements Metric {
@Override
public void decrement() {
- value.decrementAndGet();
+ value.updateAndGet(currentValue -> subtractFrom(currentValue, 1));
}
@Override
@@ -51,11 +51,20 @@ public class RecordingMetric implements Metric {
@Override
public void remove(int i) {
- value.addAndGet(-1 * i);
+ value.updateAndGet(currentValue -> subtractFrom(currentValue, i));
}
@Override
public long getCount() {
- return value.get();
+ return value.longValue();
+ }
+
+ private int subtractFrom(int currentValue, int minus) {
+ int result = currentValue - minus;
+ if (result < 0) {
+ throw new UnsupportedOperationException("metric counter is supposed to be a non-negative number," +
+ " thus this operation cannot be applied");
+ }
+ return result;
}
}
diff --git a/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java b/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java
index 682fa31..636ea69 100644
--- a/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java
+++ b/metrics/metrics-tests/src/test/java/org/apache/james/metrics/tests/RecordingMetricTest.java
@@ -19,9 +19,12 @@
package org.apache.james.metrics.tests;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
import org.apache.james.metrics.api.Metric;
import org.apache.james.metrics.api.MetricContract;
import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
class RecordingMetricTest implements MetricContract {
@@ -36,4 +39,19 @@ class RecordingMetricTest implements MetricContract {
public Metric testee() {
return testee;
}
+
+ @Test
+ void decrementShouldThrowWhenCounterIsZero() {
+ assertThatThrownBy(() -> testee.decrement())
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("metric counter is supposed to be a non-negative number, thus this operation cannot be applied");
+ }
+
+ @Test
+ void removeShouldThrowWhenCounterIsLessThanPassedParam() {
+ testee.add(10);
+ assertThatThrownBy(() -> testee.remove(11))
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage("metric counter is supposed to be a non-negative number, thus this operation cannot be applied");
+ }
}
\ No newline at end of file
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org