A YANG Data Model for Service Assurance
draft-ietf-opsawg-service-assurance-yang-11

I support Eric's DISCUSS position.  In particular, please fix the BCP 14 boilerplate.  BCP 13 is MIME registration procedures.

We covered the terseness of the shepherd writeup during the telechat, so my DISCUSS is resolved.  Preserving the comment:

A number of the shepherd writeup questions were hastily answered and the information we need is largely missing.  For instance:

--
> What type of RFC publication is being requested on the IETF stream (Best
> Current Practice, Proposed Standard, Internet Standard,
> Informational, Experimental or Historic)? Why is this the proper type
> of RFC? Do all Datatracker state attributes correctly reflect this intent?

yes.
--

There are three questions here, and only the last of them can be answered "yes".  The second one is the most interesting one.

--
> Several IETF Areas have assembled lists of common issues that their
> reviewers encounter. For which areas have such issues been identified
> and addressed? For which does this still need to happen in subsequent
> reviews?

no.
--

I don't understand.

--
> Have reasonable efforts been made to remind all authors of the intellectual
> property rights (IPR) disclosure obligations described in BCP 79? To
> the best of your knowledge, have all required disclosures been filed? If
> not, explain why. If yes, summarize any relevant discussion, including links
> to publicly-available messages when applicable.

yes.
--

So something's been filed, or reasonable reminders were sent?  The datatracker appears to imply this must mean the latter, but it would be great to be clear.

I have not much to add to my esteemed collegue's Éric::Vyncke/128 review, two nits:

  |     +--rw under-maintenance!
  |     |  +--rw contact    string

This could use a freeform field or duration field to hold information about expected maintenance time, as that would be the main question anyone could have for the contact person :)

  description
    "This module extends the ietf-service-assurance-device module to
     add specific support for devices of ACME Corporation. ";

I would somehow put in the Example word in there to avoid anyone thinking that this is a real definition. I know it is stated before, but implementers just scanning code might miss it :)

** YANG.  under-maintenance/contact.  

It is suggested that this name contain one or more of the
  following: IP address, management station name,
  network manager's name, location, or phone number.

Section 1 of the documents states that this YANG module is intended to be machine readable.  This field appears to be a free form text field with a number of possible of values.  Should additional structure be provided?

Thanks the the authors for writing this document, and also to Bo Wu for the OpsDir review - https://datatracker.ietf.org/doc/review-ietf-opsawg-service-assurance-yang-10-opsdir-telechat-wu-2022-12-12/ .


I'm not a YANG expert, and so much of my comments are just editorial / readability nits to try make the document even better. Feel free to address them, or not...

1: "An instance of the device subservice is representing a subpart of the network system, namely a specific device. An instance of the ip-connectivity subservice representing a feature of the network, namely the connectivity between two specific IP addresses on two devices."
P: s/subservice is representing a/subservice represents a/  (in both places)

2: "For ip-connectivity subservice, at " -> "For *the* ip-connectivity subservice, at "

3: "a health-status indicating how healthy the subservice is and when the subservice is not healthy, a list of symptoms explaining why the subservice is not healthy."
P: "a health-status indicating how healthy the subservice is, and when the subservice is not healthy, a list of symptoms explaining why the subservice is not healthy." (I had to reread this a few time to parse)

4: "However, such an instability might be a relevant symptom for diagnosing the root cause of a problem."
P: "However, such instability might be a relevant symptom for diagnosing the root cause of a problem."

5: "In this section, we propose some guidelines in order to build theses extensions."
s/theses/these/

6: "We define two subservice types in the next sections:"
P: "We define two subservice types in the following sections:"

Thanks for working on this document.

Boilerplate and downref issues have already been pointed out by other ADs. I have a similar thought or question about namespace as Eric V has, would like to see the response from the authors.

I entirely support Murrays discuss

# GEN AD review of draft-ietf-opsawg-service-assurance-yang-10

CC @larseggert

Thanks to Dan Romascanu for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/PUK6ex0SQtj7-di4O2RM31mul2A).

## Comments

### Boilerplate

This document uses the RFC2119 keywords "SHALL NOT", "SHOULD NOT", "SHALL",
"MAY", "REQUIRED", "RECOMMENDED", "OPTIONAL", "SHOULD", "MUST NOT", "NOT
RECOMMENDED", and "MUST", but does not contain the recommended RFC8174
boilerplate. (It refers to BCP 13 and not BCP 14...)

### DOWNREFs

DOWNREF `[I-D.ietf-opsawg-service-assurance-architecture]` from this Proposed
Standard to Informational `draft-ietf-opsawg-service-assurance-architecture`.
(For IESG discussion. It seems this DOWNREF was not mentioned in the Last Call
and also seems to not appear in the DOWNREF registry.)

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Outdated references

Reference `[RFC7895]` to `RFC7895`, which was obsoleted by `RFC8525` (this may
be on purpose).

### Grammar/style

#### Section 3.2, paragraph 29
```
. The subservices hierarchically organised by dependencies constitute an ass
                                 ^^^^^^^^^
```
Do not mix variants of the same word ("organise" and "organize") within a
single text.

#### Section 3.3, paragraph 16
```
ption "Date and time at which the symptoms history starts for this subservic
                                  ^^^^^^^^
```
An apostrophe may be missing.

#### Section 3.3, paragraph 18
```
iption "Container for the list of agents’s symptoms"; list agent { key "id";
                                  ^^^^^^^^
```
Did you mean "agent's" or "agents'"?

#### Section 3.3, paragraph 18
```
e some guidelines in order to build theses extensions. The mechanism to add a
                                    ^^^^^^
```
Did you mean "these"?

#### Section 3.3, paragraph 18
```
 The parameters should be defined in an container named "parameters" augmenti
                                     ^^
```
Use "a" instead of "an" if the following word doesn't start with a vowel sound,
e.g. "a sentence", "a university".

#### Section 6.1, paragraph 3
```
 be given access to monitor their services status (e.g. via model-driven tel
                                  ^^^^^^^^
```
An apostrophe may be missing.

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool