Opened 3 years ago

Last modified 3 years ago

#22208 new enhancement

Provide lines containing extra, unrecognized arguments

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone: metrics-lib 3.0.0
Component: Metrics/Library Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


This question came up when implementing #21934: do we need to include lines with extra arguments in Descriptor#getUnrecognizedLines()? Or do we need another method Descriptor#getPartiallyUnrecognizedLines()? Or can we safely drop those extra arguments on the floor? See also TorperfResults#getUnrecognizedKeys() which is somewhat related here, but not exactly the same.

This is not super urgent, because at least the bytes of the entire descriptor are available for further processing.

The fix is also not trivial to write, because we'll have manually go through all places where extra arguments are permitted and ignored and add those lines to a list of partially unrecognized lines.

The result could be that doing nothing is the way to go. Needs more thoughts.

Child Tickets

Change History (4)

comment:1 Changed 3 years ago by karsten

Milestone: metrics-lib 1.9.0

Let's try to squeeze this into 1.9.0, so that we can finish all interface changes before 2.0.0 when we can remove deprecated interface parts and after which we won't be able to do that again before 3.0.0.

comment:2 Changed 3 years ago by karsten

Milestone: metrics-lib 1.9.0metrics-lib 2.0.0

I thought more about possible use cases and came up with the following two:

  1. When we sanitize bridge descriptors we want to know whether we did not fully recognize a descriptor. We need to look at the unrecognized parts manually before deciding whether they can go in or not. And in this case it doesn't matter whether we didn't recognize a whole line or just a line part.
  1. When we extend the descriptor format, either by writing a tor patch or by rewriting descriptors like for the directory-request Laplace noise simulation, we may want to parse descriptor parts that metrics-lib cannot yet recognize. And in such a case it also doesn't matter whether metrics-lib didn't recognize the whole line or just a line part.

I think it's easiest to simply add lines containing extra, unrecognized arguments to Descriptor#getUnrecognizedLines(), together with unrecognized lines. That also keeps the interface small, as opposed to adding yet another method for partially unrecognized lines. Oh, and dropping extra arguments on the floor also doesn't seem very useful.

This means that we'll need to change the semantics of Descriptor#getUnrecognizedLines(), and we can only do that in 2.0.0. Moving to that milestone.

comment:3 Changed 3 years ago by karsten

Heavy-heartedly, I could see us moving this from 2.0.0 to a later 2.x milestone, possibly with a new method name to avoid making this change backward-incompatible. But I see how we're running out of time in June to get 2.0.0 as stable as it should be.

comment:4 Changed 3 years ago by karsten

Milestone: metrics-lib 2.0.0metrics-lib 3.0.0

I made a start in [my task-22208 branch], but I'm even more convinced now that this patch would destabilize master, and that seems like a really bad idea one day before the planned major release. Let's move this to 3.0.0.

Note: See TracTickets for help on using tickets.