[metrics-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 iwakeh):

 Replying to [comment:19 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.

 Yes, thanks for starting!

 >
 > 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?

 I didn't want to impose additional changes for generating the index.json*
 files.  I think it is also useful to state explicitly that files ending in
 '.json' are not compressed.
 Maybe, we can revisit the question later when applying more changes, e.g.,
 also adding enums for 'tar' and 'tar.gz' etc.?

 >  - 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.

 Agreed, I removed the comments.

 >  - 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.)

 True, the catch can be more restrictive; a fixup-commit contains the
 `IllegalArgumentException | NullPointerException` variation.

 >  - 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).

 Removed.

 >  - 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?

 Good point!  I added a fixup commit, which provides a constructor arg for
 the default compression, but that will only be used, if the supplied log
 file doesn't have any compression extension.  All other cases, like
 changing compression type should be handled outside of metrics-lib using
 FileType enums directly.

 There is no contradiction, b/c the given bytes will be compressed, but not
 the file.  The File constructor argument is here to implement `Descriptor`
 interface's method `getDescriptorFile` (and also b/c we need filename and
 immediate parent path for meta-data).
 I think, it doesn't hurt to move the discussion and resulting changes to
 'the big picture' of ticket #22695.

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

 All changes are provided as fixup commits.

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