Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce new CRPCStats class to save key RPC statistics #970

Merged
merged 3 commits into from
May 21, 2022

Conversation

Jouzo
Copy link
Collaborator

@Jouzo Jouzo commented Dec 9, 2021

What kind of PR is this?:

/kind feature

What this PR does / why we need it:

Save RPC latency and payload size on each call.

Introduces 2 RPCs:

  • getrpcstats
  • listrpcsstats

getrpcstats :

defi-cli -regtest getrpcstats getblockcount
{
  "name": "getblockcount",
  "lastUsedTime": 1639041504,
  "latency": {
    "max": 1,
    "min": 0,
    "avg": 0
  },
  "payload": {
    "max": 37,
    "min": 37,
    "avg": 37
  },
  "count": 3,
  "history": [
    {
      "timestamp": 1639041311,
      "latency": 0,
      "payload": 37
    },
    {
      "timestamp": 1639041348,
      "latency": 1,
      "payload": 37
    },
    {
      "timestamp": 1639041504,
      "latency": 1,
      "payload": 37
    }
  ]
}
defi-cli -regtest listrpcstats
[
  {
    "name": "getblockcount",
    "lastUsedTime": 1639041504,
    "latency": {
      "max": 1,
      "min": 0,
      "avg": 0
    },
    "payload": {
      "max": 37,
      "min": 37,
      "avg": 37
    },
    "count": 3,
    "history": [
      {
        "timestamp": 1639041311,
        "latency": 0,
        "payload": 37
      },
      {
        "timestamp": 1639041348,
        "latency": 1,
        "payload": 37
      },
      {
        "timestamp": 1639041504,
        "latency": 1,
        "payload": 37
      }
    ]
  },
  {
    "name": "getrpcstats",
    "lastUsedTime": 1639041506,
    "latency": {
      "max": 0,
      "min": 0,
      "avg": 0
    },
    "payload": {
      "max": 329,
      "min": 167,
      "avg": 248
    },
    "count": 2,
    "history": [
      {
        "timestamp": 1639041487,
        "latency": 0,
        "payload": 167
      },
      {
        "timestamp": 1639041506,
        "latency": 0,
        "payload": 329
      }
    ]
  },
  {
    "name": "listmasternodes",
    "lastUsedTime": 1639041356,
    "latency": {
      "max": 1,
      "min": 1,
      "avg": 1
    },
    "payload": {
      "max": 4255,
      "min": 4255,
      "avg": 4255
    },
    "count": 1,
    "history": [
      {
        "timestamp": 1639041356,
        "latency": 1,
        "payload": 4255
      }
    ]
  },
  {
    "name": "listrpcstats",
    "lastUsedTime": 1639041536,
    "latency": {
      "max": 0,
      "min": 0,
      "avg": 0
    },
    "payload": {
      "max": 608,
      "min": 608,
      "avg": 608
    },
    "count": 1,
    "history": [
      {
        "timestamp": 1639041536,
        "latency": 0,
        "payload": 608
      }
    ]
  },
  {
    "name": "listunspent",
    "lastUsedTime": 1639041363,
    "latency": {
      "max": 3400,
      "min": 3400,
      "avg": 3400
    },
    "payload": {
      "max": 5313266,
      "min": 5313266,
      "avg": 5313266
    },
    "count": 1,
    "history": [
      {
        "timestamp": 1639041363,
        "latency": 3400,
        "payload": 5313266
      }
    ]
  }
]

@Jouzo Jouzo changed the title Introduce new CRPCStats class to save various RPC statistics Introduce new CRPCStats class to save key RPC statistics Dec 9, 2021
@Jouzo Jouzo force-pushed the feature/listrpcstats branch from a562c37 to e8d483e Compare December 9, 2021 21:41
@Jouzo Jouzo marked this pull request as ready for review December 13, 2021 15:01
@prasannavl prasannavl added this to the Planned milestone Dec 20, 2021
src/rpc/stats.cpp Outdated Show resolved Hide resolved
src/rpc/stats.cpp Outdated Show resolved Hide resolved
src/rpc/stats.cpp Outdated Show resolved Hide resolved
src/rpc/stats.cpp Outdated Show resolved Hide resolved
src/rpc/stats.cpp Outdated Show resolved Hide resolved
src/rpc/stats.cpp Outdated Show resolved Hide resolved
@bvbfan bvbfan self-requested a review December 29, 2021 13:08
bvbfan
bvbfan previously approved these changes Dec 29, 2021
@prasannavl
Copy link
Member

prasannavl commented Feb 2, 2022

Use something on the lines of

  RPCStatEntry {
    name: string,
    lastUsedTime: int64,
    latency: RangeStatEntry { min, max, avg },
    payload: RangeStatEntry,
    count: uint64,
    history: std::vector<StatHistoryEntry>
  }
  
  std::map<string(name), RPCStatEntry> so it's easier to look up. (or unordered_map which is lookup optimized hash table, but the map will be ready for nicer json output -- but not a hotspot so doesn't really matter). 

for the internal memory repr and serialize / deserliaze in one-go.

Currently, each UniValue is (which adds up quickly)

  UniValue::VType typ;
  std::string val;                       // numbers are stored as C++ strings
  std::vector<std::string> keys;
  std::vector<UniValue> values;

@Jouzo Jouzo changed the base branch from master to v3 February 3, 2022 09:41
@Jouzo Jouzo requested a review from fuxingloh as a code owner February 3, 2022 09:41
@prasannavl prasannavl modified the milestones: Planned, 2.6.2 Feb 23, 2022
@Jouzo Jouzo requested review from bvbfan and prasannavl March 1, 2022 21:21
@Jouzo Jouzo changed the base branch from v3 to master March 9, 2022 08:02
@Jouzo Jouzo changed the base branch from master to v3 March 9, 2022 08:16
@prasannavl prasannavl added 2.6.2 and removed 2.7.0 labels Mar 9, 2022
@Jouzo Jouzo force-pushed the feature/listrpcstats branch from 5195180 to 52f04df Compare March 9, 2022 10:28
@Jouzo Jouzo changed the base branch from v3 to master March 9, 2022 10:28
@bvbfan bvbfan requested review from bvbfan and removed request for monstrobishi March 18, 2022 14:27
bvbfan
bvbfan previously approved these changes Mar 18, 2022
@Jouzo Jouzo force-pushed the feature/listrpcstats branch 2 times, most recently from f8cf4e3 to 3257ece Compare April 1, 2022 20:55
@prasannavl prasannavl added 3.0.0 and removed 2.7.0 labels May 5, 2022
@prasannavl
Copy link
Member

prasannavl commented May 18, 2022

The reason this PR is still pending is that, map is a protected global resource. While part of it is accessed correctly, under the lock, there are places where the references that it returns are accessed freely, which again should be protected, since it does not return a copy of the structs, but rather the references to global structures. This will end up in races, and is dependent on the higher access patterns synchronising this and serialising access.

src/rpc/stats.cpp Show resolved Hide resolved
src/rpc/stats.cpp Show resolved Hide resolved
@Jouzo Jouzo force-pushed the feature/listrpcstats branch from 77cd761 to 7000675 Compare May 18, 2022 19:37
@bvbfan
Copy link
Contributor

bvbfan commented May 19, 2022

@Jouzo rebase to v3, revert your last changes, they are incorrect.

@prasannavl prasannavl added 3.0.1 and removed 3.0.0 labels May 19, 2022
@Jouzo Jouzo changed the base branch from master to v3 May 19, 2022 07:59
@Jouzo Jouzo force-pushed the feature/listrpcstats branch 2 times, most recently from 9d42c10 to 344566f Compare May 19, 2022 08:13
@prasannavl
Copy link
Member

prasannavl commented May 21, 2022

Based on #970 (comment), the shared resource usage is well covered as it is right now.

All usages are simple value types that are copied over on the stack as stated in the above. Let's make sure there are no usage patterns that cause dead lock and we can proceed to merge.

@Jouzo Jouzo force-pushed the feature/listrpcstats branch from 6fc8268 to 344566f Compare May 21, 2022 08:37
@prasannavl
Copy link
Member

Build failure was due to v3 branch. Should be good now.

@prasannavl prasannavl merged commit 0f691ea into v3 May 21, 2022
@prasannavl prasannavl deleted the feature/listrpcstats branch May 21, 2022 13:39
Jouzo added a commit that referenced this pull request May 22, 2022
* Add CRPCStats to log RPC usage

* Use CLockFreeMutex

* Copy map via getMap method
prasannavl pushed a commit that referenced this pull request May 22, 2022
* Introduce new CRPCStats class to save key RPC statistics (#970)

* Add CRPCStats to log RPC usage

* Use CLockFreeMutex

* Copy map via getMap method

* Use atomic_bool for CLockFreeGuard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants