Subject: [Gen-art] Gen-art telechat review of draft-ietf-i2rs-traceability-09

From: Elwyn Davies <[email-masked]>
Date: Thu, 5 May 2016 14:40:33 +0100

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair. Please wait for direction from your
document shepherd or AD before posting a new version of the draft.

For more information, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-i2rs-traceability-09.txt
Reviewer: Elwyn Davies
Review Date: 2016/05/05
IETF LC End Date: 2016/04/29
IESG Telechat date: 2016/05/05

Summary:
I have concerns about the trace model used as explained below.  It may 
be that there is good reason and WG consensus for the model adopted, but 
it would be good to see some explanation of the rather curious hybrid 
model used.  There are also significant issues with the description of 
timestamps and a number of other nits/editorial matters to address.

Apologies for the last minute delivery.

Possibly Major issues:
Trace model:  The tracing model seems to be a curious hybrid of state 
recording and event logging.  The introduction seems to imply that the 
tracing model records events.  Indeed it does but state entry events do 
not appear to get recorded until the sequence transitions out of the 
state.  I can see that the COMPLETED entries record the total processing 
period, but this loses the detail of when actual processing of the event 
starts (as opposed to becoming PENDING).  I was somewhat surprised that 
a simple chained transition event model was not used (especially since 
the tracing entries are actually chained together already).

In particular if some sort of disaster occurs, it seems possible in this 
model that events in the PENDING queue might never appear in the trace 
log at all if the request hasn't started being processed. It also 
doesn't record any preprocessing time before the request becomes 
PENDING.  If there is a processing bottleneck this could be significant 
information.

I was also wondering whether this model traces the arrival and departure 
of clients (and whether authoentication/authorisation worked or not).   
This may be covered by operation types in the architecture which I 
haven't had time to read in detail.

Minor issues:

Nits/editorial comments:
s1:  The Intro should also contain a description of the intention of the 
document - basically a slight reworking of the abstract.  It should also 
outline the association of the framework with the interface (i2rs 
client<->agent) to which the traceability applies.

s3:
> The
>     ability to automate and abstract even complex policy-based controls
>     highlights the need for an equally scalable traceability function to
>     provide event-level granularity of the routing system compliant with
>     the requirements of I2RS (Section 5 of
>     [I-D.ietf-i2rs-problem-statement]).
The 'routing system' doesn't have an event-level granularity.  Maybe
OLD:
provide event-level granularity of the routing system
NEW:
provide recording at event-level granularity of the evolution of the 
routing system
END

s4:  The section ends with this list of 'use cases':
>     As I2RS becomes increasingly pervasive in routing environments, a
>     traceability model offers significant advantages and facilitates the
>     following use cases:
>
>     1  Automated event correlation, trend analysis, and anomaly
>        detection;
>
>     2  Trace log storage for offline (manual or tools) analysis;
>
>     3  Improved accounting of routing system operations;
>
>     4  Standardized structured data format for writing common tools;
>
>     5  Common reference for automated testing and incident reporting;
>
>     6  Real-time monitoring and troubleshooting;
>
>     7  Enhanced network audit, management and forensic analysis
>        capabilities.
I have added numbers to facilitate these comments:
IMO #2 and #4 are either not use cases or a not phrased as use cases.  
The automated testing is not really a use case as such. Having these 
characteristics supports the implementation of the actual use cases.  
Related to the data retention comment above, storing some or all of the 
trace log - and knowing which bits might be critical to control data 
retention - is a use case but the basic storage is just a necessary 
prerequisite of doing other things.  I also might suggest a reordering 
indicating importance perhaps.

Thus I would suggest replacing this with something like:

    As I2RS becomes increasingly pervasive in routing environments, a
    traceability model that supports controllable trace log retention
    using a standardized structured data format offers significant 
advantages,
    such as the ability to create common tools and support automated 
testing,
    and facilitates the following use cases:

    o  Real-time monitoring and troubleshooting of router events;

    o  Automated event correlation, trend analysis, and anomaly
       detection;

    o  Offline (manual or tools-based) analysis of router state evolution
        from the retained trace logs;

    o  Enhanced network audit, management and forensic analysis
        capabilities;

    o  Improved accounting of routing system operations; and

    o Providing a standardized format for incident reporting and test 
logging.

s5: .. is empty: Empty sections are not desirable.  A brief overview of 
the following sub-sections should be added (or alternatively promote 
s5.1 which actually describes the framework).

s5.1, para 1:
> Some notable elements of the architecture are in
>     this section.
I don't understand this sentence.  If it implies that elements of the 
architecture are defined in this section then one has to ask 'Why aren't 
they defined in the architecture document?'  Since s5.1 contains the 
whole framework, what other elements than the 'some notable' ones are there?

s5.1, para 2: The term 'northbpund' is not defined (and isn't used in 
the architecture').

s5.2: The title is ' I2RS Trace Log Mandatory Fields'  - nothing that 
isn't mandatory is discussed.  Should there be some words about optional 
extra fields?

s5.2, timestamps:  The RFC3339 format doesn't tie up with 32 bit 
resolution - there are hours and minutes etc and decimal representation 
is used.  Things like origin for timestamps needs to be defined if they 
are to be truly useful for comparison outside an individual enterprise 
(as might be implied by the incident reporting use case).  If RFC 3339 
format is really used, then the timestamps need to include the date as 
well since logs will certainly run over more than one day.  I note that 
the example in s6 shows full RFC 3339 date/time format examples.

s5.2, Applied Operation Data:  Does the Operation Data Present flag 
apply to this field?  Can this be present even if there is no Requested 
Operation Data?

  s5.2, Result Code: Need to expand acronym RIB.

s7.2:  One key point about timestamping (motivated by bitter experience) 
is that timestamps need to be recorded at the point when the event 
actually happens and not when the event is (potentially significantly 
later) entered into the log.  Logging is (as indicated) often allocated 
a low priority and event log writing may end up being postponed for a 
considerable time.

s11: I would consider I-D.ietf-i2rs-problem-statement and 
I-D.ietf-i2rs-pub-sub-requirements to be Informative; and
I-D.ietf-i2rs-rib-info-model, RFC 3339 and possibly RFC 5424 to be 
normative.

---

From: Jari Arkko <[email-masked]>
Date: Thu, 5 May 2016 10:16:25 -0400

Ok. We’re already on the telechat, but the questions seem important. Do the authors have thoughts?

Jari

On 05 May 2016, at 09:40, Elwyn Davies <[email-masked]> wrote:

> I have concerns about the trace model used as explained below.  It may be that there is good reason and WG consensus for the model adopted, but it would be good to see some explanation of the rather curious hybrid model used.  There are also significant issues with the description of timestamps and a number of other nits/editorial matters to address.
> 
> Trace model:  The tracing model seems to be a curious hybrid of state recording and event logging.  The introduction seems to imply that the tracing model records events.  Indeed it does but state entry events do not appear to get recorded until the sequence transitions out of the state.  I can see that the COMPLETED entries record the total processing period, but this loses the detail of when actual processing of the event starts (as opposed to becoming PENDING).  I was somewhat surprised that a simple chained transition event model was not used (especially since the tracing entries are actually chained together already).
> 
> In particular if some sort of disaster occurs, it seems possible in this model that events in the PENDING queue might never appear in the trace log at all if the request hasn't started being processed. It also doesn't record any preprocessing time before the request becomes PENDING.  If there is a processing bottleneck this could be significant information.
> 
> I was also wondering whether this model traces the arrival and departure of clients (and whether authoentication/authorisation worked or not).   This may be covered by operation types in the architecture which I haven't had time to read in detail.
> 
> Minor issues:
> 
> Nits/editorial comments:
> s1:  The Intro should also contain a description of the intention of the document - basically a slight reworking of the abstract.  It should also outline the association of the framework with the interface (i2rs client<->agent) to which the traceability applies.
> 
> s3:
>> The
> provide recording at event-level granularity of the evolution of the routing system
> END
> 
> IMO #2 and #4 are either not use cases or a not phrased as use cases.  The automated testing is not really a use case as such. Having these characteristics supports the implementation of the actual use cases.  Related to the data retention comment above, storing some or all of the trace log - and knowing which bits might be critical to control data retention - is a use case but the basic storage is just a necessary prerequisite of doing other things.  I also might suggest a reordering indicating importance perhaps.
> 
>   using a standardized structured data format offers significant advantages,
>   such as the ability to create common tools and support automated testing,
>   o Providing a standardized format for incident reporting and test logging.
> 
> s5: .. is empty: Empty sections are not desirable.  A brief overview of the following sub-sections should be added (or alternatively promote s5.1 which actually describes the framework).
> 
> s5.1, para 1:
> I don't understand this sentence.  If it implies that elements of the architecture are defined in this section then one has to ask 'Why aren't they defined in the architecture document?'  Since s5.1 contains the whole framework, what other elements than the 'some notable' ones are there?
> 
> s5.1, para 2: The term 'northbpund' is not defined (and isn't used in the architecture').
> 
> s5.2: The title is ' I2RS Trace Log Mandatory Fields'  - nothing that isn't mandatory is discussed.  Should there be some words about optional extra fields?
> 
> s5.2, timestamps:  The RFC3339 format doesn't tie up with 32 bit resolution - there are hours and minutes etc and decimal representation is used.  Things like origin for timestamps needs to be defined if they are to be truly useful for comparison outside an individual enterprise (as might be implied by the incident reporting use case).  If RFC 3339 format is really used, then the timestamps need to include the date as well since logs will certainly run over more than one day.  I note that the example in s6 shows full RFC 3339 date/time format examples.
> 
> s5.2, Applied Operation Data:  Does the Operation Data Present flag apply to this field?  Can this be present even if there is no Requested Operation Data?
> 
> s7.2:  One key point about timestamping (motivated by bitter experience) is that timestamps need to be recorded at the point when the event actually happens and not when the event is (potentially significantly later) entered into the log.  Logging is (as indicated) often allocated a low priority and event log writing may end up being postponed for a considerable time.
> 
> s11: I would consider I-D.ietf-i2rs-problem-statement and I-D.ietf-i2rs-pub-sub-requirements to be Informative; and
> I-D.ietf-i2rs-rib-info-model, RFC 3339 and possibly RFC 5424 to be normative.
> 
> _______________________________________________
> Gen-art mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/gen-art

---

From: Joe Clarke <[email-masked]>
Date: Fri, 6 May 2016 16:27:27 -0400

Thank you for your review, Elwyn.  You raise some excellent points.

I'm top-posting here as I think we've addressed all of your concerns. 
We're still reviewing the new text among ourselves, but I wanted to show 
you the SxS diffs to get your take.  Some of the changes incorporate 
other AD/IESG comments as well.

http://www.marcuscom.com/draft-ietf-i2rs-traceability.txt-from-09-10.diff.html

Joe

---

From: Elwyn Davies <[email-masked]>
Date: Tue, 10 May 2016 22:51:09 +0100

Thanks for the rapid update.  This looks to be making good progress.

Couple of nits and comments:
s1, para 2: s/describes use cases/describe use cases/

s5.2, Event ID:
> An event can be a Client authenticating with the Agent, a Client to 
> Agent operation, or a Client disconnecting from an Agent.
This is a good thing, but I am not sure that the format provides a way 
to identify the authentication and disconnection events.

s5.2, Starting Timestamp:
[I don't understand 'three points of prevision'.] Maybe...
OLD:
Given that many I2RS operations can occur in rapid succession, the use 
of fractional seconds MUST be used to provide adequate granularity.  
Fractional seconds SHOULD be expressed with at least three points of 
prevision in second.microsecond format.
NEW:
Given that many I2RS operations can occur in rapid succession, the 
fractional seconds element of the timestamp MUST be used to provide 
adequate granularity.  Fractional seconds SHOULD be expressed with at 
least three [or more?] significant digits in second.microsecond format.
END

s5.2, Ending Timestamp:
See the comments on the Starting Timestamp - though I think you could 
just refer to the words in the Starting Timestamp and avoid duplication.

s7.4/s7.4.3: Given that the I2RS pub-sub access method is 
mandatory-to-implement, i think I-D.ietf-i2rs-pub-sub-requirements has 
to be a Normative Reference.

Regards,
Elwyn

On 06/05/2016 21:27, Joe Clarke wrote:
> We're still reviewing the new text among ourselves, but I wanted to 
> show you the SxS diffs to get your take.  Some of the changes 
> incorporate other AD/IESG comments as well.
>

---

From: Joe Clarke <[email-masked]>
Date: Wed, 11 May 2016 11:18:08 -0400

> s1, para 2: s/describes use cases/describe use cases/

Fixed.

> to identify the authentication and disconnection events.

The intent was that these would be Operations (i.e., AUTHENTICATE 
CLIENT, DISCONNECT CLIENT).  There is nothing in the text that precludes 
this.  We can explicitly state this.

> least three [or more?] significant digits in second.microsecond format.

Changed.

> just refer to the words in the Starting Timestamp and avoid duplication.

Done.

> to be a Normative Reference.

Changed.

See the new text at 
https://www.marcuscom.com/draft-ietf-i2rs-traceability.txt-from-09-10.diff.html 
.

Thanks again for the review!

Joe

---

From: Elwyn Davies <[email-masked]>
Date: Wed, 11 May 2016 20:11:47 +0100

Hi.

I had a look at the revised diff.  Looks pretty good now.

Couple of minor points in line below.

Cheers,
Elwyn

On 11/05/2016 16:18, Joe Clarke wrote:
> CLIENT, DISCONNECT CLIENT).  There is nothing in the text that 
> precludes this.  We can explicitly state this.
I think stating it explicitly would be a good idea.
>> least three [or more?] significant digits in second.microsecond format.
Do you think millisecond resolution will be good enough?  I put in three 
because of the 'three points of prevision'   but wonder if you might 
need something closer to microsecond resolution in high throughput 
routers?  I don't know what might be desirable - I have some experience 
of a similar logging system (DTN2) and full microsecond resolution is 
occasionally useful.

---

From: Joe Clarke <[email-masked]>
Date: Fri, 13 May 2016 11:36:06 -0400

> Couple of minor points in line below.

Thanks, Elwyn.  We have posted rev -10 of the draft, which should 
address all of your comments.

Joe

---

From: Elwyn Davies <[email-masked]>
Date: Fri, 13 May 2016 21:43:35 +0100

Hi, Joe.

Thanks for the heads up.

Apologies - I missed one question that I should have asked.  You have 
removed the idea that the specified fields are mandatory in s5.2.  My 
original issue with this was about whether you would envisage that 
implementors could add additional, optional fields to the basic set, and 
if so would it be good to mention/explain how this would be handled.

The current state gives a rigid set of fields in  each trace report with 
no flexibility  (or at least not within the specification).  Is this 
what the authors intended?

Cheers,
Elwyn

---

From: Joe Clarke <[email-masked]>
Date: Fri, 13 May 2016 17:56:54 -0400

> if so would it be good to mention/explain how this would be handled.

No problem.  Revision numbers are cheap :-).

> what the authors intended?

We set out to list those things that we wanted to see from a baseline, 
all-vendor point of view.  Your point was raised more than once, but I 
personally was worried that a broad, "plus anything else" statement 
might also receive some criticism.

Do you have any examples we might add as an "e.g." to set context? 
Things that crossed my mind were Client vendor (think User-Agent type 
data) and Agent stats like free memory.  The former isn't part of the 
architecture, but could be implemented by a vendor.  The latter might be 
useful in certain support scenarios.

Joe

---

From: Elwyn Davies <[email-masked]>
Date: Thu, 19 May 2016 11:16:02 +0100

Hi, Joe.

I checked through the changes in -11 and that all looks good now.

Thanks,
Elwyn

On 13/05/2016 21:43, Elwyn Davies wrote:
> implementors could add additional, optional fields to the basic set, 
> and if so would it be good to mention/explain how this would be handled.
>
> The current state gives a rigid set of fields in  each trace report 
> with no flexibility  (or at least not within the specification).  Is 
> this what the authors intended?
>
> Cheers,
> Elwyn
>
>>>>> use