chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version #2080
Conversation
vpavlin
requested review from
alrevuelta,
jm-clius and
Ivansete-status
and removed request for
alrevuelta
|
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
|
You can find the image built from this PR at Built from 6d55bc4 |
…tially concurrent, add version
vpavlin
force-pushed
the
chore/refactore-network-monitor
branch
from
7e88f24
to
7fd66c0
Compare
vpavlin
added
the
E:Define network and community metrics
| info "number of successful connections", amount=nOfOkConnections | ||
| info "number of successful connections", amount=successfulConnections | ||
|
|
||
| proc updateMetrics(allPeersRef: CustomPeersTableRef) {.gcsafe, raises: [KeyError].} = |
There was a problem hiding this comment.
nitpick, use Result instead of raising except?
There was a problem hiding this comment.
Well, I do not return anything, but Nim kept complaining about KeyError being potentially raised on line 206. This solved it. Couldn't google up any other solution:D
There was a problem hiding this comment.
yeap, the thing would be to catch that and return Result[void] error. No big deal since this is not part of the core codebase, but usually what we do in nwaku.
There was a problem hiding this comment.
Ok, I can add that, just still don't understad how it could throw that - feels like false positive
| "Histogram tracking ping durations for discovered peers", | ||
| buckets = [100.0, 200.0, 300.0, 400.0, 500.0, 600.0, 700.0, 800.0, 900.0, 1000.0, 2000.0, Inf] | ||
|
|
||
| declarePublicGauge networkmonitor_peer_count, | ||
| "Number of discovered peers", |
There was a problem hiding this comment.
is this "discovered"? based on the code i think its something like "attempted to connect"?
There was a problem hiding this comment.
So it track number of all discovered peers, but distinguishes between successful and unsuccessful connections based on connected label
…tially concurrent, add version (#2080) * chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version * add more metrics, refactor how most metrics are calculated * rework metrics table fillup * reset connErr to make sure we honour successful reconnection
* chore: add retention policy with GB or MB limitation #1885 * chore: add retention policy with GB or MB limitation * chore: updated code post review- retention policy * ci: extract discordNotify to separate file Signed-off-by: Jakub Sokołowski <jakub@status.im> * ci: push images to new wakuorg/nwaku repo Signed-off-by: Jakub Sokołowski <jakub@status.im> * ci: enforce default Docker image tags strictly Signed-off-by: Jakub Sokołowski <jakub@status.im> * ci: push GIT_REF if it looks like a version Signed-off-by: Jakub Sokołowski <jakub@status.im> * fix: update wakuv2 fleet DNS discovery enrtree https://github.com/status-im/infra-misc/issues/171 * chore: resolving DNS IP and publishing it when no extIp is provided (#2030) * feat(coverage): Add simple coverage (#2067) * Add test aggregator to all directories. * Implement coverage script. * fix(ci): fix name of discord notify method Also use absolute path to load Groovy script. Signed-off-by: Jakub Sokołowski <jakub@status.im> * chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version (#2080) * chore(networkmonitor): refactor setConnectedPeersMetrics, make it partially concurrent, add version * add more metrics, refactor how most metrics are calculated * rework metrics table fillup * reset connErr to make sure we honour successful reconnection * chore(cbindings): Adding cpp example that integrates the 'libwaku' (#2079) * Adding cpp example that integrates the `libwaku` --------- Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com> * fix(ci): update the dependency list in pre-release WF (#2088) * chore: adding NetConfig test suite (#2091) --------- Signed-off-by: Jakub Sokołowski <jakub@status.im> Co-authored-by: Jakub Sokołowski <jakub@status.im> Co-authored-by: Anton Iakimov <yakimant@gmail.com> Co-authored-by: gabrielmer <101006718+gabrielmer@users.noreply.github.com> Co-authored-by: Álex Cabeza Romero <alex93cabeza@gmail.com> Co-authored-by: Vaclav Pavlin <vaclav@status.im> Co-authored-by: Ivan Folgueira Bande <128452529+Ivansete-status@users.noreply.github.com> Co-authored-by: NagyZoltanPeter <113987313+NagyZoltanPeter@users.noreply.github.com>
Description
This PR refactors
setConnectedPeersMetricsproc a bitshouldReconnectcheckResetRetriesAftertime (in seconds) to retry peers we failed to connect (if they are re-discovered)It also adds
--version(cc @jakubgs)Changes
shouldReconnectanalyzePeer--versionIssue
partially based on comments from #2068