[tor-bugs] #22983 [Metrics/metrics-lib]: add a descriptor interface and implementation for web-logs

[Tor Bug Tracker & Wiki]

#22983: add a descriptor interface and implementation for web-logs
---------------------------------+-----------------------------------
 Reporter:  iwakeh               |          Owner:  metrics-team
     Type:  enhancement          |         Status:  needs_review
 Priority:  Medium               |      Milestone:  metrics-lib 2.1.0
Component:  Metrics/metrics-lib  |        Version:
 Severity:  Normal               |     Resolution:
 Keywords:                       |  Actual Points:
Parent ID:                       |         Points:
 Reviewer:                       |        Sponsor:
---------------------------------+-----------------------------------

Comment (by karsten):

 That's quite a few commits there, more than I can handle in a single
 chunk. But let me start going through them and put reviews here as I
 finish them.

 e0c5774 and e224680 look good. Already merged to master.

 Some suggestions for 77b143d:
  - JSON is not exactly a compression type, it's a file content type. Maybe
 we can remove it now and fall back to the same PLAIN type that we fall
 back to for uncompressed logs?
  - The (duplicate) documentation in `FileType` doesn't help that much. I'd
 say let's either document all types or none of them. If I had to choose,
 I'd say none.
  - In `FileType#findType`, I think it's bad to catch `RuntimeException`,
 because that covers all kinds of programming errors made by application
 developers. For example, if they give us `ext = null` as parameter, we'll
 tell them it's a `PLAIN` file, but really they shouldn't give us that
 parameter. Better catch `IllegalArgumentException` only and let them catch
 their `NullPointerException` if they think that giving us `null` is a good
 idea. Related to this, let's not say in the documentation that we're not
 throwing any exceptions. (Of course, if the plan is to handle `null`
 exactly like this, let's put `IllegalArgumentException |
 NullPointerException` in the `catch` clause to indicate that we put it
 there intentionally.)
  - If you make changes to this commit, please make them as fixup/squash
 commit on top of the branch, so that I can continue reviewing subsequent
 commits.

 d687f44 looks good.

 I didn't finish my review of 76ae1e7, but here's some early feedback:
  - There's a file `IndexNode.java.orig` which should go away (in a later
 fixup commit).
  - The documentation of `WebServerAccessLogImpl` says that "the file is
 not read." Yet, it says a few sentences later that "it will be compressed
 to the default compression type" in case it's not compressed yet. That's a
 contradiction. And should we really do this? Or should we provide a way
 for the (internal) user to compress the file using the compression type of
 their choice?

 I'll resume the review as soon as I get to it.

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