[tor-bugs] #20540 [Metrics]: define log-levels for all java metrics-products

[Tor Bug Tracker & Wiki]

#20540: define log-levels for all java metrics-products
-------------------------+------------------------------
 Reporter:  iwakeh       |          Owner:
     Type:  enhancement  |         Status:  needs_review
 Priority:  Medium       |      Milestone:
Component:  Metrics      |        Version:
 Severity:  Normal       |     Resolution:
 Keywords:               |  Actual Points:
Parent ID:               |         Points:
 Reviewer:               |        Sponsor:
-------------------------+------------------------------

Comment (by iwakeh):

 Replying to [comment:8 karsten]:

 Good to continue on that!


 I really want to keep the info level
 [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/diff/src/main/java/org/torproject/descriptor/DescriptorSourceFactory.java?h=task-20540&id=679577985141b44bed7e2a7e8dd65c3093a99243
 here], because this gives valuable runtime information.  Maybe, the text
 could be phrased differently to make it seem less 'techy'?  On the other
 hand, people running metrics-lib or other Metrics products can be expected
 to not be offended by such an info level statement.

 The two `if (log.isDebugEnabled())` are redundant as the logging statement
 is already in the good form, see
 [http://logback.qos.ch/manual/architecture.html#parametrized Parametrized
 Logging] in the Logback manual (especially the ''Better alternative''
 section). (Will add this to the wiki logging section.)

 [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/tree/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java?id=679577985141b44bed7e2a7e8dd65c3093a99243#n56
 This] warning should contain a hint about what to do.  The sentence `Local
 directory already exists and is not a directory.` sounds quite cryptic.

 All info level statements in DescriptorIndexCollector are fine.
 [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/tree/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java?id=679577985141b44bed7e2a7e8dd65c3093a99243#n91
 This] could be changed to `Finished descriptor collection.`?

 [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/tree/src/main/java/org/torproject/descriptor/index/FileNode.java?id=679577985141b44bed7e2a7e8dd65c3093a99243#n70
 Here] I would add a hint to look at the error message, e.g., adding `The
 following error message provides more details.` or similar.  Something
 that makes operators pay attention and thus detect some file system errors
 or what ever.

 Why a slash in [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/tree/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java?id=679577985141b44bed7e2a7e8dd65c3093a99243#n115
 here]: `log.debug("Fetching remote file /{} with expected ...`?

 [https://gitweb.torproject.org/user/karsten/metrics-
 lib.git/tree/src/main/java/org/torproject/descriptor/index/DescriptorIndexCollector.java?id=679577985141b44bed7e2a7e8dd65c3093a99243#n57
 This] makes me raise the escalate vs. logging question, which is not yet
 covered in the logging definition.  The aborted descriptor collection
 should be propagated to the calling application, e.g., turn the warning
 into a RuntimeException with the same text.
 Thoughts?

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