Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19035 closed enhancement (implemented)

Rip out client directory fetch instrumentation

Reported by: arma Owned by: arma
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 029-nickm-says-yes, review-group-2, TorCoreTeam201605
Cc: weasel, atagar, dgoulet Actual Points:
Parent ID: Points: 0.1 remaining
Reviewer: dgoulet Sponsor:

Description

In the distant past, weasel put in a feature where clients would track number of bytes they receive by what category of directory information, and then you could query the current sums via the dirport ("GET /tor/bytes.txt"). It went into 0.2.1.1-alpha I think (see git commits 01c1a355 and 716558a), but it didn't get a changelog entry or a trac ticket, and also it only works #if defined(INSTRUMENT_DOWNLOADS) || defined(RUNNING_DOXYGEN).

I don't think anybody is using it now, and nobody in the Montreal Meeting wanted to keep it, so it looks like we should simplify by removing it.

I am cc'ing weasel since it was his feature originally, and now he can speak up in favor of its life if he wants.

Child Tickets

Change History (24)

comment:1 Changed 3 years ago by arma

Turns out there's a control-spec way of requesting this data too:

    "dir-usage"
      A newline-separated list of how many bytes we've served to answer
      each type of directory request. The format of each line is:
         Keyword 1*SP Integer 1*SP Integer
      where the first integer is the number of bytes written, and the second
      is the number of requests answered.

But that said, it simply returns "Not implemented" unless your Tor has the compile-time option enabled. So I think nobody is going to miss this option.

comment:2 Changed 3 years ago by arma

My task19035 branch removes these features.

comment:3 Changed 3 years ago by arma

Keywords: 029-proposed added
Status: newneeds_review

Also see my task19035-spec branch in my torspec.

comment:4 Changed 3 years ago by arma

Cc: atagar dgoulet added

Damian: will we break anything in your stem-land with this change?

comment:5 Changed 3 years ago by weasel

I don't mind having it removed.

comment:6 Changed 3 years ago by atagar

Nothing that's been mentioned ('GET /tor/bytes.txt' or 'GETINFO dir-usage') is ringing any bells. Sounds fine to remove to me.

comment:7 Changed 3 years ago by nickm

Keywords: 029-nickm-says-yes added

comment:8 Changed 3 years ago by nickm

Points: 0.1 remaining

comment:9 Changed 3 years ago by nickm

Keywords: review-group-2 added

Create a review-group-2 from (most of the) tickets in 0.2.8 or 0.2.9 or 029-nickm-says-yes listed as needs_review,

comment:10 Changed 3 years ago by nickm

Owner: set to arma
Status: needs_reviewassigned

setting owner

comment:11 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:12 Changed 3 years ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

Calling these included in 0.2.9 because the (remaining) amount of work should be very small.

comment:13 Changed 3 years ago by arma

Keywords: TorCoreTeam201605 added

comment:14 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

This is failing to build:

src/or/directory.c: In function ‘handle_get_frontpage’:
src/or/directory.c:2875:72: error: unused parameter ‘args’ [-Werror=unused-parameter]
 handle_get_frontpage(dir_connection_t *conn, const get_handler_args_t *args)

Adding (void) args; at the beginning of the function seems to work well for me.

Some reference still exists that we could probably get rid off:

./orconfig.h +536 /* #undef INSTRUMENT_DOWNLOADS */
./src/or/or.h +20 #ifndef INSTRUMENT_DOWNLOADS
./src/or/or.h +21 #define INSTRUMENT_DOWNLOADS 1

The rest lgtm;

comment:15 Changed 3 years ago by arma

Status: needs_revisionneeds_review

Awesome. I have pushed some fixup! commits to my task19035 branch, along with a whitespace fix.

There is now an XXX029 question for Nick that I'd like his opinion on.

And there's a task19035-fixedup branch that is the one we'd want to merge.

comment:16 Changed 3 years ago by nickm

+/* XXX029 Nothing left here? Take out or leave in? */

Take it out, says I.

comment:17 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

With that XXX029 removed, it's ready imo. Builds and test passes.

Putting that one in merge_ready.

comment:18 Changed 3 years ago by cypherpunks

Also remove the configuration option because it does nothing now.

diff --git a/configure.ac b/configure.ac
index d7db0db..b5e4b2b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -33,8 +33,6 @@ CPPFLAGS="$CPPFLAGS -I\${top_srcdir}/src/common"
 #XXXX020 We should make these enabled or not, before 0.2.0.x-final
 AC_ARG_ENABLE(openbsd-malloc,
    AS_HELP_STRING(--enable-openbsd-malloc, [use malloc code from OpenBSD.  Linux only]))
-AC_ARG_ENABLE(instrument-downloads,
-   AS_HELP_STRING(--enable-instrument-downloads, [instrument downloads of directory resources etc.]))
 AC_ARG_ENABLE(static-openssl,
    AS_HELP_STRING(--enable-static-openssl, [link against a static openssl library. Requires --with-openssl-dir]))
 AC_ARG_ENABLE(static-libevent,

comment:19 Changed 3 years ago by cypherpunks

Status: merge_readyneeds_revision

comment:20 Changed 3 years ago by arma

Status: needs_revisionmerge_ready

There are now two more commits on task19035-fixedup.

comment:21 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged!

comment:22 in reply to:  3 Changed 3 years ago by arma

Replying to arma:

Also see my task19035-spec branch in my torspec.

I just pushed this branch to torspec. (Surely all the reviewers reviewed it and loved it, and Nick just skipped over it when closing the ticket. Right? :)

comment:23 Changed 3 years ago by nickm

spec branch merged.

(I had a joke here, but I decided that the joke would be confusing, and deleted it.)

comment:24 Changed 3 years ago by U+039b

I filed a ticket on FreeBSD bugzilla: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=210389

Note: See TracTickets for help on using tickets.