Re: [tor-bugs] #22141 [Metrics/metrics-lib]: Deprecate `DescriptorFile` and add relevant information to `Descriptor`

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#22141: Deprecate `DescriptorFile` and add relevant information to `Descriptor`
---------------------------------+-----------------------------------
 Reporter:  karsten              |          Owner:  karsten
     Type:  enhancement          |         Status:  needs_review
 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):

 Replying to [comment:17 iwakeh]:
 > Replying to [comment:16 karsten]:
 > > `DescriptorIndexCollector` indeed does not parse any descriptors, and
 neither did `DescriptorCollectorImpl`.  But I didn't refer to
 `DescriptorParseException` here.  We wouldn't be able to include other
 exceptions than `DescriptorParseException`, like `IOException`, in an
 output by `DescriptorIndexCollector`, because there's no output.  And that
 was the plan for `DescriptorReader`, where we were planning to include
 IOException in a returned `InvalidDescriptor`.  (The code may make this
 more obvious.)
 >
 > Hmm, maybe I forgot about something, but IOE rather belongs to a file
 than to a descriptor unless the two coincide.

 Sorry for not being clear.  Let me try to rephrase.

 The issue with `DescriptorCollector` returning `void` is that we cannot
 return any exceptions caught while fetching descriptor files from the
 remote CollecTor host to the caller.  This could be anything from fetching
 the `index.json` file or an actual descriptor file to differences between
 expected and actual file size or even indexing the local target directory.

 What we ''can'' do is log these exceptions/issues, so that the operator
 gets aware of them.  But we can't hand over exceptions to the application.
 At least not easily (see `ExceptionListener`).

 But if we can't pass exceptions while collecting descriptors to the
 application, why should we try hard to do this while reading descriptors?
 My point is that we can similarly log these warnings and don't have to
 start creating `InvalidDescriptor` just to let the application know about
 these exceptions.

 Regardless, we can pass parse exceptions encapsulated in
 `UnparseableDescriptor` to the application.  That's limited to
 `DescriptorReader` and `DescriptorParser`, because `DescriptorCollector`
 doesn't parse and return descriptors.

 Does this make more sense?

 > > I also now understand your "ParseExceptionsLog" idea better.
 Basically, we would create a new structure for logging that is less
 dependent on classes and more related to operation.  Not opposed to that
 idea.  I could imagine that we'd want to look more at the other log
 statements and see what other channels would be useful.  How about we
 leave that for after 2.0.0 is released?
 >
 > Yes, that's fine or later.

 Okay!

 > > Can you look at the branch?  I think it makes sense to look now.
 Thanks!
 >
 > I think UnparseableDescriptor should extend Descriptor otherwise there
 is no access to the raw bytes etc. from the API.
 >
 > * getDescriptorFile, getRawDescriptorBytes: should work in Un.Desc. as
 in other Descr.
 > * getAnnotations, getUnrecognizedLines:  This behavior needs to be
 defined and documented in small tests.  (Annotations might be corrupt,
 unrecognized lines might not be identifiable etc.)

 Ugh, totally.  That's an oversight.  Fixed in a new commit in the same
 branch.

 > Other than these this looks ok as a 1.9.0 release solution.

 Awesome!  I'm also optimistic that this will work and that we won't run
 into too many yet unforseen issues.  I'll test this branch (with the
 `extends Descriptor` fix) in Onionoo and a few other applications.

 Thanks!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22141#comment:18>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs