[tor-bugs] #16225 [Metrics/metrics-lib]: Unify exception/error handling in metrics-lib

[Tor Bug Tracker & Wiki]

#16225: Unify exception/error handling in metrics-lib
---------------------------------+-----------------------------------
 Reporter:  karsten              |          Owner:  karsten
     Type:  enhancement          |         Status:  needs_information
 Priority:  Medium               |      Milestone:  metrics-lib 1.7.0
Component:  Metrics/metrics-lib  |        Version:
 Severity:  Normal               |     Resolution:
 Keywords:                       |  Actual Points:
Parent ID:                       |         Points:
 Reviewer:                       |        Sponsor:
---------------------------------+-----------------------------------

Comment (by karsten):

 Replying to [comment:6 iwakeh]:
 > Ok, I start from the opposite direction and hopefully we meet in the
 perfect middle :-)

 Okay, let's try that! :)

 > The use-case this ticket mostly refers to is bulk processing, i.e. there
 is a bunch of descriptors somewhere (file system, remote server) and
 metrics-lib fetches, synchronizes, and parses these files. One constraint
 is that there will always be some descriptors that cannot be parsed.

 Agreed.

 > From this scenario, I would conclude that methods like readDescriptors,
 parseDescriptors, collectDescriptors don't throw
 DescriptorParseExceptions, but simply process as much as possible and
 provide a list of problematic descriptors at the end, which the API using
 program can choose to ignore or process.
 > Does this sound like the right use-case description?

 It does sound like the right use-case description!

 The part that needs more thoughts is when and how we should provide
 problematic descriptors and related error cases.  Providing them at the
 end can easily lead to out-of-memory situations, because we might have to
 keep more and more problematic descriptors in memory until we're finally
 done parsing.  Maybe we can include problematic descriptors in line with
 non-problematic ones.

 How about this: whenever we can't parse a descriptor, we include a
 `Descriptor` instance with the raw contents we cannot parse in the
 descriptor queue.  And we include a new method `Descriptor#getException()`
 that returns the `DescriptorParseException` that we ran into, or `null` if
 we didn't run into an exception while parsing.

 Similarly, if we run into an exception while downloading files from a
 remote server or while reading files from the file system, so before
 splitting contents into descriptor-sized chunks and attempting to parse
 those, we include a `Descriptor` instance without descriptor contents and
 with just the exception.

 I could see how this works even without breaking existing code, because
 any code processing `Descriptor` instances should check whether they're
 `instanceof` whatever they had hoped to get before casting the instance
 and accessing its getters.  At least that's what I'm doing, and I'm
 skipping any unexpected descriptors.  If an application would want to
 learn more about reasons why a descriptor was skipped, it can log the
 exception or handle it otherwise.  It can even distinguish between I/O and
 parse exceptions.  But if it doesn't care as much, it can simply rely on
 `Descriptor` subtypes to be valid, parsed descriptors.

 > Regarding the code-example:  configuration of a descriptor source by
 fluent-style (or method chaining) is fine, but metrics-lib currently has
 the DescriptorSourceFactory approach, which would need to be adapted.
 That is, I see two things:  the ideas around the code of
 DescriptorSourceBuilder are ideas about a new way of configuration and not
 exception/error handling in metrics-lib, i.e. a different ticket (there is
 one for redesign of the interface hierarchy, I'll look for it).  Second,
 the current configuration and descriptor source instanciation need to be
 considered together (in that other ticket).

 Yes, you're right, this issue is orthogonal and should be discussed on a
 different ticket.  I'll create one.

 Thanks for your very valuable input above!

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