[tor-dev] Proposal: Controller events to better understand connection/circuit usage

[Karsten Loesing]

Hi Damian!

Thanks a lot for your feedback!  The proposed event formats are not at
all set in stone.  If we can change formats to make events easier to
parse, by all means let's do that.  You probably have more experience
with the control port than anyone at Tor, so I say let's listen to you.

On 2/7/13 5:33 PM, Damian Johnson wrote:
> Hi Karsten. Looks good to me, just a few comments on the spec parts...
> 
>> 5.1. Adding an ID field to ORCONN events
> 
> Please define the format of ConnId. This would belong in section 2.4,
> expanding...
> 
> ; Unique identifiers for streams or circuits.  Currently, Tor only
> ; uses digits, but this may change
> StreamID = 1*16 IDChar
> CircuitID = 1*16 IDChar
> IDChar = ALPHA / DIGIT

Good point, will do.

>> 5.2. Bandwidth used on an OR or DIR or EXIT connection
> 
> Minor nit pick, but the normal way of showing an enumeration is...
> 
> ConnType = "OR" / "DIR" / "EXIT"
> 
> Please make it clear in the spec if new ConnType are allowed or not.

Yes, new types should be allowed, though I think it will be just these
three types for now.

I wonder if we should avoid restricting connection types in the spec by
defining this argument as connection type _string_ as opposed to an
enumeration.  That would be similar to cell type strings in CELL_STATS
events.  Has the advantage that we don't have to touch the spec whenever
we add another connection type.  Does that make sense?

>> 5.3. Per-circuit cell stats
> 
> Again, please specify the format of all of these cells. Most are
> obviously numeric, but a formal specification helps answer questions
> like 'can I assume this can't be negative?'.

All numbers are non-negative integers.  Will specify this more formally.

> Are you sure that you want to use positional arguments for all of
> these? If you do then you'll lose a lot of flexibility. Making them
> positional means...
> 
> * these values will always be mandatory, even if you want to deprecate
> them in the future
> * you'll always need to keep this ordering, with new values appended to the end

A fine question.  Here are two examples of this event:

CELL_STATS 8 47110 15 created=1;relay=1 created=1;relay=1
created=0;relay=0 39943 16 create=1 create=1 create=0

CELL_STATS 8 47110 15 - - - 39943 16 relay_early=1 relay_early=1
relay_early=0

Can you suggest a better format, maybe one without positional arguments?

>> "650" SP "TB_EMPTY" SP ["GLOBAL" || "RELAY" || "ORCONN" SP ConnID]
>> SP ReadBucketEmpty SP WriteBucketEmpty SP LastRefill CRLF
> 
> Ewww! No, please don't do this. Pretty please with a cherry on top?

Ewww, cherries... ;)

> Having either one *or* two positional arguments for the first slot
> makes this a special snowflake among tor events (and hence need
> special handling). ConnID should be an optional keyword argument
> instead, documented to only exist if the
> event type is "ORCONN".

Sure, sounds doable.  Will fix that.

> Same cautioning as before about positional vs keyword arguments, and
> defining the values.

I'm fine changing positional arguments to keyword arguments.  Maybe we
should do that for all three new events.  Want to suggest new event
formats with keyword arguments to make them easier to parse by Stem and
friends?  I'll wait with making changes to the proposal until I hear
back from you.

Thanks again for your very valuable feedback!

Cheers!
Karsten