This is my YANG doctors review of draft-ietf-opsawg-sap-02.  I have reviewed the YANG module only.

The YANG module has the following structure:

    +--rw service* [sap-type]
       +--rw sap-type                    identityref
       +--rw service-attachment-point* [attachment-id]
          +--rw attachment-id               string
          +--rw description?                string
          +--rw attachment-interface?       string
          +--rw parent-termination-point?   nt:tp-id
          +--rw interface-type?             identityref
          +--rw encapsulation-type?         identityref
          +--rw role?                       identityref
          +--rw peer-customer-sap-id?       string

o  "service"

  The model defines a list "service", with the key "sap-type".  This
  looks a bit weird - is the term "service" the same thing as "sap"?
  I don't think so, and the text in the document talks about "service
  type" in section 5.

  I don't know which term is correct.  If the correct term "service",
  I suggest you change "sap-type" to "service-type".  If the correct
  term is "sap", I suggest you change the list name.

  In any case, there can obviously only be one "service" of a given
  type, but I assume this is intentional.


o  "service-attachment-point"

  In other places in the model you use "sap" ("peer-customer-sap-id",
  "sap-type").  Perhaps simple call this "sap".


o  "attachment-id"

  In the description of the leaf "attachment-id", it suggests that
  this is the identifier of the "service-attachment-point".  Normally,
  I would suggest to not repeat the name of the list in the names of
  the nodes in the list, and thus I would suggest simply "id".
  However, in this case it seems the model that this module augments
  use this convention, so perhaps call this leaf
  "service-attachment-point-id" - which is quite long, perhaps
  "sap-id".

  Also note the name mismatch "peer-customer-sap-id" and
  "attachement-id".

  Also, I note that the type of the id in this document is a string,
  whereas in the other models it is an inet:uri.  See section 4.4.11
  of RFC 8345.  Note that all examples in RFC 8345 violates the "uri"
  type, so it it is not clear to me that they really meant uri.  My
  suggestion is to stick to string for this type.  (BTW, the example
  in this document also violates the "uri" type, e.g.:
             "network-id": "an-id"
  is not a valid uri.)


o  "attachment-interface" vs "interface-type"

  These two leafs define properties of the same object; the name and
  type of the "interface to which the SAP is bound".

  This should be reflected in the names of these leafs.  Perhaps:
  "interface-name" and "interface-type".

  Also, I suggest you put these two leafs together in the data model, giving:
  
          ...
          +--rw description?                string
          +--rw parent-termination-point?   nt:tp-id
          +--rw interface-name?             string
          +--rw interface-type?             identityref


o  "interface-type"

  It is not clear to me how this relates to the
  "/interfaces/interface/type" from RFC 8343.  How are the interface
  types defined in iana-if-type mapped to the types defined in this
  document?  Perhaps they are not?


o  "role"

  The description of "role" says: 

           description
             "Indicates whether the SAP is an UNI or a NNI.";

  Now, the role is an identity, which is an "extensible enumeration".
  The description seems to indicate that this leaf really can have
  just two values.  If the description is right, perhaps change this
  type to an enumeration.  Otherwise, change the description.


o  "peer-customer-sap-id"

  The description says:

             Indicates an identifier of the peer's termination
             identifier (e.g., Customer Edge (CE)). This

  Is the peer always a "customer sap"?  If so, it could be explained
  better in the description.  If not, the leaf's name is not correct.



/martin