Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update gNMI proto encoding, remove decimal64 #151

Merged
merged 5 commits into from
Apr 29, 2022

Conversation

gcsl
Copy link
Contributor

@gcsl gcsl commented Nov 16, 2021

References Decimal64 deprecation document in #150

@gcsl gcsl requested a review from robshakir November 16, 2021 15:56
@gwizdms
Copy link

gwizdms commented Nov 16, 2021

Would it be possible to reference examples when choosing the different enum encodings?

@mirovr
Copy link

mirovr commented Nov 29, 2021

this redefines how PROTO encoding should be use in non backwards compatible manner (iso using any_val use scalar values).
So any implementation using previous interpretation needs to change (?).
Can be this new specification of PROTO encoding be named with different enum, so that is is clear which interpretation gnmi client is asking for?

@gcsl
Copy link
Contributor Author

gcsl commented Nov 29, 2021

We can look to add examples encodings.

This would be a backward incompatible change according to the existing wording. Are there existing implementations using the any_val today? I am not aware of any. One of the main downsides of any_val is that it will embed a full path to the proto definition for every instance of the any_val message, significantly increasing the overhead of delivering such a value. We have an ongoing discussion on an alternative that would eliminate this overhead that we are awaiting real need to codify in the specification. It certainly would be possible to use a different enum name if necessary. We are trying to clarify and clean up the document to remove unimplemented specifications.

@mirovr
Copy link

mirovr commented Dec 1, 2021

Carl, Thanks for your response.
Actually, we have working implementation (and customer) using "protobuf_bytes" as defined in https://github.com/openconfig/reference/blob/master/rpc/gnmi/protobuf-vals.md.
Could you please explain how this should work in compliance with the changes made in the specification? It is not quite clear to me.

many thanks!

Miro

@robshakir
Copy link
Contributor

Hi Miro,

One question -- today, we were intending protobuf_bytes to be used where a subtree is serialised into a binary protobuf (specification here). This subtree being serialised (rather than scalar values) was something that we expected to be allowed only where it was explicitly marked by the schema (atomic particularly). The justification for this is that subtrees generally should not be serialised in telemetry, since they do not have a single timestamp, and gNMI does not carry the timestamp per field.

To allow the subtree serialisation then we had a field (allow_aggregation) that specified that we would produce such values.

A few questions:

  1. Does your implementation use protobuf_val use this for parts of the tree that are not marked as atomic?
  2. Does your implementation use the allow_aggregation field at all?
  3. Is this support using the OpenConfig models, or other data models?

Cheers,
r.

@mirovr
Copy link

mirovr commented Jan 21, 2022

Hi Rob,

yes we use protobuf_bytes in "atomic" fashion. What I mean is that all leafs sent in a single notification have been collected at the same time per definition. If this would not be case, they would be sent as separate notifications.

to answer your remaining questions:

  1. Does your implementation use protobuf_val use this for parts of the tree that are not marked as atomic?
    our implementation does not use protobuf_val.

  2. Does your implementation use the allow_aggregation field at all?
    our implementation does not support "allow_aggregation"field

  3. Is this support using the OpenConfig models, or other data models?
    our implementation supports protobuf encoding for OpenConfig as well as our native models

I hope this answers your question. If not please let me know.

many thanks!

Miro

@robshakir
Copy link
Contributor

robshakir commented Jan 24, 2022

Miro,

Apologies -- I typoed protobuf_val when I meant protobuf_bytes.

If I understand you right, you're essentially aggregating multiple values together in a manner that does not consider either atomic or allow_aggregation -- I think today we had said that the schema and client explicitly must allow this (aggregating values). So setting allow_aggregation would be the mechanism by which a user would choose between having TypedValue that contains protobuf_bytes vs. those that only use the scalar types.

How do you tell whether the values can be aggregated -- is it based on the underlying schema (e.g.., OpenConfig's telemetry-atomic annotation) , or is it an internal implementation choice that isn't dependent upon the schema?

If we create a new enum type here that is specifically "scalar-only TypedValue" then I think we need to probably consider removing allow_aggregation since the user could choose PROTO encoding which would allow for aggregated values and scalar TypedValue, and then SCALAR_PROTO (or whatever we call it) which would allow for aggregation to be turned off.

Does this make sense?

r.

@osquitun
Copy link

Rob, Carl,

I have not seen response from Miro to Rob’s last comment, but since the same topic has continued to surface with other vendors, I wanted to provide some additional comparisons of observed implementations and comments as I don’t believe another enum type is required or desired when following the existing specification controls provide the desired results.

An item that has been observed across implementations is that various aggregation approaches have been implemented despite the fact that “allow_aggregration” has never been set. In fact, the current state of the yang modules suggests that aggregation is not technically allowed due to the on the lack of application of the “telemetry-atomic” definition. I have only found the use of atomic within the oc-message module for syslog.

While looking through different implementations it also became evident that the use of json encoding was not adhering to the same allow_aggregation and atomic controls if the aggregation behavior should be independent of the encoding used for streaming. The description and examples in 2.3.1 of the gNMI specification for json provide examples of serialization as if is aggregation was allowed and the schema had defined atomic. These examples appear to have been used by many as the specification to encode using json where aggregation and atomic had been defined. There have also been many discussions, comparison and confusion regarding comparison of proto and json that are now subject to question as they compared aggregation using json with scalar TypedValues using proto.

A remaining question is then the expectation for using protobuf.any encoding. If this is to remain an option, I might suggest adding it as part of an oc-extension to be exchanged using the Capabilities exchange and included in the Subscribe if accepted by the client and set by the target if aggregation and atomic allow for its use without need for yet another enum to distinguish between bytes and any.

Thanks for your consideration.

Eric

@mirovr
Copy link

mirovr commented Feb 16, 2022

Eric, Rob, Carl,
my apology for not responding to this thread earlier. I have reached to Rob and Carl in private for the clarification regarding different encoding options.
Coming back to this thread, it is true that we do aggregation regardless the allow_aggregation flag, but this is done only when client requests PROTO encoding in SubscribeRPC. In such a case we use proto_bytes. Rob and Carl have explained to me that this was intended to be used only when allow_aggregation flag is set. In case allow_aggregation is not set, scalar values should be used. We can adjust our implementation to observe this rule.
As far as what leafs are aggregated together, in our implementation this is driven by internal structures, rather than by relying on "atomic" tag in YANG models. This is mainly because what can be internally collected at the same time is given by the way our applications retrieve data internally.
Many thanks for the help with this.
Miro

Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, couple of minor changes.

rpc/gnmi/gnmi-specification.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-specification.md Outdated Show resolved Hide resolved
rpc/gnmi/gnmi-specification.md Outdated Show resolved Hide resolved
gcsl and others added 3 commits April 29, 2022 11:56
Co-authored-by: Rob Shakir <robjs@google.com>
Co-authored-by: Rob Shakir <robjs@google.com>
Copy link
Contributor

@robshakir robshakir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge as per previous approval :-)

@gcsl gcsl merged commit 98dffd1 into master Apr 29, 2022
@gcsl gcsl deleted the proto-encoding-remove-dec64 branch April 29, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants