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

Plumb in three more metrics #344

Merged
merged 8 commits into from
Dec 3, 2018
Merged

Plumb in three more metrics #344

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4c2ac70
Plumb in three more metrics
shemnon Dec 1, 2018
9f8c908
Merge remote-tracking branch 'upstream/master' into metrics
shemnon Dec 2, 2018
59fc263
go back to instance on demand. My local server code was causing the …
shemnon Dec 2, 2018
61ee867
always init MetricSystem
shemnon Dec 2, 2018
dff58b4
Merge remote-tracking branch 'upstream/master' into metrics
shemnon Dec 3, 2018
d86bf6f
tweak formatting for log: 3 decimal places and correct units.
shemnon Dec 3, 2018
447e991
lost in the merge
shemnon Dec 3, 2018
54af8a7
initializing MetricsSystem only in the main command lead to failures …
shemnon Dec 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions acceptance-tests/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@ dependencies {
testRuntime 'org.apache.logging.log4j:log4j-core'
testRuntime 'org.apache.logging.log4j:log4j-slf4j-impl'

testImplementation project(':config')
testImplementation project(':consensus:clique')
testImplementation project(':crypto')
testImplementation project(':ethereum:eth')
testImplementation project(':ethereum:core')
testImplementation project(':ethereum:blockcreation')
testImplementation project(':ethereum:jsonrpc')
testImplementation project(':metrics')
testImplementation project(':pantheon')
testImplementation project(':config')
testImplementation project(':consensus:clique')
testImplementation project(':util')
testImplementation project(path: ':ethereum:core', configuration: 'testSupportArtifacts')

Expand Down
4 changes: 4 additions & 0 deletions ...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import tech.pegasys.pantheon.controller.KeyPairUtil;
import tech.pegasys.pantheon.controller.PantheonController;
import tech.pegasys.pantheon.ethereum.eth.sync.SynchronizerConfiguration;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;

import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -48,6 +50,7 @@ public void startNode(final PantheonNode node) {
nodeExecutor = Executors.newCachedThreadPool();
}

final MetricsSystem noOpMetricsSystem = new NoOpMetricsSystem();
final PantheonControllerBuilder builder = new PantheonControllerBuilder();
final EthNetworkConfig ethNetworkConfig =
node.ethNetworkConfig()
Expand Down Expand Up @@ -81,6 +84,7 @@ public void startNode(final PantheonNode node) {
.webSocketConfiguration(node.webSocketConfiguration())
.dataDir(node.homeDirectory())
.bannedNodeIds(Collections.emptySet())
.metricsSystem(noOpMetricsSystem)
.build();

nodeExecutor.submit(runner::execute);
Expand Down
1 change: 1 addition & 0 deletions ethereum/core/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ dependencies {
implementation project(':crypto')
implementation project(':ethereum:rlp')
implementation project(':ethereum:trie')
implementation project(':metrics')
implementation project(':services:kvstore')

implementation 'com.fasterxml.jackson.core:jackson-databind'
Expand Down
17 changes: 16 additions & 1 deletion ethereum/core/src/main/java/tech/pegasys/pantheon/ethereum/db/DefaultMutableBlockchain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import tech.pegasys.pantheon.ethereum.core.Transaction;
import tech.pegasys.pantheon.ethereum.core.TransactionReceipt;
import tech.pegasys.pantheon.ethereum.util.InvalidConfigurationException;
import tech.pegasys.pantheon.metrics.MetricCategory;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.util.Subscribers;
import tech.pegasys.pantheon.util.uint.UInt256;

Expand All @@ -51,10 +53,23 @@ public class DefaultMutableBlockchain implements MutableBlockchain {
private final Subscribers<BlockAddedObserver> blockAddedObservers = new Subscribers<>();

public DefaultMutableBlockchain(
final Block genesisBlock, final BlockchainStorage blockchainStorage) {
final Block genesisBlock,
final BlockchainStorage blockchainStorage,
final MetricsSystem metricsSystem) {
checkNotNull(genesisBlock);
this.blockchainStorage = blockchainStorage;
this.setGenesis(genesisBlock);

metricsSystem.createGauge(
MetricCategory.BLOCKCHAIN,
"height",
"Height of the chainhead",
() -> (double) this.getChainHeadBlockNumber());
metricsSystem.createGauge(
MetricCategory.BLOCKCHAIN,
"difficulty_total",
"Total difficulty of the chainhead",
() -> (double) this.getChainHead().getTotalDifficulty().toLong());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast to double is proving to be an annoyingly common pattern. Should we introduce a LongSupplier version of gauges? Even if it just did the cast internally it would tidy code up. Also, in hind-sight I should have used DoubleSupplier. Might be a separate PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because I also see the potential of a UInt256Supplier.

}

@Override
Expand Down
4 changes: 3 additions & 1 deletion ...rc/test-support/java/tech/pegasys/pantheon/ethereum/core/ExecutionContextTestFixture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStoragePrefixedKeyBlockchainStorage;
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStorageWorldStateStorage;
import tech.pegasys.pantheon.ethereum.worldstate.DefaultMutableWorldState;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
import tech.pegasys.pantheon.services.kvstore.InMemoryKeyValueStorage;
import tech.pegasys.pantheon.services.kvstore.KeyValueStorage;

Expand All @@ -50,7 +51,8 @@ private ExecutionContextTestFixture(
new DefaultMutableBlockchain(
genesis,
new KeyValueStoragePrefixedKeyBlockchainStorage(
keyValueStorage, MainnetBlockHashFunction::createHash));
keyValueStorage, MainnetBlockHashFunction::createHash),
new NoOpMetricsSystem());
this.stateArchive =
new WorldStateArchive(new KeyValueStorageWorldStateStorage(keyValueStorage));
this.protocolSchedule = protocolSchedule;
Expand Down
4 changes: 3 additions & 1 deletion ...re/src/test-support/java/tech/pegasys/pantheon/ethereum/core/InMemoryStorageProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStoragePrefixedKeyBlockchainStorage;
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStorageWorldStateStorage;
import tech.pegasys.pantheon.ethereum.worldstate.WorldStateStorage;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
import tech.pegasys.pantheon.services.kvstore.InMemoryKeyValueStorage;

public class InMemoryStorageProvider implements StorageProvider {
Expand All @@ -36,7 +37,8 @@ public static MutableBlockchain createInMemoryBlockchain(
final InMemoryKeyValueStorage keyValueStorage = new InMemoryKeyValueStorage();
return new DefaultMutableBlockchain(
genesisBlock,
new KeyValueStoragePrefixedKeyBlockchainStorage(keyValueStorage, blockHashFunction));
new KeyValueStoragePrefixedKeyBlockchainStorage(keyValueStorage, blockHashFunction),
new NoOpMetricsSystem());
}

public static WorldStateArchive createInMemoryWorldStateArchive() {
Expand Down
4 changes: 3 additions & 1 deletion ...um/core/src/test/java/tech/pegasys/pantheon/ethereum/db/DefaultMutableBlockchainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStoragePrefixedKeyBlockchainStorage;
import tech.pegasys.pantheon.ethereum.testutil.BlockDataGenerator;
import tech.pegasys.pantheon.ethereum.testutil.BlockDataGenerator.BlockOptions;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
import tech.pegasys.pantheon.services.kvstore.InMemoryKeyValueStorage;
import tech.pegasys.pantheon.services.kvstore.KeyValueStorage;
import tech.pegasys.pantheon.util.uint.UInt256;
Expand Down Expand Up @@ -713,6 +714,7 @@ private DefaultMutableBlockchain createBlockchain(
return new DefaultMutableBlockchain(
genesisBlock,
new KeyValueStoragePrefixedKeyBlockchainStorage(
kvStore, MainnetBlockHashFunction::createHash));
kvStore, MainnetBlockHashFunction::createHash),
new NoOpMetricsSystem());
}
}
4 changes: 3 additions & 1 deletion ethereum/core/src/test/java/tech/pegasys/pantheon/ethereum/db/GenesisBlockMismatchTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import tech.pegasys.pantheon.ethereum.mainnet.MainnetBlockHashFunction;
import tech.pegasys.pantheon.ethereum.storage.keyvalue.KeyValueStoragePrefixedKeyBlockchainStorage;
import tech.pegasys.pantheon.ethereum.util.InvalidConfigurationException;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
import tech.pegasys.pantheon.services.kvstore.InMemoryKeyValueStorage;
import tech.pegasys.pantheon.services.kvstore.KeyValueStorage;
import tech.pegasys.pantheon.util.bytes.Bytes32;
Expand Down Expand Up @@ -73,7 +74,8 @@ public void suppliedGenesisBlockMismatchStoredChainDataException() {
new DefaultMutableBlockchain(
genesisBlock00,
new KeyValueStoragePrefixedKeyBlockchainStorage(
kvStore, MainnetBlockHashFunction::createHash));
kvStore, MainnetBlockHashFunction::createHash),
new NoOpMetricsSystem());

final BlockHeader genesisHeader01 =
BlockHeaderBuilder.create()
Expand Down
21 changes: 18 additions & 3 deletions ...um/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@
import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
import tech.pegasys.pantheon.ethereum.p2p.wire.messages.DisconnectMessage.DisconnectReason;
import tech.pegasys.pantheon.ethereum.rlp.RLPException;
import tech.pegasys.pantheon.metrics.MetricCategory;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.metrics.OperationTimer;
import tech.pegasys.pantheon.util.uint.UInt256;

import java.util.ArrayList;
Expand Down Expand Up @@ -64,21 +67,29 @@ public class BlockPropagationManager<C> {

private final Set<Hash> requestedBlocks = new ConcurrentSet<>();
private final PendingBlocks pendingBlocks;
private final OperationTimer announcedBlockIngestTimer;

BlockPropagationManager(
final SynchronizerConfiguration config,
final ProtocolSchedule<C> protocolSchedule,
final ProtocolContext<C> protocolContext,
final EthContext ethContext,
final SyncState syncState,
final PendingBlocks pendingBlocks) {
final PendingBlocks pendingBlocks,
final MetricsSystem metricsSystem) {
this.config = config;
this.protocolSchedule = protocolSchedule;
this.protocolContext = protocolContext;
this.ethContext = ethContext;

this.syncState = syncState;
this.pendingBlocks = pendingBlocks;

this.announcedBlockIngestTimer =
metricsSystem.createTimer(
MetricCategory.BLOCKCHAIN,
"pantheon_blockchain_announcedBlock_ingest",
"Time to ingest a single announced block");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it meaningful to track importing an announced block separately to just importing a block? ie should we just time how long BlockImporter takes as that will capture all block imports? I could see it being useful to track how long it takes from first hearing about a block to it being on-chain, but this metric doesn't quite capture that since it only starts the timer after the new block has been retrieved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was timing the amount of time to process the block. Its separate code paths IIRc between download and announced blocks, and different tasks (download I see being split into at least three non-adjacent tasks : headers, bodies, receipts).

I could push this down into the persistBlock task, but I was hoping to have the number for the logging statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the timing in the log message is probably justification enough for this. I think at some point having a timer inside BlockImporter itself which will capture every block imported makes sense, but there are going to be a number of different timings related to block import (e.g. timing import of each batch of blocks when synchronizing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pluming it into tasks is a much larger PR. I'll leave it here for now, but it may move.

}

public void start() {
Expand Down Expand Up @@ -241,21 +252,25 @@ CompletableFuture<Block> importOrSavePendingBlock(final Block block) {
final PersistBlockTask<C> importTask =
PersistBlockTask.create(
protocolSchedule, protocolContext, block, HeaderValidationMode.FULL);
final OperationTimer.TimingContext blockTimer = announcedBlockIngestTimer.startTimer();
return ethContext
.getScheduler()
.scheduleWorkerTask(importTask::run)
.whenComplete(
(r, t) -> {
if (t != null) {
// TODO do we time failures? But we cannot drop a label in at this point.
LOG.warn(
"Failed to import announced block {} ({}).",
block.getHeader().getNumber(),
block.getHash());
} else {
final double time = blockTimer.stopTimer();
LOG.info(
"Successfully imported announced block {} ({}).",
"Successfully imported announced block {} ({}) in {}ms.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include the time in the log message is a nice touch.

block.getHeader().getNumber(),
block.getHash());
block.getHash(),
time);
}
});
}
Expand Down
7 changes: 5 additions & 2 deletions ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/DefaultSynchronizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import tech.pegasys.pantheon.ethereum.eth.sync.state.PendingBlocks;
import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
import tech.pegasys.pantheon.metrics.MetricsSystem;

import java.util.Optional;
import java.util.concurrent.atomic.AtomicBoolean;
Expand All @@ -40,7 +41,8 @@ public DefaultSynchronizer(
final ProtocolSchedule<C> protocolSchedule,
final ProtocolContext<C> protocolContext,
final EthContext ethContext,
final SyncState syncState) {
final SyncState syncState,
final MetricsSystem metricsSystem) {
this.syncState = syncState;
this.blockPropagationManager =
new BlockPropagationManager<>(
Expand All @@ -49,7 +51,8 @@ public DefaultSynchronizer(
protocolContext,
ethContext,
syncState,
new PendingBlocks());
new PendingBlocks(),
metricsSystem);
this.downloader =
new Downloader<>(syncConfig, protocolSchedule, protocolContext, ethContext, syncState);

Expand Down
9 changes: 7 additions & 2 deletions ...th/src/test/java/tech/pegasys/pantheon/ethereum/eth/sync/BlockPropagationManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import tech.pegasys.pantheon.ethereum.mainnet.ProtocolSchedule;
import tech.pegasys.pantheon.ethereum.testutil.BlockDataGenerator;
import tech.pegasys.pantheon.ethereum.testutil.BlockDataGenerator.BlockOptions;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.metrics.noop.NoOpMetricsSystem;
import tech.pegasys.pantheon.util.uint.UInt256;

import java.util.Collections;
Expand All @@ -56,6 +58,7 @@ public class BlockPropagationManagerTest {
private SynchronizerConfiguration syncConfig;
private final PendingBlocks pendingBlocks = new PendingBlocks();
private SyncState syncState;
private final MetricsSystem metricsSystem = new NoOpMetricsSystem();

@BeforeClass
public static void setupSuite() {
Expand Down Expand Up @@ -87,7 +90,8 @@ public void setup() {
protocolContext,
ethProtocolManager.ethContext(),
syncState,
pendingBlocks);
pendingBlocks,
metricsSystem);
}

@Test
Expand Down Expand Up @@ -462,7 +466,8 @@ public void purgesOldBlocks() {
protocolContext,
ethProtocolManager.ethContext(),
syncState,
pendingBlocks);
pendingBlocks,
metricsSystem);

final BlockDataGenerator gen = new BlockDataGenerator();
// Import some blocks
Expand Down
4 changes: 2 additions & 2 deletions ...2p/src/test/java/tech/pegasys/pantheon/ethereum/p2p/discovery/PeerDiscoveryAgentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ public void blacklistIncompatiblePeerWhoIssuesDisconnect() throws Exception {
@Test
public void shouldBeActiveWhenConfigIsTrue() {
final DiscoveryConfiguration config = new DiscoveryConfiguration();
config.setActive(true);
config.setActive(true).setBindPort(0);

final PeerDiscoveryAgent agent = startDiscoveryAgent(config, new PeerBlacklist());

Expand All @@ -305,7 +305,7 @@ public void shouldBeActiveWhenConfigIsTrue() {
@Test
public void shouldNotBeActiveWhenConfigIsFalse() {
final DiscoveryConfiguration config = new DiscoveryConfiguration();
config.setActive(false);
config.setActive(false).setBindPort(0);

final PeerDiscoveryAgent agent = startDiscoveryAgent(config, new PeerBlacklist());

Expand Down
3 changes: 2 additions & 1 deletion metrics/src/main/java/tech/pegasys/pantheon/metrics/MetricCategory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ public enum MetricCategory {
PEERS("peers"),
RPC("rpc"),
JVM("jvm", false),
PROCESS("process", false);
PROCESS("process", false),
BLOCKCHAIN("blockchain");

private final String name;
private final boolean pantheonSpecific;
Expand Down
3 changes: 2 additions & 1 deletion metrics/src/main/java/tech/pegasys/pantheon/metrics/OperationTimer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ public interface OperationTimer {
TimingContext startTimer();

interface TimingContext extends Closeable {
void stopTimer();
/** @return Elapsed time in seconds. */
double stopTimer();

@Override
default void close() {
Expand Down
2 changes: 1 addition & 1 deletion metrics/src/main/java/tech/pegasys/pantheon/metrics/noop/NoOpMetricsSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
public class NoOpMetricsSystem implements MetricsSystem {

private static final Counter NO_OP_COUNTER = new NoOpCounter();
private static final TimingContext NO_OP_TIMING_CONTEXT = () -> {};
private static final TimingContext NO_OP_TIMING_CONTEXT = () -> 0;
private static final OperationTimer NO_OP_TIMER = () -> NO_OP_TIMING_CONTEXT;

@Override
Expand Down
8 changes: 6 additions & 2 deletions pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import tech.pegasys.pantheon.ethereum.p2p.wire.Capability;
import tech.pegasys.pantheon.ethereum.p2p.wire.SubProtocol;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.metrics.prometheus.PrometheusMetricsSystem;
import tech.pegasys.pantheon.util.bytes.BytesValue;

import java.nio.file.Path;
Expand All @@ -78,6 +77,7 @@ public class RunnerBuilder {
private WebSocketConfiguration webSocketConfiguration;
private Path dataDir;
private Collection<String> bannedNodeIds;
private MetricsSystem metricsSystem;

public RunnerBuilder vertx(final Vertx vertx) {
this.vertx = vertx;
Expand Down Expand Up @@ -134,11 +134,15 @@ public RunnerBuilder bannedNodeIds(final Collection<String> bannedNodeIds) {
return this;
}

public RunnerBuilder metricsSystem(final MetricsSystem metricsSystem) {
this.metricsSystem = metricsSystem;
return this;
}

public Runner build() {

Preconditions.checkNotNull(pantheonController);

final MetricsSystem metricsSystem = PrometheusMetricsSystem.init();
final DiscoveryConfiguration discoveryConfiguration;
if (discovery) {
final Collection<?> bootstrap;
Expand Down
Loading