From 015350d594a107ef1c1a14fe4a10f12329f119a8 Mon Sep 17 00:00:00 2001 From: Adrian Sutton Date: Mon, 10 Jun 2019 16:11:32 +1000 Subject: [PATCH] Only increment the added transaction counter if we actually added the transaction (#1543) --- ethereum/eth/build.gradle | 1 + .../eth/transactions/PendingTransactions.java | 16 ++- .../transactions/PendingTransactionsTest.java | 49 +++++++- metrics/core/build.gradle | 5 + .../pantheon/metrics/StubMetricsSystem.java | 112 ++++++++++++++++++ 5 files changed, 173 insertions(+), 10 deletions(-) create mode 100644 metrics/core/src/test-support/java/tech/pegasys/pantheon/metrics/StubMetricsSystem.java diff --git a/ethereum/eth/build.gradle b/ethereum/eth/build.gradle index 3acc561496..7d8309a5a8 100644 --- a/ethereum/eth/build.gradle +++ b/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' diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java index a10dcf295f..71638709de 100644 --- a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java +++ b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactions.java @@ -122,16 +122,20 @@ List getLocalTransactions() { public boolean addRemoteTransaction(final Transaction transaction) { final TransactionInfo transactionInfo = new TransactionInfo(transaction, false, clock.instant()); - final boolean addTransaction = addTransaction(transactionInfo); - remoteTransactionAddedCounter.inc(); - return addTransaction; + final boolean transactionAdded = addTransaction(transactionInfo); + if (transactionAdded) { + remoteTransactionAddedCounter.inc(); + } + return transactionAdded; } boolean addLocalTransaction(final Transaction transaction) { - final boolean addTransaction = + final boolean transactionAdded = addTransaction(new TransactionInfo(transaction, true, clock.instant())); - localTransactionAddedCounter.inc(); - return addTransaction; + if (transactionAdded) { + localTransactionAddedCounter.inc(); + } + return transactionAdded; } void removeTransaction(final Transaction transaction) { diff --git a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java index 77693bd794..2d76fba0f8 100644 --- a/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java +++ b/ethereum/eth/src/test/java/tech/pegasys/pantheon/ethereum/eth/transactions/PendingTransactionsTest.java @@ -25,8 +25,7 @@ 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.noop.NoOpMetricsSystem; +import tech.pegasys.pantheon.metrics.StubMetricsSystem; 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 void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { transactions.addRemoteTransaction(transaction2); assertThat(transactions.size()).isEqualTo(3); - List localTransactions = transactions.getLocalTransactions(); + final List localTransactions = transactions.getLocalTransactions(); assertThat(localTransactions.size()).isEqualTo(1); } @@ -80,9 +84,11 @@ public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() { 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 void shouldDropOldestTransactionWhenLimitExceeded() { 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 void shouldReplaceTransactionWithSameSenderAndNonce() { 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 void shouldReplaceOnlyTransactionFromSenderWhenItHasTheSameNonce() { 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 void shouldEvictMultipleOldTransactions() { 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 void shouldEvictSingleOldTransaction() { 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 void shouldEvictExclusivelyOldTransactions() { 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); } } diff --git a/metrics/core/build.gradle b/metrics/core/build.gradle index f919d0cc37..8770fea7b1 100644 --- a/metrics/core/build.gradle +++ b/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 } diff --git a/metrics/core/src/test-support/java/tech/pegasys/pantheon/metrics/StubMetricsSystem.java b/metrics/core/src/test-support/java/tech/pegasys/pantheon/metrics/StubMetricsSystem.java new file mode 100644 index 0000000000..23b9fa8544 --- /dev/null +++ b/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 counters = new HashMap<>(); + private final Map gauges = new HashMap<>(); + + @Override + public LabelledMetric 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 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 streamObservations(final MetricCategory category) { + throw new UnsupportedOperationException("Observations aren't actually recorded"); + } + + public static class StubLabelledCounter implements LabelledMetric { + private final Map, 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; + } + } +}