[tor-bugs] #22983 [Metrics/Library]: Add a Descriptor subinterface and implementation for Tor web server logs

[Tor Bug Tracker & Wiki]

#22983: Add a Descriptor subinterface and implementation for Tor web server logs
-----------------------------+-----------------------------------
 Reporter:  iwakeh           |          Owner:  metrics-team
     Type:  enhancement      |         Status:  needs_revision
 Priority:  Medium           |      Milestone:  metrics-lib 2.2.0
Component:  Metrics/Library  |        Version:
 Severity:  Normal           |     Resolution:
 Keywords:  metrics-2017     |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:                   |        Sponsor:
-----------------------------+-----------------------------------
Changes (by karsten):

 * status:  needs_review => needs_revision


Comment:

 Alright, here's what I found:

 Commit c6c805c looks trivially correct. It comes a bit out of nowhere, so
 may you could add a remark to the commit message of future commits like
 this saying why you're making this change now. Like, did it violate a
 checkstyle rule? And was this the only place that was lacking a space? In
 any case, ready to merge this one.

 Commit 3b426bf contains a couple of code style violations: the summary
 fragment should describe the package and not be a warning; the summary
 fragment is not supposed to contain markup like <h1>; there needs to be an
 empty line before the next <p>; and the line starting with
 "descriptors.</p>" is wrongly indented. I'm providing some tweaks in a
 fixup commit. Please review and tweak further as needed.

 Commit 7822a27 has several issues that require several fixup commits
 and/or discussion:
  - My commit e4d6e90 in my task-22983-4 branch contains a bunch of
 whitespace fixes and other trivial tweaks which I believe are mostly
 oversights. (Unless they are not, in which case see the next item.)
  - My commit 664f540 is a mix of suggestions and tweaks. Some of the
 tweaks are an attempt to make your code fit better into existing code,
 which is either based on our style guide or on implicit conventions in
 existing code for which we don't have a written style guide yet. Please
 review that commit carefully. If you believe that some of these are non-
 issues, let's discuss that. I'm not saying that anything or anyone is
 wrong, I'm just trying to keep (heh, or make?) the overall code base as
 readable as possible and at the same time make future review processes
 even smoother.
  - I wonder if we should take out the `LogDescriptor` interface for
 several reasons: there is just one subinterface extending that, so YAGNI;
 the only fields/methods that it adds are the log date, and it's unclear if
 future logs will cover a single date or have entirely different
 requirements. Looks like premature interface hierarchy optimization to me.
  - I'm unclear whether this new code will cover both original and
 sanitized web server logs. Possibly related to that question, why is
 validation separated from sanitization, and is validation performed before
 sanitization or not? Can you clarify?
  - I may have further comments based on your answers to my
 questions/remarks above.

 I haven't reviewed commit 929e265 in detail yet.

 When you make changes, please make them as fixup commits on top of my
 branch. Thanks!

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