Opened 7 months ago

Closed 6 months ago

#25163 closed defect (implemented)

rephist: Remove unused counters in or_history_t

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-rephist, code-removal
Cc: karsten Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The rephist subsystem seems to track EXTEND cells attempt (on the client side) but the overall results of this is not used except in rep_hist_dump_stats(). This is the only use of link_history_t also afaict. See:

void rep_hist_note_extend_succeeded(const char *from_name,
                                    const char *to_name);
void rep_hist_note_extend_failed(const char *from_name, const char *to_name);

Furthermore, tor tracks downtime and uptime of relays but actually never use that information anywhere. See:

void rep_hist_note_connect_failed(const char* nickname, time_t when);
void rep_hist_note_connect_succeeded(const char* nickname, time_t when);
void rep_hist_note_disconnect(const char* nickname, time_t when);
void rep_hist_note_connection_died(const char* nickname, time_t when);

The side effect of this is that we keep adding objects to the history_map that are wasting memory and never used in the end except when we dump statistics.

By removing this, we would cleanup quite a bit of code and effectively make the or_history_t object *ONLY* useful to directory authorities for relay reachability tracking.

Child Tickets

Change History (9)

comment:1 Changed 7 months ago by dgoulet

Status: newneeds_review

This removes some stats from a SIGUSR1 signal but it also reduces considerably the size of the or history map in rephist.c

If you think we should keep this for the stats that it gives us, feel free to toss the branch out.

See branch: ticket25163_033_01

comment:2 Changed 7 months ago by nickm

Status: needs_reviewmerge_ready

I'm okay with these. I'd like others (arma? karsten?) to get a chance to sign off on them first, though.

comment:3 Changed 6 months ago by karsten

Cc: karsten added

comment:4 Changed 6 months ago by karsten

It looks like this change won't affect anything related to metrics. But I have no idea whether people would miss these stats from the SIGUSR1 signal. Maybe this can be useful information to debug something? Maybe ask on tor-relays@?

comment:5 Changed 6 months ago by dgoulet

Owner: set to dgoulet
Status: merge_readyaccepted

comment:6 Changed 6 months ago by dgoulet

Status: acceptedmerge_ready

comment:7 Changed 6 months ago by arma

i haven't looked at the patch, but dgoulet filed this "why not get this of this stuff" ticket after talking to me on irc, so i am ok with the concept.

comment:8 Changed 6 months ago by nickm

 8 files changed, 11 insertions(+), 331 deletions(-)

Merged to master!

comment:9 Changed 6 months ago by nickm

Resolution: implemented
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.