Should TogNMINotifications consider Encoding as well?

[gsindigi]

Hi, TogNMINotifications helps in converting a GoStruct to a slice of Notification messages. While doing so, should this also consider the target's supported Encoding types as an additional argument? Or, is there any other way to accomplish the same? Per the gNMI spec, the clients can specify the server's supported encoding format for Get & Subscribe requests. Server resort to JSON encoding as default, when client do not specify any encoding value.

Thanks

[robshakir]

Hi,

TogNMINotifications marshals specifically using the TypedValue message in gNMI. This is compliant with the recommended behaviour for the Subscribe RPC in gNMI >0.5.0.

To marshal a struct to JSON, then you can use ygot.EmitJSON which will produce JSON output. It is clearly possible to add a function that packs this into a Notification message, but this also seems relatively trivial to achieve by the caller, such that I'm not sure of its value in a library function.

Kind regards, r.

[gsindigi]

Thanks @robshakir for the response. What you mentioned is right from the library function.

Perhaps, my interpretation of the gNMI spec lead me look for such additional argument.
I am using gNMI-0.60 and my interpretation of Encoding in Capabilities exchanged between client & server is, client can request the server to respond with _Notification_s of requisite Encoding e.g., byte_val, json_val, ascii_val etc. Then server would need to send the _Update_s with Notification embedded in corresponding Encoding type requested by client for Get/Subscribe calls. With such understanding, Irrespective of the underlying data-type e.g., uint64 (InterfaceStateCounters), the Update will always contain specified encoding typed_val like byte_val and client will deal with the received updates as per Encoding specified. Hence, I was trying to encode the Notification message and was hoping TogNMINotfications would be the right place to have taken care of it as its doing most of the bulk job in iterating and creating necessary Notifications. After converting _GoStruct_s to Notifications, I could not take care of the encoding type (per above understanding). Instead, I ended up re-inventing what TogNMINotifications is doing and take care of this while creating the Notification messages. Is this right interpretation? Or, am I heading in a wrong direction? Feel free to correct, either ways. Or simply put, is it sufficient to use TogNMINotfications returned Notification messages and send it to clients, irrespective of the Encoding specified? Kindly clarify.

<-- snip begin // Encoding defines the value encoding formats that are supported by the gNMI // protocol. These encodings are used by both the client (when sending Set // messages to modify the state of the target) and the target when serializing // data to be returned to the client (in both Subscribe and Get RPCs). // Reference: gNMI Specification Section 2.3 type Encoding int32

const ( Encoding_JSON Encoding = 0 Encoding_BYTES Encoding = 1 Encoding_PROTO Encoding = 2 Encoding_ASCII Encoding = 3 Encoding_JSON_IETF Encoding = 4 ) <-- snip end

[robshakir]

Today's implementation just uses the native protobuf types for leaf encodings in the TypedValue message in gNMI. We have historically used json_val here, whereby each leaf was marshalled to its JSON representation to be transported on the wire. This isn't currently supported by TogNMINotifications.

I would be open to a PR which adds an encoding option to the GNMINotificationsConfig argument. This would need to be handled by some rework in leavesToNotifications to cover the encoding type that is requested.

Today, our general expectation is that the native TypedValue fields are the right way to encode values in Notification messages. We have found that there is some operational pain with the use of JSON here. The only place I can see Encoding specification really being useful is when combined with aggregation.

Since the work here is pretty small - I'm happy to support this rather than force a whole alternate implementation. Please let me know if you'd like some guidance on sending a PR?

[gsindigi]

Even I too had narrowed down to GNMINotificationsConfig and leavesToNotifications as you mentioned above, so that Encoding can be passed down as a cfg argument along with others and be taken care of.

You mentioned about Encoding being useful when combined with aggregation. Do you see it appropriate to update the spec ? I am not sure (with current limited understanding), just checking. As I too don't see a real need for custom Encoding in Get & Subscribe responses and current TypedValue fields' approach is more appropriate.

To be current gNMI spec (gNMI-0.6.0) compliant and current implementation of TogNMINotifications, what should be Encoding server be publishing? Is it Encoding_PROTO alone?

On the PR front, share any guidance and pointers you may have.

[robshakir]

Yes, we have some outstanding changes to the spec. I think ensuring that leaf-only values are only serialised using the TypedValue fields is the right direction. The reason to have Encoding for Get is that a client may wish to specify what the serialisation of the underlying data source is -- should it be JSON, proto, -- so I don't see removing it there.

I think an implementation that only implements TypedValue for leaves sent via Subscribe is fine. We expect implementations to evolve over time, and this would be a reasonable limitation.

The best place to start for opening a PR is probably the contributing doc. I'm not sure if there's specific other questions you have?

[gsindigi]

Hi, would it be right to retain the existing support as is for Encoding type Encoding_PROTO ? If not, how does client should know when TypedValue fields are used? With spec mapping each encoding type to TypedValue field, is it not required to use only such encoded fields in the payloads? Referring to Structured data types section 2.3.1 example, I could see encoding being done in json_ietf_val and json_val based different path elements. Am not sure, whether my interpretation of Encoding type in Get & Subscribe is right. Perhaps, any example would help better understand.

[robshakir]

I think the existing behaviour is the correct behaviour for any Subscribe implementation that is using the new TypedValue fields.

We are transitioning away from JSON encoding for <key, value> pairs in Subscribe.

For a Get implementation, the creation of the Notification is much more trivial -- simply packing the prefix+path plus serialised payload, such that I think we would want another function for this.