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 1 commit
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
11 changes: 8 additions & 3 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.Builder;
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 @@ -49,11 +51,12 @@ public void startNode(final PantheonNode node) {
nodeExecutor = Executors.newCachedThreadPool();
}

final MetricsSystem noOpMetricsSystem = new NoOpMetricsSystem();
final PantheonControllerBuilder builder = new PantheonControllerBuilder();
final EthNetworkConfig ethNetworkConfig =
node.ethNetworkConfig()
.orElse(new EthNetworkConfig.Builder(mainnet()).setNetworkId(NETWORK_ID).build());
PantheonController<?> pantheonController;
final PantheonController<?> pantheonController;
try {
pantheonController =
builder.build(
Expand All @@ -63,7 +66,8 @@ public void startNode(final PantheonNode node) {
false,
node.getMiningParameters(),
node.isDevMode(),
KeyPairUtil.getDefaultKeyFile(node.homeDirectory()));
KeyPairUtil.getDefaultKeyFile(node.homeDirectory()),
noOpMetricsSystem);
} catch (final IOException e) {
throw new RuntimeException("Error building PantheonController", e);
}
Expand All @@ -81,7 +85,8 @@ public void startNode(final PantheonNode node) {
node.jsonRpcConfiguration(),
node.webSocketConfiguration(),
node.homeDirectory(),
Collections.emptySet());
Collections.emptySet(),
noOpMetricsSystem);

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
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
10 changes: 9 additions & 1 deletion metrics/src/main/java/tech/pegasys/pantheon/metrics/prometheus/PrometheusMetricsSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.function.Supplier;
import java.util.stream.Stream;

import com.google.common.base.Suppliers;
import io.prometheus.client.Collector;
import io.prometheus.client.Collector.MetricFamilySamples;
import io.prometheus.client.Collector.MetricFamilySamples.Sample;
Expand All @@ -48,9 +49,16 @@ public class PrometheusMetricsSystem implements MetricsSystem {
private static final String PANTHEON_PREFIX = "pantheon_";
private final Map<MetricCategory, Collection<Collector>> collectors = new ConcurrentHashMap<>();

private static final Supplier<MetricsSystem> INSTANCE =
Suppliers.memoize(PrometheusMetricsSystem::init);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong as it makes the metrics system JVM-wide whereas we want to be able to run two separate Pantheon instances in the same JVM and have them entirely isolated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My Prometheus Server code introduced the test breakage. So this is reverted.


PrometheusMetricsSystem() {}

public static MetricsSystem init() {
public static MetricsSystem instance() {
return INSTANCE.get();
}

private static MetricsSystem init() {
final PrometheusMetricsSystem metricsSystem = new PrometheusMetricsSystem();
metricsSystem.collectors.put(MetricCategory.PROCESS, singleton(new StandardExports()));
metricsSystem.collectors.put(
Expand Down
5 changes: 2 additions & 3 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,11 +77,11 @@ public Runner build(
final JsonRpcConfiguration jsonRpcConfiguration,
final WebSocketConfiguration webSocketConfiguration,
final Path dataDir,
final Collection<String> bannedNodeIds) {
final Collection<String> bannedNodeIds,
final MetricsSystem metricsSystem) {

Preconditions.checkNotNull(pantheonController);

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