[metrics-bugs] #22428 [Metrics/CollecTor]: Add webstats module

Tor Bug Tracker & Wiki blackhole at torproject.org
Mon Jan 22 15:37:56 UTC 2018


#22428: Add webstats module
-------------------------------+---------------------------------
 Reporter:  iwakeh             |          Owner:  iwakeh
     Type:  enhancement        |         Status:  needs_revision
 Priority:  High               |      Milestone:  CollecTor 1.5.0
Component:  Metrics/CollecTor  |        Version:
 Severity:  Normal             |     Resolution:
 Keywords:  metrics-2018       |  Actual Points:
Parent ID:                     |         Points:
 Reviewer:                     |        Sponsor:
-------------------------------+---------------------------------

Comment (by iwakeh):

 Replying to [comment:46 karsten]:
 > I just reviewed the last commits (086e904, d7a2835, 45dd764) altogether.
 Here's what I found:

 Thanks for the quick review!  No objections to the suggested changes and
 fine to start testing more.
 A few comments inline:

 >
 >  - We'll have to clean up `CHANGELOG.md` before this branch gets merged.
 It contains duplicate/overlapping entries for the new changes. I can do
 that as part of merging.

 +1

 >  - Let's also not forget to release metrics-lib 2.2.0 before merging
 this branch and updating to 2.2.0 rather than 2.2.0-dev. I can do that as
 part of merging, too.

 +1

 >  - New copyrights should be changed to 2018. I can do this, too.

 Thanks, I tried to catch all places, but didn't succeed.

 >  - Can I ask you to reference all member attributes or methods with
 `this.`? I know it's syntactically irrelevant for the compiler, but for me
 as a human being it improves readability a lot.

 Sure.  Do you intend to add the missing ones before testing or should I
 check and supply a branch?

 >  - There's a line in `SanitizeWeblogs.java` , namely
 `SortedSet<LocalDate> sorted = new TreeSet();`, which is missing a pair of
 `<>`. This is the only actual code change I have, which is why I didn't
 include this change in a patch.

 An oversight, please change.

 >  - The default values `WebstatsPeriodMinutes = 31` and
 `WebstatsOffsetMinutes = 0` seem a bit unusual. Wouldn't it make more
 sense to start with an offset of 1 minute and then run every 30 minutes?
 And regardless of the period being unusual, isn't that far too often? How
 about 360 as period and, say, 31 as offset?

 That was arbitrarily chosen.  The larger intervals make more sense in
 production.

 >  - Other than that, I'd say let's try it out when you say it's ready. I
 think that's more efficient to find remaining bugs than re-reading and
 tweaking the code. Just let me know when I should do some tests.

 Fine to go!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22428#comment:48>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online


More information about the metrics-bugs mailing list