#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 iwakeh): Replying to [comment:18 karsten]: > [snip] > > 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`). Well, throwing an Exception is a way to communicate problems to the caller. Anyway, there are not that many distinguishable error situations; e.g. w/o index.json there is no descriptor downloading -> Exception. > > 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. I chose the name `InvalidDescriptor` in the beginning just to have a name; 'unparsable' is a better prefix. In general this type should be there to communicate that there is a descriptor that couldn't be parsed and provide all information accessible w/o proper parsing. The exception itself is not (yet) of much use. > > Does this make more sense? I think our ideas/concepts converge :-) > > > > 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! Added a comment to #16225 as reminder. > > > > 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. > Taking a look at the update next. > > 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:19> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online _______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs