[metrics-bugs] #22139 [Metrics/metrics-lib]: last_restarted and platform missing even though it is available in descriptor

[Tor Bug Tracker & Wiki]

#22139: last_restarted and platform missing even though it is available in
descriptor
---------------------------------+-----------------------------------
 Reporter:  cypherpunks          |          Owner:  metrics-team
     Type:  defect               |         Status:  new
 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:

 I looked more at this issue and believe that it's not exactly a mad bug
 but a rather sad design decision.  Let me explain.

 The situation is that we have a descriptor file starting with a truncated
 descriptor D,,0,, followed by several complete descriptors D,,1,, to
 D,,n,,.  In this specific case D,,0,, is truncated in the middle of a
 base64-encoded digest string that we're trying to validate, so we're
 throwing a `DescriptorParseException`.  But we're not catching that
 exception and moving on to D,,1,,.  Instead, we're catching the exception
 in `DescriptorReader` where we're passing that exception to
 `DescriptorFileImpl#setException()` and not touching
 `DescriptorFileImpl#setDescriptors()` at all.  Note that we would have
 done the exact same thing when running into a `DescriptorParseException`
 in D,,n,,!  As a result, a `DescriptorFile` either contains parsed
 descriptors or an exception.  All or nothing.

 This is not a bug, because we never claimed that we're returning parsed
 descriptors even if we run into an exception.  That would have been the
 better design: just skip the descriptor we can't parse, set the exception
 if it's the first exception we ran into (as documented), and then move on
 to the next descriptor.  But we can't say that the current implementation
 is incorrect.  Even worse, if we change this behavior now, that might be
 considered a backward-incompatible change.

 History lesson: when metrics-lib was designed, descriptors were usually
 stored as one descriptor per file.  We later switched to concatenating
 server descriptors and extra-info descriptors because handling many small
 files was less efficient.  And even later, last year, we considered
 concatenating even more descriptors including votes.  But that was all not
 the case when metrics-lib came to life.  End of history lesson.

 My suggestion would be to implement #22141 first, so that the smallest
 returned unit is a `Descriptor`, not a `DescriptorFile`.  We can fail a
 single descriptor that we can't parse, but we don't have to fail all other
 descriptors that happen to be contained in the same descriptor file.  I'm
 changing the milestone to 1.9.0 for now.

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