#25700 closed enhancement (fixed)

Fetch descriptors from both CollecTor instances

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

Description

As discussed at the last team meeting, we should attempt to make Onionoo more robust by fetching descriptors from both CollecTor instances.

I started working on this by extending DescriptorSource and am currently trying it out. As soon as I have something to review, I'll post it here.

This change won't require a protocol change and can go into the next release.

Child Tickets

Attachments (1)

collector-downtimes.png (59.7 KB) - added by karsten 11 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 months ago by karsten

Status: assignedneeds_review

Changed 11 months ago by karsten

Attachment: collector-downtimes.png added

comment:2 Changed 11 months ago by karsten

Somewhat related, though mostly as a motivation why this ticket matters, here's a graph of temporary downtimes of the primary CollecTor host in the first quarter of 2018:


comment:3 Changed 11 months ago by iwakeh

Reviewer: iwakeh

comment:4 Changed 11 months ago by iwakeh

Just curious: what are the downtimes for collector2?

comment:5 Changed 11 months ago by karsten

Much lower. But I can't make a graph of that. I took the data for this graph from consensus tarballs by plotting time differences between consensus valid-after time and the file's last-modified time. We don't have a similar archive from collector2, AFAIK.

comment:6 Changed 11 months ago by iwakeh

Status: needs_reviewneeds_revision

Queues get added twice here.

When constructing recentDir the code is not necessarily platform independent, maybe use Paths.get.

As Onionooo should be made easier to configure soon and we decided on having one config file (in #24041) maybe the construction of directories and supplying the hosts could be put in one place? This would also avoid repeating the path concatenation. Something like a list of host URL and in directory?
(Just prepare and enable future config file use, not yet implementing it, of course.)

Last edited 11 months ago by iwakeh (previous) (diff)

comment:7 in reply to:  5 Changed 11 months ago by iwakeh

Replying to karsten:

Much lower. But I can't make a graph of that. I took the data for this graph from consensus tarballs by plotting time differences between consensus valid-after time and the file's last-modified time. We don't have a similar archive from collector2, AFAIK.

Ah, thanks! No, collector2 doesn't archive.

comment:8 Changed 11 months ago by karsten

I just made the first two changes, but while looking into the third change I discovered a potential bug: it might be that we're reading and parsing the entire set of recent descriptors 7 times and throwing away all but one descriptor type in each execution. I need to look closer at that, but I have to leave for lunch now.

comment:9 in reply to:  8 Changed 11 months ago by karsten

Status: needs_revisionneeds_review

Replying to karsten:

I just made the first two changes, but while looking into the third change I discovered a potential bug: it might be that we're reading and parsing the entire set of recent descriptors 7 times and throwing away all but one descriptor type in each execution. I need to look closer at that, but I have to leave for lunch now.

False alarm. I overlooked a line that prevents just that. Phew.

Alright, please find commit 5c96f5c in my task-25700 branch. I think that addresses all things you mentioned above. If not, please consider providing a fix, so that we can get this merged by end of today, if possible. I'm also currently running a local test.

comment:10 Changed 11 months ago by karsten

FWIW, local test succeeded.

comment:11 in reply to:  10 Changed 11 months ago by iwakeh

Status: needs_reviewmerge_ready

Replying to karsten:

FWIW, local test succeeded.

Great, thanks! This looks fine, and, will make introducing the configuration improvement easier.
Merge ready.

comment:12 Changed 11 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Great! Squashed and merged to master. Closing. Thanks!

Note: See TracTickets for help on using tickets.