Features/add initial subnet metrics #233
Conversation
cryptoAtwill
changed the title
Co-authored-by: adlrocha <[email protected]>
|
We are making good progress. Thanks! I would try the event listener approach as part of this PR now that you know how to collect metrics. I think it can give all the skeleton that we need to collect "real-time" metrics. |
Co-authored-by: adlrocha <[email protected]>
…yptoAtwill/eudico into features/add_subnet_count_metric
cryptoAtwill
changed the title
Co-authored-by: adlrocha <[email protected]>
…yptoAtwill/eudico into features/add_subnet_count_metric
| Using docker-compose | ||
| In the root directory | ||
| ```shell | ||
| docker build -t eudico:latest -f Dockerfile.eudico . |
There was a problem hiding this comment.
Nice!!! Have you tried to spawn a network with more than one node with Docker compose?
There was a problem hiding this comment.
I will give it a try
cmd/eudico-stats/entity.go
Outdated
| } | ||
|
|
||
| func (s *SubnetNode) Equals(o *SubnetNode) bool { | ||
| return s.SubnetID == o.SubnetID && |
There was a problem hiding this comment.
Why not doing s==o directly?
There was a problem hiding this comment.
this does a pointer compare? not the deep compare using pointers?
| // The nextEpoch to resync stats with the chain | ||
| nextEpoch abi.ChainEpoch | ||
| // The list of subnets currently managed by the current subnet | ||
| subnets map[address.SubnetID]bool |
There was a problem hiding this comment.
I don't get this variable. Aren´t you handling already the list of subnets being managed in EudicoStatsListener?
There was a problem hiding this comment.
This one tracks the children in memory.
There was a problem hiding this comment.
the list of subnets are for the nodes. This one is to maintain the list of child subnets. Maybe I should give it a differernt name.
cmd/eudico-stats/listen.go
Outdated
| ctx context.Context, | ||
| id address.SubnetID, | ||
| ) error { | ||
| // TODO: ideally there is no need to add read write lock. Keep in view. |
There was a problem hiding this comment.
You may need a lock here, what if the listener accesses the map because you start syncing with a new subnet while you are reading from it to collect some metrics? I'd suggest running with race to see if this is the case.
| // FIXME: Consider moving this to an independent optional process | ||
| // (so that is not dependent on the main process). | ||
| if cctx.Bool("export-metrics") { | ||
| go func() { |
There was a problem hiding this comment.
Is this still needed? eudico-statsnow runs in its own process, right?
There was a problem hiding this comment.
There might be metrics that cannot be collected by eudico-stats, such as response time and stuff like that?
scripts/eudico.sh
Outdated
| @@ -0,0 +1,81 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
What about renaming this script eudico-stats or eudico-stats-example?
Proposed Changes
Add initial metrics to track:
Sample dashboard:
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, testarea: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps