[netconf] Re: WGLC for trace-ctx-extension-03

["Roque Gagliano (rogaglia)" <[e-mail]>]

Hi Per,

Thanks for your comments, please check my comments inline.

We will publish a new version of the document later today to address several of your comments.

Roque

--
Roque Gagliano
Automation Principal Solutions Architect
[phone]

Join the NSO Digital Ecosystem:
NSO on DevNet: www.cisco.com/go/nsodevnet<http://www.cisco.com/go/nsodevnet>
NSO Developer Hub: www.cisco.com/go/nsohub<http://www.cisco.com/go/nsohub>


From: Per Andersson <[e-mail]>
Date: Friday, 4 October 2024 at 10:19
To: [e-mail] <[e-mail]>
Subject: [netconf] Re: WGLC for trace-ctx-extension-03
Hi!

I have reviewed draft-ietf-netconf-trace-ctx-extension-01.

The document is well written and needed, thanks!
We have already implemented it.

My review notes below.


The document could benefit from a terminology section explaining the
following terms:

* OTLP

* OTLP Protocol

* M.E.L.T

* gNMI

(Roque) Correct. We will add them in the next release.

Figure 3 probably means to have only bidirectional arrows since the
referenced retrieval mechanisms are either "pull or push". Currently
only the top arrow is bidirectional while the two are unidirectional.

(Roque) Correct. We will add them in the next release.


As can be seen below, the YANG module validation the recommended
namespace value is

    urn:ietf:params:xml:ns:yang:ietf-netconf-otlp-context

This ties better to the YANG module name and the presented xmlns prefix

    xmlns:ietf-netconf-otlp-context

It is noted that the namespaces for

     urn:ietf:params:xml:ns:yang:traceparent:1.0

and

     urn:ietf:params:xml:ns:yang:tracestate:1.0

correspondingly should have longer names containing the YANG module
name. However, this would be very long. Perhaps a shorter, but
corresponding namespace such as

    urn:ietf:params:xml:ns:yang:otlp-context-traceparent

could be used? If so maybe the shorter "otlp-context" namespace can be
kept instead of the recommended "ietf-netconf-otlp-context", which would
signal relation?

(Roque) This is great feedback but I believe we should not tie ourselves to OTLP, context is independent of the exporting protocol. We are updating the module for next release with “ietf-trace-context” (which would work for both RESTCONF and NETCONF but also for future protocols). Would that be ok?


YANG module

* Suggest explaining what "otlp" is in identity meta-error. Or maybe it
  is enough to use upper case if it is explained in the suggested
  terminology section?

(Roque) Good point. What we did in the new release is to remove any reference to OTLP from the YANG module as there is no dependency with the exporting protocol.


Running pyang --ietf ietf-netconf-otlp-context.yang reports:

ietf-netconf-otlp-context.yang:3: warning: RFC 8407: 4.9: namespace
value should be
"urn:ietf:params:xml:ns:yang:ietf-netconf-otlp-context"
ietf-netconf-otlp-context.yang:45: warning: RFC 8407: Appendix B: The
text about which RFC this module is part of seems to be missing or is
not correct (see pyang --ietf-help for details).

Suggest updating the YANG module to accomodate for these warnings. See
above for discussion on namespace.

(Roque) Correct, this warning is related to missing the RFC number as not yet published. This should be fix by IETF editor.


yanglint is happy!


The following commands

pyang -f yang --keep-comments --yang-line-length 69 \
      ietf-netconf-otlp-context.yang > new.yang \
&& diff ietf-netconf-otlp-context.yang new.yang

yields a diff (formatting) which should update the YANG module:


(Roque) Thanks, taking care of these warnings.

8c8,9
<     reference "RFC8791: YANG Data Structure Extensions";
---
>     reference
>       "RFC8791: YANG Data Structure Extensions";
12,13c13
<      "IETF NETCONF (Network Configuration) Working Group";
<
---
>     "IETF NETCONF (Network Configuration) Working Group";
16,17c16
<     WG List:  <mailto:[e-mail]>";
<
---
>      WG List:  <mailto:[e-mail]>";
20,44c19,43
<     client and servers needs to share some specific contextual
<     information. This NETCONF extensions aligns the NETCONF
<     protocol to the W3C trace-context document:
<     https://www.w3.org/TR/2021/REC-trace-context-1-20211123
<
<     Copyright (c) 2024 IETF Trust and the persons identified as
<     authors of the code.  All rights reserved.
<
<     Redistribution and use in source and binary forms, with or
<     without modification, is permitted pursuant to, and subject to
<     the license terms contained in, the Revised BSD License set
<     forth in Section 4.c of the IETF Trust's Legal Provisions
<     Relating to IETF Documents
<     (https://trustee.ietf.org/license-info)
<
<     This version of this YANG module is part of RFC XXXX
<
<     (https://www.rfc-editor.org/info/rfcXXXX) see the RFC itself
<     for full legal notices
<
<     The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL
<     NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED',
<     'MAY', and 'OPTIONAL' in this document are to be interpreted as
<     described in BCP 14 (RFC 2119) (RFC 8174) when, and only when,
<     they appear in all capitals, as shown here.
---
>      client and servers needs to share some specific contextual
>      information. This NETCONF extensions aligns the NETCONF
>      protocol to the W3C trace-context document:
>      https://www.w3.org/TR/2021/REC-trace-context-1-20211123
>
>      Copyright (c) 2024 IETF Trust and the persons identified as
>      authors of the code.  All rights reserved.
>
>      Redistribution and use in source and binary forms, with or
>      without modification, is permitted pursuant to, and subject to
>      the license terms contained in, the Revised BSD License set
>      forth in Section 4.c of the IETF Trust's Legal Provisions
>      Relating to IETF Documents
>      (https://trustee.ietf.org/license-info)
>
>      This version of this YANG module is part of RFC XXXX
>
>      (https://www.rfc-editor.org/info/rfcXXXX) see the RFC itself
>      for full legal notices
>
>      The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL
>      NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED',
>      'MAY', and 'OPTIONAL' in this document are to be interpreted as
>      described in BCP 14 (RFC 2119) (RFC 8174) when, and only when,
>      they appear in all capitals, as shown here.
55c54,55
<     description "Base identity for otlp attribute errors.";
---
>     description
>       "Base identity for otlp attribute errors.";
60,61c60,62
<     description "Indicates an attribute or header that is required
<       (in the current situation) is missing.";
---
>     description
>       "Indicates an attribute or header that is required
>        (in the current situation) is missing.";
62a64
>
65,66c67,69
<     description "Indicates an attribute or header value where the
<       value is incorrectly formatted.";
---
>     description
>       "Indicates an attribute or header value where the
>        value is incorrectly formatted.";
67a71
>
70,71c74,76
<     description "Indicates that the server encountered a processing
<       error while processing the attribute or header value.";
---
>     description
>       "Indicates that the server encountered a processing
>        error while processing the attribute or header value.";
78c83,84
<     description "Error type";
---
>     description
>       "Error type";
84c90
<          "This error is returned by a NETCONF or RESTCONF server
---
>         "This error is returned by a NETCONF or RESTCONF server
94,98c100,104
<           In NETCONF, the qualified XML attribute name.
<           In RESTCONF, the HTTP header name.
<           If a client sent a NETCONF RPC with the attribute
<           w3ctc:traceparent='incorrect-format'
<           this leaf would have the value 'w3ctc:traceparent'";
---
>            In NETCONF, the qualified XML attribute name.
>            In RESTCONF, the HTTP header name.
>            If a client sent a NETCONF RPC with the attribute
>            w3ctc:traceparent='incorrect-format'
>            this leaf would have the value 'w3ctc:traceparent'";
104,107c110,113
<           by the server.
<           If a client sent a NETCONF RPC with the attribute
<           w3ctc:traceparent='incorrect-format'
<           this leaf would have the value 'incorrect-format'.";
---
>            by the server.
>            If a client sent a NETCONF RPC with the attribute
>            w3ctc:traceparent='incorrect-format'
>            this leaf would have the value 'incorrect-format'.";
113,117c119,123
<           detected by the server.
<           If a client sent an RPC annotated with the attribute
<           w3ctc:traceparent='incorrect-format'
<           this leaf might have the value
<           'ietf-netconf-otlp-context:bad-format'.";
---
>            detected by the server.
>            If a client sent an RPC annotated with the attribute
>            w3ctc:traceparent='incorrect-format'
>            this leaf might have the value
>            'ietf-netconf-otlp-context:bad-format'.";


Nits:

* nc_trace as short title. Suggest using something more descriptive.
  (Also noticed short illegible titles in in
  draft-ietf-netconf-restconf-trace-ctx-headers and
  draft-ietf-netconf-transaction-id.)

(Roque) I am not sure of the process to change draft titles. Could you please advice as WG Chair?


* Section 1.2 YANG DataStore.; several more instances of "DataStore"
  -> Suggest using "YANG Datastore", this is what can be found in other
     documents.

(Roque) Done in next release.

* Inconsistency using DataStore, Datastore, and datastore throughout the
  document.
  -> Suggest using Datastore and datastore, since this is usually found
     in other WG documents.

(Roque) Done in next release.

* Section 2.2 "versionning" typo -> versioning

(Roque) Done in next release.

* "complemetary" typo -> complementary

(Roque) Done in next release.

* "faciliate" typo -> facilitate

(Roque) Done in next release.

* "infomration" typo -> information


(Roque) Done in next release.

* Markdown formatting leaking into the text rendering of the document:
  _traceparent_, _tracestate_, _w3ctc:traceparent_, _w3ctc:tracestate_,
  _nc:rpc_, _nc:rpc-reply_, _notif:notification_, _yp:push-update_, and
  _yp:push-change-update_.

(Roque) Done in next release.

* "additonal" typo -> additional
(Roque) Done in next release.

Additionally idnits reports a few warnings and comments. However,
some of them seem to be false positives.

  Checking nits according to https://www.ietf.org/id-info/checklist :
  ----------------------------------------------------------------------------

  == There are 1 instance of lines with non-RFC6890-compliant IPv4 addresses
     in the document.  If these are example addresses, they should be changed.

(Roque) This is a reference to a section on another document and not an IP address/

  -- The draft header indicates that this document obsoletes
     draft-netconf-trace-ctx-extension-00, but the abstract doesn't seem to
     mention this, which it should.

(Roque) Did not see this, there is “obsolete” in our document.

  Miscellaneous warnings:
  ----------------------------------------------------------------------------

  == The document seems to lack the recommended RFC 2119 boilerplate, even if
     it appears to use RFC 2119 keywords -- however, there's a paragraph with
     a matching beginning. Boilerplate error?

     (The document does seem to have the reference to RFC 2119 which the
     ID-Checklist requires).
  -- No information found for rfcdraft-netconf-trace-ctx-extension-00 - is
     the name correct?

  -- The document date (8 July 2024) is 87 days in the past.  Is this
     intentional?

(Roque) Fixing in next release.



  Checking references for intended status: Proposed Standard
  ----------------------------------------------------------------------------

     (See RFCs 3967 and 4897 for information about using normative references
     to lower-maturity documents in RFCs)

  -- Possible downref: Non-RFC (?) normative reference: ref.
     'W3C-Trace-Context'

(Roque) This is correct.

  == Outdated reference: A later version (-06) exists of
     draft-ietf-netconf-transaction-id-05

(Roque) It gets automatically updated to latest release when we re-spin the draft. Thanks.


     Summary: 0 errors (**), 0 flaws (~~), 4 warnings (==), 4 comments (--).


Thanks for your contribution!


--
Per, as a contributor

_______________________________________________
netconf mailing list -- [e-mail]
To unsubscribe send an email to [e-mail]