keep-network / keep-core

The smart contracts and reference client behind the Keep network
https://keep.network
MIT License
118 stars 73 forks source link

Adding Ethereum latest block number diagnostic #3637

Closed dimpar closed 1 year ago

dimpar commented 1 year ago

Refs https://github.com/keep-network/keep-core/issues/3608

Exposing the latest block number diagnostic can be helpful to diagnose some of the problems with a node connectivity and determine if a node is catching up with the global Ethereum state.

Example output:

{
  "chain_info": {
    "latest_eth_block_number": 3525
  },
  "client_info": {
    "chain_address": "0x407190f04d838Ec47dad4C0e0D2a9c49d7737479",
    "network_id": "16Uiu2HAm4WrMWq9Gc7WsymsqgrwbaD1KeW2eH8eY63DTZHXbRsuE",
    "revision": "8c444b4b9",
    "version": "v2.0.0-m3-70-g8c444b4b9"
  },
  "connected_peers": null
}
pdyraga commented 1 year ago

General question: why is the Ethereum chain tip block number a metric and not a diagnostic?

Metrics are useful for time series, measurable, and changing over time. The number of nodes connected is a good metric. The connectivity status (0/1) to some external service like Ethereum or Electrum is a good metric. The number of pre-params in the pool is a good metric. The values can go up and down over time and they are measurable: N peers, N parameters, yes/no status.

I don't think chain time block number is a good metric, I would rather make it a diagnostic. It is always increasing and is not really measurable. We can think that "there are N blocks in the chain" but we are mostly interested in a non-measurable block number of the tip.

dimpar commented 1 year ago

General question: why is the Ethereum chain tip block number a metric and not a diagnostic? Metrics are useful for time series, measurable, and changing over time. The number of nodes connected is a good metric. The connectivity status (0/1) to some external service like Ethereum or Electrum is a good metric. The number of pre-params in the pool is a good metric. The values can go up and down over time and they are measurable: N peers, N parameters, yes/no status. I don't think chain time block number is a good metric, I would rather make it a diagnostic. It is always increasing and is not really measurable. We can think that "there are N blocks in the chain" but we are mostly interested in a non-measurable block number of the tip.

Agree with what you said. I put it under metrics because codewise it's very close to Ethereum connectivity status and required very minimal change. But I think that Eth connectivity also doesn't exactly fit under metrics then. The status is binary, 0 or 1, connection problems or not and I'd rather see it under client's diagnostics. It won't be changed like pre-params where the range is way bigger. We can certainly leave it alone cause most likely some API calls are already in place and use it. Just giving you my reasoning.

For the latest block number, I will move it to diagnostics.

pdyraga commented 1 year ago

Tested locally, works as advertised!

❯ curl -D - localhost:8081/diagnostics
HTTP/1.1 200 OK
Date: Tue, 20 Jun 2023 13:07:22 GMT
Content-Length: 323
Content-Type: text/plain; charset=utf-8

{
  "chain_info": {
    "latest_eth_block_number": 741
  },
  "client_info": {
    "chain_address": "0x146748a2b46B99ee1470b587bc9812Ea59b79597",
    "network_id": "16Uiu2HAm2GxAmGNcQ6Aq8GMEYsYCnuZqbWLqCKD5fGWciwvUMtoj",
    "revision": "0493b72b3",
    "version": "v2.0.0-m3-73-g0493b72b3"
  },
  "connected_peers": null
}