Opened 3 years ago

Closed 3 years ago

#21047 closed defect (fixed)

atlas plotting bandwidth use from 2019

Reported by: arma Owned by: metrics-team
Priority: Medium Milestone: Onionoo 3.2-1.1.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


I see some of the graphs have data out to 2019.

Apparently onionoo has data to back it up (albeit mostly nulls):

Something here is not right. :)

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by cypherpunks

Last edited 3 years ago by cypherpunks (previous) (diff)

comment:2 Changed 3 years ago by karsten

Component: Metrics/AtlasMetrics/Onionoo
Keywords: metrics-help added
Owner: changed from irl to metrics-team

Good catch! This issue is caused by the relay reporting bandwidth statistics for 2019 and Onionoo not discarding excluding those from bandwidth docs until it's 2019.

I'd rather want us to fix this in Onionoo, not in Atlas and all other Onionoo clients.

Example descriptor 6/3/63ff123399bafc9b28d837ea0fa63f798c4a6a2a from this tarball:

@type extra-info 1.0
extra-info ibibUNC0 7C0AA4E3B73E407E9F5FEB1912F8BE26D8AA124D
-----BEGIN ED25519 CERT-----
-----END ED25519 CERT-----
published 2016-08-02 10:10:33
write-history 2019-08-07 05:31:45 (14400 s) 0,0,0,0,0,0
read-history 2019-08-07 05:31:45 (14400 s) 0,0,0,0,0,0
dirreq-write-history 2019-08-07 07:30:55 (14400 s) 0,0,0,0,0,0
dirreq-read-history 2019-08-07 07:30:55 (14400 s) 0,0,0,0,0,0
geoip-db-digest 6346E26E2BC96F8511588CE2695E9B0339A75D32
geoip6-db-digest 43CCB43DBC653D8CC16396A882C5F116A6004F0C
dirreq-stats-end 2016-08-01 18:06:45 (86400 s)
dirreq-v3-ips be=8,de=8,es=8,fr=8,us=8
dirreq-v3-reqs us=16,be=8,de=8,es=8,fr=8
dirreq-v3-resp ok=16,not-enough-sigs=0,unavailable=0,not-found=0,not-modified=0,busy=0
dirreq-v3-direct-dl complete=0,timeout=0,running=0
dirreq-v3-tunneled-dl complete=16,timeout=0,running=0,min=214075,d1=214075,d2=572799,q1=600118,d3=600118,d4=655941,md=1484660,d6=1494430,d7=1681836,q3=1683076,d8=1683076,d9=1708236,max=1920647
hidserv-stats-end 2016-08-01 18:06:45 (86400 s)
hidserv-rend-relayed-cells 739 delta_f=2048 epsilon=0.30 bin_size=1024
hidserv-dir-onions-seen 121 delta_f=8 epsilon=0.30 bin_size=8
router-sig-ed25519 BzxV6kj3qhd/Gr7yoTGc+ExAC81XIN27wZrOzZf+Gw2Wohkg8b3wd3FoB/K5tBDlEz2nqsR11RiS+Hmd0n1gDQ

With the analysis above I'd say that this ticket is a fine candidate for a new volunteer. Fixing this issue could start with writing a test that uses the descriptor above as input and produces a bandwidth document until 2019. The next step would be to fix the issue.

Last edited 3 years ago by karsten (previous) (diff)

comment:3 Changed 3 years ago by karsten

Status: newneeds_review

Please review my task-21047 branch with the trivial fix and a corresponding test.

comment:4 Changed 3 years ago by iwakeh

Looks fine and has a shiny new test!

I removed a checkstyle complaint from the test and changed it to use test data with a future date computed from the current time instead of having another dependency on Time and TimeFactory class. (Long ago we discussed that these are problematic and by reducing using them they might be obsolete at some point. Maybe, Time and TimeFactory should even be deprecated?)

Please find the patch here.

comment:5 Changed 3 years ago by karsten

Status: needs_reviewneeds_revision

Thanks for reviewing, and thanks for catching that checkstyle complaint!

However, there's a problem with the amended test: as time progresses, the four sample lines will be merged into a single data point in a "3_months" graph, and that graph won't be written anymore, because we require at least two consecutive data points in a graph. tl;dr: we'll have to calculate dates for the first four lines as well, ideally by picking yesterday or the day before. Should we do that and keep the Time dependency out, or should we just keep the test unchanged and only fix the checkstyle issue? I don't feel strongly.

Oh, one more thing to fix: status.setFromDocumentString(documentString); needs to stay, or the test won't succeed.


comment:6 Changed 3 years ago by iwakeh

Thanks for looking carefully!

Avoiding and finally removing these Time-changer classes is important.
I amended my branch with the suggested changes, and added a deprecation commit on top that will remind about this in future.

Please review.

comment:7 Changed 3 years ago by karsten

Keywords: metrics-help removed
Milestone: Onionoo 3.2-1.1.0
Status: needs_revisionmerge_ready

Looks good! Merged!

Leaving this ticket open until we have put out the next release and deployed the new version on the server. Otherwise we'll wonder why the bug is still around when in fact it's already fixed in the undeployed code.

comment:8 Changed 3 years ago by karsten

Resolution: fixed
Status: merge_readyclosed

New plan: closing this ticket now, because the fix is in master, but it'll take another week or so until the bugfix will be deployed.

Note: See TracTickets for help on using tickets.