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

[PIE-1580] Metrics for smart contract permissioning actions #1492

Merged
merged 11 commits into from
May 26, 2019
Merged

[PIE-1580] Metrics for smart contract permissioning actions #1492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
915b5d5
Wired the metrics system through
Errorific May 21, 2019
6d3e984
node smart contract metrics
Errorific May 22, 2019
9a7f7ff
New functionality needs the metrics system
Errorific May 23, 2019
e1f7fb0
metrics test for node smart contract permissioning
Errorific May 23, 2019
81ae096
Tests for the node permissioning
Errorific May 23, 2019
455c1fb
Transaction smart contract metrics tests
Errorific May 23, 2019
a57be76
spotless
Errorific May 23, 2019
17e22ba
Trying to make errorprone comfortable
Errorific May 23, 2019
8a429f5
lets ignore the casting
Errorific May 23, 2019
c2ef587
Javadoc fixes
Errorific May 24, 2019
c157d15
Merge branch 'master' into feature/PIE-1580.Permissioning_metrics
Errorific May 26, 2019
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
1 change: 1 addition & 0 deletions ethereum/permissioning/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ dependencies {
implementation project(':util')
implementation project(':ethereum:core')
implementation project(':crypto')
implementation project(':metrics:core')

implementation 'com.google.guava:guava'
implementation 'net.consensys.cava:cava-toml'
Expand Down
10 changes: 7 additions & 3 deletions ...java/tech/pegasys/pantheon/ethereum/permissioning/NodePermissioningControllerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningProvider;
import tech.pegasys.pantheon.ethereum.permissioning.node.provider.SyncStatusNodePermissioningProvider;
import tech.pegasys.pantheon.ethereum.transaction.TransactionSimulator;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.util.bytes.BytesValue;
import tech.pegasys.pantheon.util.enode.EnodeURL;

Expand All @@ -32,7 +33,8 @@ public NodePermissioningController create(
final Synchronizer synchronizer,
final Collection<EnodeURL> fixedNodes,
final BytesValue localNodeId,
final TransactionSimulator transactionSimulator) {
final TransactionSimulator transactionSimulator,
final MetricsSystem metricsSystem) {

Optional<SyncStatusNodePermissioningProvider> syncStatusProviderOptional;

Expand All @@ -55,15 +57,17 @@ public NodePermissioningController create(
NodeSmartContractPermissioningController smartContractProvider =
new NodeSmartContractPermissioningController(
smartContractPermissioningConfiguration.getNodeSmartContractAddress(),
transactionSimulator);
transactionSimulator,
metricsSystem);
providers.add(smartContractProvider);
}

if (fixedNodes.isEmpty()) {
syncStatusProviderOptional = Optional.empty();
} else {
syncStatusProviderOptional =
Optional.of(new SyncStatusNodePermissioningProvider(synchronizer, fixedNodes));
Optional.of(
new SyncStatusNodePermissioningProvider(synchronizer, fixedNodes, metricsSystem));
}
} else {
syncStatusProviderOptional = Optional.empty();
Expand Down
36 changes: 34 additions & 2 deletions ...ech/pegasys/pantheon/ethereum/permissioning/NodeSmartContractPermissioningController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
import tech.pegasys.pantheon.ethereum.transaction.CallParameter;
import tech.pegasys.pantheon.ethereum.transaction.TransactionSimulator;
import tech.pegasys.pantheon.ethereum.transaction.TransactionSimulatorResult;
import tech.pegasys.pantheon.metrics.Counter;
import tech.pegasys.pantheon.metrics.MetricCategory;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.util.bytes.BytesValue;
import tech.pegasys.pantheon.util.bytes.BytesValues;
import tech.pegasys.pantheon.util.enode.EnodeURL;
Expand All @@ -40,6 +43,9 @@ public class NodeSmartContractPermissioningController implements NodePermissioni
"connectionAllowed(bytes32,bytes32,bytes16,uint16,bytes32,bytes32,bytes16,uint16)";
// hashed function signature for connection allowed call
private static final BytesValue FUNCTION_SIGNATURE_HASH = hashSignature(FUNCTION_SIGNATURE);
private final Counter checkCounter;
private final Counter checkCounterPermitted;
private final Counter checkCounterUnpermitted;

// The first 4 bytes of the hash of the full textual signature of the function is used in
// contract calls to determine the function being called
Expand All @@ -60,11 +66,30 @@ private static BytesValue hashSignature(final String signature) {
*
* @param contractAddress The address at which the permissioning smart contract resides
* @param transactionSimulator A transaction simulator with attached blockchain and world state
* @param metricsSystem The metrics provider that is to be reported to
*/
public NodeSmartContractPermissioningController(
final Address contractAddress, final TransactionSimulator transactionSimulator) {
final Address contractAddress,
final TransactionSimulator transactionSimulator,
final MetricsSystem metricsSystem) {
this.contractAddress = contractAddress;
this.transactionSimulator = transactionSimulator;

this.checkCounter =
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have the counter for permitted and unpermitted, do we really need the total counter or can we just infer from the other two metrics?
I'm not aware of the impact of the metrics in system resources so maybe we are better playing safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's an outcome where the permissioning check crashes (no contract, bad transaction etc) which increments the total counter but not the other 2. I'm assuming the metrics are no heavier than adding log lines so I decided to be distinct about all the things we might be checking.

metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"node_smart_contract_check_count",
"Number of times the node smart contract permissioning provider has been checked");
this.checkCounterPermitted =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"node_smart_contract_check_count_permitted",
"Number of times the node smart contract permissioning provider has been checked and returned permitted");
this.checkCounterUnpermitted =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"node_smart_contract_check_count_unpermitted",
"Number of times the node smart contract permissioning provider has been checked and returned unpermitted");
}

/**
Expand All @@ -76,6 +101,7 @@ public NodeSmartContractPermissioningController(
*/
@Override
public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinationEnode) {
this.checkCounter.inc();
final BytesValue payload = createPayload(sourceEnode, destinationEnode);
final CallParameter callParams =
new CallParameter(null, contractAddress, -1, null, null, payload);
Expand All @@ -101,7 +127,13 @@ public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio
}
}

return result.map(r -> checkTransactionResult(r.getOutput())).orElse(false);
if (result.map(r -> checkTransactionResult(r.getOutput())).orElse(false)) {
this.checkCounterPermitted.inc();
return true;
} else {
this.checkCounterUnpermitted.inc();
return false;
}
}

// Checks the returned bytes from the permissioning contract call to see if it's a value we
Expand Down
36 changes: 34 additions & 2 deletions ...asys/pantheon/ethereum/permissioning/TransactionSmartContractPermissioningController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import tech.pegasys.pantheon.ethereum.transaction.CallParameter;
import tech.pegasys.pantheon.ethereum.transaction.TransactionSimulator;
import tech.pegasys.pantheon.ethereum.transaction.TransactionSimulatorResult;
import tech.pegasys.pantheon.metrics.Counter;
import tech.pegasys.pantheon.metrics.MetricCategory;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.util.bytes.BytesValue;
import tech.pegasys.pantheon.util.bytes.BytesValues;

Expand All @@ -41,6 +44,9 @@ public class TransactionSmartContractPermissioningController
"transactionAllowed(address,address,uint256,uint256,uint256,bytes)";
// hashed function signature for connection allowed call
private static final BytesValue FUNCTION_SIGNATURE_HASH = hashSignature(FUNCTION_SIGNATURE);
private final Counter checkCounterPermitted;
private final Counter checkCounter;
private final Counter checkCounterUnpermitted;

// The first 4 bytes of the hash of the full textual signature of the function is used in
// contract calls to determine the function being called
Expand All @@ -61,11 +67,30 @@ private static BytesValue hashSignature(final String signature) {
*
* @param contractAddress The address at which the permissioning smart contract resides
* @param transactionSimulator A transaction simulator with attached blockchain and world state
* @param metricsSystem The metrics provider that is to be reported to
*/
public TransactionSmartContractPermissioningController(
final Address contractAddress, final TransactionSimulator transactionSimulator) {
final Address contractAddress,
final TransactionSimulator transactionSimulator,
final MetricsSystem metricsSystem) {
this.contractAddress = contractAddress;
this.transactionSimulator = transactionSimulator;

this.checkCounter =
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing about the total checks counter.

metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"transaction_smart_contract_check_count",
"Number of times the transaction smart contract permissioning provider has been checked");
this.checkCounterPermitted =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"transaction_smart_contract_check_count_permitted",
"Number of times the transaction smart contract permissioning provider has been checked and returned permitted");
this.checkCounterUnpermitted =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"transaction_smart_contract_check_count_unpermitted",
"Number of times the transaction smart contract permissioning provider has been checked and returned unpermitted");
}

/**
Expand All @@ -76,6 +101,7 @@ public TransactionSmartContractPermissioningController(
*/
@Override
public boolean isPermitted(final Transaction transaction) {
this.checkCounter.inc();
final BytesValue payload = createPayload(transaction);
final CallParameter callParams =
new CallParameter(null, contractAddress, -1, null, null, payload);
Expand Down Expand Up @@ -103,7 +129,13 @@ public boolean isPermitted(final Transaction transaction) {
}
}

return result.map(r -> checkTransactionResult(r.getOutput())).orElse(false);
if (result.map(r -> checkTransactionResult(r.getOutput())).orElse(false)) {
this.checkCounterPermitted.inc();
return true;
} else {
this.checkCounterUnpermitted.inc();
return false;
}
}

// Checks the returned bytes from the permissioning contract call to see if it's a value we
Expand Down
40 changes: 38 additions & 2 deletions ...ys/pantheon/ethereum/permissioning/node/provider/SyncStatusNodePermissioningProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
import tech.pegasys.pantheon.ethereum.core.SyncStatus;
import tech.pegasys.pantheon.ethereum.core.Synchronizer;
import tech.pegasys.pantheon.ethereum.permissioning.node.NodePermissioningProvider;
import tech.pegasys.pantheon.metrics.Counter;
import tech.pegasys.pantheon.metrics.MetricCategory;
import tech.pegasys.pantheon.metrics.MetricsSystem;
import tech.pegasys.pantheon.util.enode.EnodeURL;

import java.util.Collection;
Expand All @@ -27,16 +30,42 @@ public class SyncStatusNodePermissioningProvider implements NodePermissioningPro

private final Synchronizer synchronizer;
private final Collection<EnodeURL> fixedNodes = new HashSet<>();
private final Counter checkCounter;
private final Counter checkCounterPermitted;
private final Counter checkCounterUnpermitted;
private OptionalLong syncStatusObserverId;
private boolean hasReachedSync = false;

public SyncStatusNodePermissioningProvider(
final Synchronizer synchronizer, final Collection<EnodeURL> fixedNodes) {
final Synchronizer synchronizer,
final Collection<EnodeURL> fixedNodes,
final MetricsSystem metricsSystem) {
checkNotNull(synchronizer);
this.synchronizer = synchronizer;
long id = this.synchronizer.observeSyncStatus(this::handleSyncStatusUpdate);
this.syncStatusObserverId = OptionalLong.of(id);
this.fixedNodes.addAll(fixedNodes);

metricsSystem.createIntegerGauge(
MetricCategory.PERMISSIONING,
"sync_status_node_sync_reached",
"Whether the sync status permissioning provider has realised sync yet",
() -> hasReachedSync ? 1 : 0);
this.checkCounter =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"sync_status_node_check_count",
"Number of times the sync status permissioning provider has been checked");
this.checkCounterPermitted =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"sync_status_node_check_count_permitted",
"Number of times the sync status permissioning provider has been checked and returned permitted");
this.checkCounterUnpermitted =
metricsSystem.createCounter(
MetricCategory.PERMISSIONING,
"sync_status_node_check_count_unpermitted",
"Number of times the sync status permissioning provider has been checked and returned unpermitted");
}

private void handleSyncStatusUpdate(final SyncStatus syncStatus) {
Expand Down Expand Up @@ -73,7 +102,14 @@ public boolean isPermitted(final EnodeURL sourceEnode, final EnodeURL destinatio
if (hasReachedSync) {
return true;
} else {
return fixedNodes.contains(destinationEnode);
checkCounter.inc();
if (fixedNodes.contains(destinationEnode)) {
checkCounterPermitted.inc();
return true;
} else {
checkCounterUnpermitted.inc();
return false;
}
}
}

Expand Down
Loading