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

Add gNMI Extension field parsing support #509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ichabond
Copy link

gNMI allows for the use of an extension field in each top-level message of the gNMI RPCs: https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-extensions.md Given this is an arbitrary Protobyte payload, the default gNMI protobufs can't decode the payload contained within the field. This PR adds the necessary configuration options to load in an arbitrary Protobuf file per extension identifier, with a message-name to lookup the message type.

In this change:

  1. A new function DecodeExtension is added. This function uses protoreflect to dynamically marshal arbitrary protoBytes into JSON. The loaded JSON is then put back in the Extension message as bytes (mainting type)
  2. The Target type has an ExtensionProtoMap added, allowing for the lookup of Extension IDs to loaded-in protobufs
  3. The required changes to the TargetConfig type to support loading in the new configuration
  4. Modified collector.go to output the gNMI message after inlining the decoded protoBytes
  5. Loading in the protobufs was added to app/target.go: parseExtensionProtos. This uses the Parser provided by protoreflect/desc/protoparse
  6. Added functionality to event.go to insert the K/Vs provided by the Extension as Tags. Given we come from JSON, all numbers are float64, so the only 2 types supported currently are string and float64
  7. Minor helper function to turn the arbitrary JSON into an arbitrary map.

This has been tested with a device emiting an extension field:

[gnmic] target "edge01_test01": gNMI Subscribe Response: &{
SubscriptionName:port_stats
SubscriptionConfig:{"name":"port_stats","paths":["/interfaces/interface/"],"mode":"STREAM","stream-mode":"SAMPLE","encoding":"JSON","sample-interval":15000000000,"heartbeat-interval":15000000000,"outputs":["prom-scrape-output"]}
Response:
update:{timestamp:1723653363502452302  prefix:{elem:{name:"interfaces"}  elem:{name:"interface"  key:{key:"name"  value:"et-1/0/3"}}}
update:{path:{elem:{name:"state"}  elem:{name:"hardware-port"}}  val:{json_val:"\"FPC1:PIC0:PORT3\""}}
update:{path:{elem:{name:"state"}  elem:{name:"transceiver"}}  val:{json_val:"\"FPC1:PIC0:PORT3:Xcvr0\""}}}
extension:{registered_ext:{id:1
    msg:"{\"systemId\":\"edge01_test01\",\"componentId\":65535,\"sensorName\":\"sensor_1005_2_1\",\"subscribedPath\":\"/interfaces/interface/\",\"streamedPath\":\"/interfaces/interface/\",\"component\":\"chassisd\",\"sequenceNumber\":\"770002\",\"payloadGetTimestamp\":\"1723653363502\",\"streamCreationTimestamp\":\"1723653361858\",\"exportTimestamp\":\"1723653363504\",\"streamId\":\"PERIODIC\"}"}}}

Which is then properly rendered to a Prometheus metric:

gnmi_interfaces_interface_state_hardware_port{component="chassisd",componentId="65535",hardware_port="FPC1:PIC0:PORT3",interface_name="et-1/0/3",metric_source="edge01_test01",subscription_name="port_stats",systemId="edge01_test01"} 1

Note that some label-drop rules have been added to remove the spurious labels to avoid a cardinality explosion.

Copy link

google-cla bot commented Aug 14, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@Ichabond
Copy link
Author

This is a proposed solution for #503

gNMI allows for the use of an `extension` field in each top-level message of the gNMI RPCs: https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-extensions.md
Given this is an arbitrary Protobyte payload, the default gNMI protobufs can't decode the payload contained within the field.
This PR adds the necessary configuration options to load in an arbitrary Protobuf file per `extension` identifier, with a message-name to lookup the message type.

In this change:
1. A new function `DecodeExtension` is added. This function uses `protoreflect` to dynamically marshal arbitrary protoBytes into JSON. The loaded JSON is then put back in the Extension message as bytes (mainting type)
2. The `Target` type has an `ExtensionProtoMap` added, allowing for the lookup of Extension IDs to loaded-in protobufs
3. The required changes to the `TargetConfig` type to support loading in the new configuration
4. Modified `collector.go` to output the gNMI message _after_ inlining the decoded protoBytes
5. Loading in the protobufs was added to `app/target.go`: `parseExtensionProtos`. This uses the `Parser` provided by `protoreflect/desc/protoparse`
6. Added functionality to `event.go` to insert the K/Vs provided by the Extension as Tags. Given we come from JSON, all numbers are float64, so the only 2 types supported currently are `string` and `float64`
7. Minor helper function to turn the arbitrary JSON into an arbitrary map.

This has been tested with a device emiting an `extension` field:
```
[gnmic] target "edge01_test01": gNMI Subscribe Response: &{
SubscriptionName:port_stats
SubscriptionConfig:{"name":"port_stats","paths":["/interfaces/interface/"],"mode":"STREAM","stream-mode":"SAMPLE","encoding":"JSON","sample-interval":15000000000,"heartbeat-interval":15000000000,"outputs":["prom-scrape-output"]}
Response:
update:{timestamp:1723653363502452302  prefix:{elem:{name:"interfaces"}  elem:{name:"interface"  key:{key:"name"  value:"et-1/0/3"}}}
update:{path:{elem:{name:"state"}  elem:{name:"hardware-port"}}  val:{json_val:"\"FPC1:PIC0:PORT3\""}}
update:{path:{elem:{name:"state"}  elem:{name:"transceiver"}}  val:{json_val:"\"FPC1:PIC0:PORT3:Xcvr0\""}}}
extension:{registered_ext:{id:1
    msg:"{\"systemId\":\"edge01_test01\",\"componentId\":65535,\"sensorName\":\"sensor_1005_2_1\",\"subscribedPath\":\"/interfaces/interface/\",\"streamedPath\":\"/interfaces/interface/\",\"component\":\"chassisd\",\"sequenceNumber\":\"770002\",\"payloadGetTimestamp\":\"1723653363502\",\"streamCreationTimestamp\":\"1723653361858\",\"exportTimestamp\":\"1723653363504\",\"streamId\":\"PERIODIC\"}"}}}
```
Which is then properly rendered to a Prometheus metric:
```
gnmi_interfaces_interface_state_hardware_port{component="chassisd",componentId="65535",hardware_port="FPC1:PIC0:PORT3",interface_name="et-1/0/3",metric_source="edge01_test01",subscription_name="port_stats",systemId="edge01_test01"} 1
```
Note that some label-drop rules have been added to remove the spurious labels to avoid a cardinality explosion.
@Ichabond Ichabond force-pushed the tstrickx/AddExtensionSupport branch from bbe282d to a5e0eaa Compare August 14, 2024 18:44
@Ichabond
Copy link
Author

Updated the commit to fix the type assertion issue as well as the variable assignment issue.

@Ichabond
Copy link
Author

Anything else needed here @karimra ?

Comment on lines +170 to +172
if len(t.Config.RegisteredExtensions) == 0 {
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move this to the beginning of the function

Comment on lines +83 to +85
if a.Config.Debug {
a.Logger.Printf("target %q: gNMI Subscribe Response: %+v", t.Config.Name, rsp)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this back under the case statement to that the debug logging always happens regardless of decoding succeeding or not ?

Comment on lines +53 to +66
for _, ext := range response.Extension {
extensionValues, err := extensionToMap(ext)
if err != nil {
return nil, err
}
for k, v := range extensionValues {
switch v := v.(type) {
case string:
prefixTags[k] = v
case float64:
prefixTags[k] = strconv.FormatFloat(v, 'G', -1, 64)
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part assumes that any extension data can and will be mapped to a map[string]any, and that the goal is to get the extension data as tags, it will not always be the case for all extensions.

if err != nil {
return err
}
extension.GetRegisteredExt().Msg = jsondata
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it's a good idea to overwrite the extension Msg with the jsondata here. It makes more sense to return the resulting json and pass it on to the outputs as metadata.

@karimra
Copy link
Collaborator

karimra commented Aug 15, 2024

While this works for that junos extension, I'm not sure it will for any other extension.
The data in an extension is not always meant to be added as tags, we should find a way to pass it on to the output and use processors to select some keys to set as tags.
The other thing I'm not entirely convinced by is that these proto files are set by target. If you have 100s of targets of the same type (same extensions), we will have to parse the proto files 100s of times. This is fine for now, but it should be improved/fixed in a future PR.

Comment on lines +70 to 82
// decode gNMI extensions
if extensions := rsp.Response.Extension; len(extensions) > 0 {
err := t.DecodeExtension(rsp.Response)
if err != nil {
a.Logger.Printf("target %q: failed to decode extension field: %v", t.Config.Name, err)
continue
}
}
err := t.DecodeProtoBytes(rsp.Response)
if err != nil {
a.Logger.Printf("target %q: failed to decode proto bytes: %v", t.Config.Name, err)
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't drop the subscribe response if we fail to decode the extension.

@Ichabond Ichabond force-pushed the tstrickx/AddExtensionSupport branch from d007ce8 to a5e0eaa Compare August 30, 2024 08:35
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.

2 participants