Re: [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 2.2.0
Component:  Metrics/metrics-lib  |Version:
 Severity:  Normal   | Resolution:
 Keywords:   |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+---
Changes (by iwakeh):

 * milestone:   => metrics-lib 2.2.0


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
Component:  Metrics/metrics-lib  |Version:
 Severity:  Normal   | Resolution:
 Keywords:   |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+---
Changes (by karsten):

 * milestone:  metrics-lib 1.9.0 =>


Comment:

 We already achieved some of the aspects here by merging #22141, and now we
 ran out of time for the 1.9.0 release which is scheduled for tomorrow.
 Let's work more on the remaining aspects here when 1.9.0 and 2.0.0 are
 out.  Removing from planned milestones.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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.9.0
Component:  Metrics/metrics-lib  |Version:
 Severity:  Normal   | Resolution:
 Keywords:   |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+---

Comment (by iwakeh):

 Also consider different Loggers that enable sorting of log-statements via
 configuration. (see #22141#comment:15)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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.9.0
Component:  Metrics/metrics-lib  |Version:
 Severity:  Normal   | Resolution:
 Keywords:   |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+---

Comment (by karsten):

 See also #22141 which has an implementation of some aspects discussed
 here.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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.9.0
Component:  Metrics/metrics-lib  |Version:
 Severity:  Normal   | Resolution:
 Keywords:   |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+---
Changes (by karsten):

 * milestone:  metrics-lib 1.8.0 => metrics-lib 1.9.0


Comment:

 It seems that we're not ready for 1.8.0 yet, and we want to release that
 next week.  Pushing back to 1.9.0.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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.8.0
Component:  Metrics/metrics-lib  |Version:
 Severity:  Normal   | Resolution:
 Keywords:   |  Actual Points:
Parent ID:   | Points:
 Reviewer:   |Sponsor:
-+---
Changes (by iwakeh):

 * milestone:  metrics-lib 1.7.0 => metrics-lib 1.8.0


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

 Replying to [comment:11 karsten]:
 > [snip]
 > > > 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.
 > >
 > > If we were using java8, I'd suggest using
 java.util.Optional.
 >
 > It seems like we won't have Java 8 in the near future (see #19622), but
 can you elaborate how that would help here?

 I didn't sink to much time into the possible design options.  It'll also
 mean a change from the blocking method calls to always returning
 something.   Actually, that would be achievable with `Future`s much
 better, where the API user decides when to wait.  But, just some random
 thoughts here, no change proposal intended.

 >
 > > Actually your suggestions goes in that direction, too.
 > > Maybe, a new descriptor class InvalidDescriptor could be useful here?
 It would be accessible via the instanceof-method and have limited
 information: at least the Exception and maybe also its origin (url, path),
 in case of a file also bytes.
 >
 > I briefly thought about such a new type for this case, too.  But the
 only reason I found was that there wouldn't be a method
 `Descriptor#getException()` that sometimes returns `null`.  However,
 adding such a type would mean that whoever is interested in
 exceptions/errors would have to do another `instanceof` check and
 downcast.  Are there other aspects?

 Yes, the usefulness of one or the other approach depends on the
 programming style of the API using client.  I don't feel strongly about
 either.

 Regarding the methods returning a collection type (like unrecognized
 fields for torperf/onionperf or the get-exception method above), I think
 it would be more convenient to return an empty (immutable) collection of
 the wanted type (like
 
[https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#emptySet%28%29
 emptySet] instead of `null`.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:10 iwakeh]:
 > Replying to [comment:8 karsten]:
 > > ...
 > > > 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!
 >
 > Maybe, add that to the javadoc somewhere :-)

 Yes, we should, once we fully support that use case.  Right now, if
 `DescriptorParser` is tasked with parsing a given file and runs into
 something it cannot parse, it just throws that exception and calls it a
 day (see #22139).

 >
 > >
 > > 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.
 >
 > If we were using java8, I'd suggest using
 java.util.Optional.

 It seems like we won't have Java 8 in the near future (see #19622), but
 can you elaborate how that would help here?

 > Actually your suggestions goes in that direction, too.
 > Maybe, a new descriptor class InvalidDescriptor could be useful here?
 It would be accessible via the instanceof-method and have limited
 information: at least the Exception and maybe also its origin (url, path),
 in case of a file also bytes.

 I briefly thought about such a new type for this case, too.  But the only
 reason I found was that there wouldn't be a method
 `Descriptor#getException()` that sometimes returns `null`.  However,
 adding such a type would mean that whoever is interested in
 exceptions/errors would have to do another `instanceof` check and
 downcast.  Are there other aspects?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

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

 Replying to [comment:8 karsten]:
 > ...
 > > 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!

 Maybe, add that to the javadoc somewhere :-)

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

 If we were using java8, I'd suggest using java.util.Optional.
 Actually your suggestions goes in that direction, too.
 Maybe, a new descriptor class InvalidDescriptor could be useful here?  It
 would be accessible via the instanceof-method and have limited
 information: at least the Exception and maybe also its origin (url, path),
 in case of a file also bytes.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:8 karsten]:
 > Replying to [comment:6 iwakeh]:
 > > Regarding the code-example: [...]
 >
 > Yes, you're right, this issue is orthogonal and should be discussed on a
 different ticket.  I'll create one.

 Added as #22196.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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):

 Here's a related question from #16151 (discussion around
 `DescriptorCollector`) that was the sole reason for keeping that ticket
 open: "When receiving something other than a 200 response code while
 fetching the directories or files shouldn't that be reported in some way?
 Things like redirects, bad requests, not authorized, or others might be
 important for the caller?"

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [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:
-+---
Changes (by iwakeh):

 * status:  needs_review => needs_information


Comment:

 Ok, I start from the opposite direction and hopefully we meet in the
 perfect middle :-)

 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.

 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?

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

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs