feat: summary peer availability tracing #127

m-schmoock · 2020-07-20T14:58:42Z

Adds:

peer availability tracing, to show the node operator which peers are reliable.
72hr exponential smoothing window

Fixes:

fix broken fiat price API by switching to public (and reliable) bitstamp ticker API.
fix PriceThread actually perform a loop instead of fetching price only one time

codecov · 2020-07-20T15:17:38Z

Codecov Report

Merging #127 into master will increase coverage by 0.54%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##           master     #127      +/-   ##
==========================================
+ Coverage   57.32%   57.87%   +0.54%     
==========================================
  Files          22       23       +1     
  Lines        2580     2611      +31     
==========================================
+ Hits         1479     1511      +32     
+ Misses       1101     1100       -1

Impacted Files	Coverage Δ
summary/summary.py	`87.05% <95.65%> (+1.52%)`	⬆️
summary/summary_avail.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ded4f6b...fab43a0. Read the comment docs.

niftynei

lgtm. codecov is complaining that the total test coverage has gone a bit :)

summary/summary.py

m-schmoock · 2020-07-23T11:09:00Z

I think I can also rearrange code to make the calculation tetstable so codecov is more happy with me

m-schmoock · 2020-07-24T23:34:40Z

Okay, I fixed the code coverage, even though it was just related to seemingly not profiling unittest thread itself.
By adding a test with a low summary-availability-interval value we get the PeerThread to execute quick enough, and this thread is audited by codecov.

@cdecker : maybe we can reconfigure codecov also tracing the actual unittest threads instead of only the plugin threads within the daemon?

niftynei · 2020-07-27T17:49:39Z

summary/summary.py

+plugin.add_option(
+    'summary-availability-interval',
+    300,
+    'How often in seconds the availability should be calculated. Causes RPC and CPU.'


The meaning of Causes RPC and CPU isn't totally clear to me. May be clearer as "uses" or "causes usage"?

If your okay, I simply removed that silly addition. I thought it would be useful to tell the user that this is just local RPC and doesn't consume much resources or network stuff. But it doesn't add anything particular interesting.

Solved, rebased an force-pushed.

summary/summary.py

niftynei

lgtm!

summary/test_summary.py

summary/summary.py

cdecker

Excellent addition, I look forward to knowing which peers of mine are lazy and should be disconnected :-)

I went through with a fine-toothed comb, and added a couple of nits, nothing serious, so feel free to ignore them, just thought that since I'm this later to review I should at least put in that much effort.

ACK 2a0d272

summary/summary.py

cdecker · 2020-08-01T10:01:50Z

summary/summary.py

@@ -2,7 +2,7 @@
 from pyln.client import Plugin, Millisatoshi
 from packaging import version
 from collections import namedtuple
-from datetime import datetime
+from summary_avail import *


If you're concerned that the path might not be unique you can explicitly import a local module by using the following:

from .availability import *

This saves you from having to prefix everything. Alternatively you could also have the executable in the top-level and create a summary package containing all the sub-modules.

Wont do, since plugin API or autoreload plugin gets me this:

from .availability import * ImportError: attempted relative import with no known parent package

cdecker · 2020-08-01T10:08:16Z

Regarding codecov, I'm really tempted to just remove it. It hasn't produced anything actionable, and has been outright misleading in some cases (some plugins flip-flopping despite not being touched...)

It might just be our setup, with tests running as unit-tests and also collecting coverage when run as plugins.

1. bitcoinaverage does not supply public prices since a while now: > Unauthenticated requests are not allowed. Take out a new plan or start > a free trial at https://pro.bitcoinaverage.com" This commit fixes this by porting to bitstamp public ticker API. 2. This commit fixes the missing thread loop, so the price is actually updated after a while. 3. This commit fixes an unchecked attribute access that crashed during my tests.

this applies a 72hr exponential smoothed sliding window to the online/offline availability state of a peer.

using level error caused the testframework to fail on non-essential price or availability thread log messages. likely caused by the test runner not being able to query the bitstamp API.

extracts the availability calculations to own testable module

m-schmoock requested a review from cdecker July 20, 2020 15:00

m-schmoock force-pushed the feat/summary_availability branch 2 times, most recently from 41ebcf7 to fd2fd0e Compare July 20, 2020 17:24

m-schmoock changed the title ~~feat: summary availability 24hr window~~ feat: summary availability Jul 21, 2020

m-schmoock force-pushed the feat/summary_availability branch from a651d48 to 5ae24d7 Compare July 21, 2020 15:40

niftynei reviewed Jul 22, 2020

View reviewed changes

summary/summary.py Outdated Show resolved Hide resolved

summary/summary.py Outdated Show resolved Hide resolved

summary/summary.py Outdated Show resolved Hide resolved

summary/summary.py Outdated Show resolved Hide resolved

summary/summary.py Outdated Show resolved Hide resolved

m-schmoock force-pushed the feat/summary_availability branch from 5ae24d7 to ac3124f Compare July 23, 2020 09:13

m-schmoock changed the title ~~feat: summary availability~~ feat: summary peer availability tracing Jul 23, 2020

m-schmoock force-pushed the feat/summary_availability branch 5 times, most recently from 5add9c5 to dae4835 Compare July 24, 2020 23:18

m-schmoock requested a review from niftynei July 25, 2020 16:35

niftynei reviewed Jul 27, 2020

View reviewed changes

summary/summary.py Outdated Show resolved Hide resolved

niftynei approved these changes Jul 27, 2020

View reviewed changes

summary/test_summary.py Outdated Show resolved Hide resolved

summary/summary.py Show resolved Hide resolved

m-schmoock force-pushed the feat/summary_availability branch from efa191d to 2a0d272 Compare July 27, 2020 20:30

cdecker approved these changes Aug 1, 2020

View reviewed changes

m-schmoock added 7 commits August 1, 2020 13:17

summary: adds 72hr availability measurement

c200cd1

this applies a 72hr exponential smoothed sliding window to the online/offline availability state of a peer.

summary: set thread loglevel to just warn

5100478

using level error caused the testframework to fail on non-essential price or availability thread log messages. likely caused by the test runner not being able to query the bitstamp API.

summary: moves availability code and adds testcases

6d6e798

extracts the availability calculations to own testable module

summary: increase test coverage

e87d613

summary: remove legacy code

c5dd7c1

fix: missing link in README

4a14533

summary: adds Channel named tuple

fab43a0

m-schmoock force-pushed the feat/summary_availability branch from 2a0d272 to fab43a0 Compare August 1, 2020 11:39

cdecker merged commit acf670b into lightningd:master Aug 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: summary peer availability tracing #127

feat: summary peer availability tracing #127

m-schmoock commented Jul 20, 2020 •

edited

codecov bot commented Jul 20, 2020 •

edited

niftynei left a comment

m-schmoock commented Jul 23, 2020

m-schmoock commented Jul 24, 2020 •

edited

niftynei Jul 27, 2020

m-schmoock Jul 27, 2020

niftynei left a comment

cdecker left a comment

cdecker Aug 1, 2020

m-schmoock Aug 1, 2020

cdecker commented Aug 1, 2020

feat: summary peer availability tracing #127

feat: summary peer availability tracing #127

Conversation

m-schmoock commented Jul 20, 2020 • edited

codecov bot commented Jul 20, 2020 • edited

Codecov Report

niftynei left a comment

Choose a reason for hiding this comment

m-schmoock commented Jul 23, 2020

m-schmoock commented Jul 24, 2020 • edited

niftynei Jul 27, 2020

Choose a reason for hiding this comment

m-schmoock Jul 27, 2020

Choose a reason for hiding this comment

niftynei left a comment

Choose a reason for hiding this comment

cdecker left a comment

Choose a reason for hiding this comment

cdecker Aug 1, 2020

Choose a reason for hiding this comment

m-schmoock Aug 1, 2020

Choose a reason for hiding this comment

cdecker commented Aug 1, 2020

m-schmoock commented Jul 20, 2020 •

edited

codecov bot commented Jul 20, 2020 •

edited

m-schmoock commented Jul 24, 2020 •

edited