Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Commit

Permalink
Only increment the added transaction counter if we actually added the…
Browse files Browse the repository at this point in the history
… transaction (#1543)
  • Loading branch information
ajsutton authored Jun 10, 2019
1 parent a7bb9eb commit 015350d
Show file tree
Hide file tree
Showing 5 changed files with 173 additions and 10 deletions.
1 change: 1 addition & 0 deletions ethereum/eth/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,20 @@ List<Transaction> 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -72,17 +76,19 @@ public void shouldReturnExclusivelyLocalTransactionsWhenAppropriate() {
transactions.addRemoteTransaction(transaction2);
assertThat(transactions.size()).isEqualTo(3);

List<Transaction> localTransactions = transactions.getLocalTransactions();
final List<Transaction> localTransactions = transactions.getLocalTransactions();
assertThat(localTransactions.size()).isEqualTo(1);
}

@Test
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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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);
}
}
5 changes: 5 additions & 0 deletions metrics/core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Original file line number Diff line number Diff line change
@@ -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;
}
}
}

0 comments on commit 015350d

Please sign in to comment.