Only increment the added transaction counter if we actually added the transaction (#1543)

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

ethereum/eth/build.gradle

@@ -48,6 +48,7 @@ dependencies {
   testImplementation project(path: ':ethereum:core', configuration: 'testArtifacts')
   testImplementation project(path: ':ethereum:core', configuration: 'testSupportArtifacts')
   testImplementation project(':ethereum:mock-p2p')
+  testImplementation project(path: ':metrics:core', configuration: 'testSupportArtifacts')
   testImplementation project(':testutil')
 
   testImplementation 'junit:junit'

ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java

@@ -122,16 +122,20 @@ public class PendingTransactions {
   public boolean addRemoteTransaction(final Transaction transaction) {
     final TransactionInfo transactionInfo =
         new TransactionInfo(transaction, false, clock.instant());
-    final boolean addTransaction = addTransaction(transactionInfo);
+    final boolean transactionAdded = addTransaction(transactionInfo);
-    remoteTransactionAddedCounter.inc();
+    if (transactionAdded) {
-    return addTransaction;
+      remoteTransactionAddedCounter.inc();
+    }
+    return transactionAdded;
   }
 
   boolean addLocalTransaction(final Transaction transaction) {
-    final boolean addTransaction =
+    final boolean transactionAdded =
         addTransaction(new TransactionInfo(transaction, true, clock.instant()));
-    localTransactionAddedCounter.inc();
+    if (transactionAdded) {
-    return addTransaction;
+      localTransactionAddedCounter.inc();
+    }
+    return transactionAdded;
   }
 
   void removeTransaction(final Transaction transaction) {

ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java

@@ -25,8 +25,7 @@ import tech.pegasys.pantheon.ethereum.core.TransactionTestFixture;
 import tech.pegasys.pantheon.ethereum.core.Util;
 import tech.pegasys.pantheon.ethereum.core.Wei;
 import tech.pegasys.pantheon.ethereum.eth.transactions.PendingTransactions.TransactionSelectionResult;
-import tech.pegasys.pantheon.metrics.MetricsSystem;
+import tech.pegasys.pantheon.metrics.StubMetricsSystem;
-import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
 import tech.pegasys.pantheon.testutil.TestClock;
 
 import java.time.temporal.ChronoUnit;
@@ -42,9 +41,14 @@ public class PendingTransactionsTest {
   private static final int MAX_TRANSACTIONS = 5;
   private static final KeyPair KEYS1 = KeyPair.generate();
   private static final KeyPair KEYS2 = KeyPair.generate();
+  private static final String ADDED_COUNTER = "transactions_added_total";
+  private static final String REMOVED_COUNTER = "transactions_removed_total";
+  private static final String REMOTE = "remote";
+  private static final String LOCAL = "local";
+  private static final String DROPPED = "dropped";
 
   private final TestClock clock = new TestClock();
-  private final MetricsSystem metricsSystem = new NoOpMetricsSystem();
+  private final StubMetricsSystem metricsSystem = new StubMetricsSystem();
   private final PendingTransactions transactions =
       new PendingTransactions(
           PendingTransactions.DEFAULT_TX_RETENTION_HOURS,
@@ -72,7 +76,7 @@ public class PendingTransactionsTest {
     transactions.addRemoteTransaction(transaction2);
     assertThat(transactions.size()).isEqualTo(3);
 
-    List<Transaction> localTransactions = transactions.getLocalTransactions();
+    final List<Transaction> localTransactions = transactions.getLocalTransactions();
     assertThat(localTransactions.size()).isEqualTo(1);
   }
 
@@ -80,9 +84,11 @@ public class PendingTransactionsTest {
   public void shouldAddATransaction() {
     transactions.addRemoteTransaction(transaction1);
     assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1);
 
     transactions.addRemoteTransaction(transaction2);
     assertThat(transactions.size()).isEqualTo(2);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(2);
   }
 
   @Test
@@ -104,10 +110,12 @@ public class PendingTransactionsTest {
       transactions.addRemoteTransaction(createTransaction(i));
     }
     assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isZero();
 
     transactions.addRemoteTransaction(createTransaction(MAX_TRANSACTIONS + 1));
     assertThat(transactions.size()).isEqualTo(MAX_TRANSACTIONS);
     assertTransactionNotPending(oldestTransaction);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
   }
 
   @Test
@@ -278,6 +286,8 @@ public class PendingTransactionsTest {
     assertTransactionPending(transaction1b);
     assertTransactionPending(transaction2);
     assertThat(transactions.size()).isEqualTo(2);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(3);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
   }
 
   @Test
@@ -290,6 +300,8 @@ public class PendingTransactionsTest {
     assertTransactionNotPending(transaction1);
     assertTransactionPending(transaction1b);
     assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(2);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
   }
 
   @Test
@@ -431,6 +443,7 @@ public class PendingTransactionsTest {
     clock.step(2L, ChronoUnit.HOURS);
     transactions.evictOldTransactions();
     assertThat(transactions.size()).isEqualTo(0);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(2);
   }
 
   @Test
@@ -444,6 +457,7 @@ public class PendingTransactionsTest {
     clock.step(2L, ChronoUnit.HOURS);
     transactions.evictOldTransactions();
     assertThat(transactions.size()).isEqualTo(0);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
   }
 
   @Test
@@ -459,5 +473,32 @@ public class PendingTransactionsTest {
     assertThat(transactions.size()).isEqualTo(2);
     transactions.evictOldTransactions();
     assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(REMOVED_COUNTER, REMOTE, DROPPED)).isEqualTo(1);
+  }
+
+  @Test
+  public void shouldNotIncrementAddedCounterWhenRemoteTransactionAlreadyPresent() {
+    transactions.addLocalTransaction(transaction1);
+    assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0);
+
+    assertThat(transactions.addRemoteTransaction(transaction1)).isFalse();
+    assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(0);
+  }
+
+  @Test
+  public void shouldNotIncrementAddedCounterWhenLocalTransactionAlreadyPresent() {
+    transactions.addRemoteTransaction(transaction1);
+    assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1);
+
+    assertThat(transactions.addLocalTransaction(transaction1)).isFalse();
+    assertThat(transactions.size()).isEqualTo(1);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, LOCAL)).isEqualTo(0);
+    assertThat(metricsSystem.getCounterValue(ADDED_COUNTER, REMOTE)).isEqualTo(1);
   }
 }

metrics/core/build.gradle

@@ -44,4 +44,9 @@ dependencies {
   testImplementation 'org.assertj:assertj-core'
   testImplementation 'org.mockito:mockito-core'
   testImplementation 'com.squareup.okhttp3:okhttp'
+
+  testSupportImplementation 'org.mockito:mockito-core'
 }
+
+
+artifacts { testSupportArtifacts testSupportJar }

metrics/core/src/test-support/java/tech/pegasys/pantheon/metrics/StubMetricsSystem.java

@@ -0,0 +1,112 @@
+/*
+ * Copyright 2019 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package tech.pegasys.pantheon.metrics;
+
+import static java.util.Arrays.asList;
+
+import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.DoubleSupplier;
+import java.util.stream.Stream;
+
+public class StubMetricsSystem implements MetricsSystem {
+
+  private final Map<String, StubLabelledCounter> counters = new HashMap<>();
+  private final Map<String, DoubleSupplier> gauges = new HashMap<>();
+
+  @Override
+  public LabelledMetric<Counter> createLabelledCounter(
+      final MetricCategory category,
+      final String name,
+      final String help,
+      final String... labelNames) {
+    return counters.computeIfAbsent(name, key -> new StubLabelledCounter());
+  }
+
+  public long getCounterValue(final String name, final String... labels) {
+    final StubLabelledCounter labelledCounter = counters.get(name);
+    if (labelledCounter == null) {
+      throw new IllegalArgumentException("Unknown counter: " + name);
+    }
+    final StubCounter metric = labelledCounter.getMetric(labels);
+    if (metric == null) {
+      return 0;
+    }
+    return metric.getValue();
+  }
+
+  @Override
+  public LabelledMetric<OperationTimer> createLabelledTimer(
+      final MetricCategory category,
+      final String name,
+      final String help,
+      final String... labelNames) {
+    return labelValues -> NoOpMetricsSystem.NO_OP_OPERATION_TIMER;
+  }
+
+  @Override
+  public void createGauge(
+      final MetricCategory category,
+      final String name,
+      final String help,
+      final DoubleSupplier valueSupplier) {
+    gauges.put(name, valueSupplier);
+  }
+
+  public double getGaugeValue(final String name) {
+    final DoubleSupplier gauge = gauges.get(name);
+    if (gauge == null) {
+      throw new IllegalArgumentException("Unknown gauge: " + name);
+    }
+    return gauge.getAsDouble();
+  }
+
+  @Override
+  public Stream<Observation> streamObservations(final MetricCategory category) {
+    throw new UnsupportedOperationException("Observations aren't actually recorded");
+  }
+
+  public static class StubLabelledCounter implements LabelledMetric<Counter> {
+    private final Map<List<String>, StubCounter> metrics = new HashMap<>();
+
+    @Override
+    public Counter labels(final String... labels) {
+      return metrics.computeIfAbsent(asList(labels), key -> new StubCounter());
+    }
+
+    private StubCounter getMetric(final String... labels) {
+      return metrics.get(asList(labels));
+    }
+  }
+
+  public static class StubCounter implements Counter {
+    private long value = 0;
+
+    @Override
+    public void inc() {
+      value++;
+    }
+
+    @Override
+    public void inc(final long amount) {
+      value += amount;
+    }
+
+    public long getValue() {
+      return value;
+    }
+  }
+}