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

Cisco MDT input: minor improvements and support for NX-OS telemetry extensions #6177

Merged
merged 4 commits into from
Sep 16, 2019

Conversation

sbyx
Copy link
Contributor

@sbyx sbyx commented Jul 29, 2019

This adds support for the platform-specific telemetry features of Cisco NX-OS platforms including support for gzip compression (via native GRPC compression) and chunking data over different GRPC messages as well as decoding logic for NX-OS DME and NX-API data structures over telemetry.

For the latter see as reference:

In addition the following other things are addressed:

  • Correctly close the listener in error case when setting up.
  • Simplify field-name generation by getting rid of passed around bytes.Buffer instances

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 31, 2019
@danielnelson danielnelson self-requested a review July 31, 2019 04:34
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Let's add some unit tests for the NX-OS message handling too.

if packet.TotalSize == 0 {
c.handleTelemetry(packet.Data)
} else if int(packet.TotalSize) <= c.MaxMsgSize {
chunkBuffer.Write(packet.Data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a bytes.Buffer, why not just send the packet.Data into handleTelemetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NX-OS can send data which is too large for a single GRPC message and if so uses "application layer" chunking where one telemetry message is reassembled from multiple GRPC messages.

for _, subfield := range field.Fields {
c.parseGPBKVField(subfield, name, path, timestamp, tags, fields)
}
} else if c.DecodeNXOS && len(field.Fields) > 0 { // NX-OS extended decoding logic
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we inspect the field and determine if it is NXOS data automatically? If so we could remove the option and handle it automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in latest update.

}
if rn, hasRN := values["rn"]; hasRN {
// Promote the relative name of the entry from a value to a key
tags[prefix] = fmt.Sprint(rn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using fmt.Sprint here can be a bit risky, since it is assuming the interface{} has the proper string representation. It would be better to create a helper function with a type switch that converts known types to a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in latest update.

@sbyx
Copy link
Contributor Author

sbyx commented Sep 6, 2019

I have reworked some of the code a bit for better clarity. I have also added a feature (EmbeddedTags) to address issue #6322 where the device emits unexpected nested content that contains additional keys.

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Here is my review, overall I think this code needs more work yet, some of the complexity is unavoidable due to the nested structure of the messages but we should try to flatten the processing so that it is as straightforward as we can get it.

@@ -32,6 +32,9 @@ The TCP dialout transport is supported on IOS XR (32-bit and 64-bit) 6.1.x and l
## Define aliases to map telemetry encoding paths to simple measurement names
[inputs.cisco_telemetry_mdt.aliases]
ifstats = "ietf-interfaces:interfaces-state/interface/statistics"

## Define (for certain nested telemetry measurements with embedded tags) which fields are tags
# enbedded_tags = ["Cisco-IOS-XR-qos-ma-oper:qos/interface-table/interface/input/service-policy-names/service-policy-instance/statistics/class-stats/class-name"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this option up above the aliases subtable so it doesn't fall into the table. Also s/enbedded_tags/embedded/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Fill extra tags
c.extraTags = make(map[string]map[string]struct{})
for _, tag := range c.EmbeddedTags {
i := strings.LastIndexByte(tag, '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to use path.Base here to get the last element of the path. You can also use path.Dir for everything up to the last item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

fields = make(map[string]interface{}, len(field.Fields))
for _, subfield := range field.Fields {
c.parseGPBKVField(subfield, &namebuf, telemetry.EncodingPath, timestamp, tags, fields)
if tags != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This flow is a bit confusing for me, it looks like there is some sort of expected ordering here: "keys" should come before "content"? To what degree is this guaranteed? Is it possible that "keys" is unset? Can there be more than one "keys" or "content" section? Are there other sections to ignore?

Not knowing these answers it is hard for me know how to fix, but one way I think this code to be more straightforward is that instead of looping over the fields and having a switch statement, have a function that gets the fields, then you can do the flow in a way that has less looping:

func GetTelemetryField(f *telemetry.Field, name string) *telemetry.Field
keysField, err := GetTelemetryField(gpvkb.Fields, "keys")
if err != nil {
	return err
}

tags :=

err := parseKeyField(keysField, tags)
if err != nil {
    return err
}

contentField, err := GetTelemetryField(gpvkb.Fields, "content")
if err != nil {
	return err
}

// tags were definitely found.
err := parseContentField(contentField, tags)
if err != nil {
    return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so far the assumption was there was a "keys"-field followed by a "content"-field which holds true for all datasets I have seen so far. But since it does not hurt I have now made a simple loop which just looks for these 2 fields afterwards a check if both have been found and an early continue if one or both are missing.

if len(name) == 0 {
name = prefix
} else if len(prefix) > 0 {
name = prefix + "/" + localname
Copy link
Contributor

Choose a reason for hiding this comment

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

What if localname = ""? Do we still want to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 371 here checks if localname is empty (name is set to localname one line before) but I will clarify.

localname := strings.Replace(field.Name, "-", "_", -1)
name := localname
if len(name) == 0 {
name = prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

What if prefix is "", is this still right? Is this to set the top "keys" field to ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Children of "keys" are iterated with prefix set to "". This branch is mainly necessary for NX-OS devices sending fields with empty name that have children. Without this we would get fieldnames with multiple /. See e.g. the format in the testcases for NX-OS decoding.

if fields != nil {
fields[namebuf.String()] = value
if tag := decodeTag(field); len(tag) > 0 {
localname := strings.Replace(field.Name, "-", "_", -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need done again? Isn't it the same as on L369?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope not necessary, will be removed.

if len(rn) > 0 {
tags[prefix] = rn
} else if !dn { // Check for distinguished name being present
c.acc.AddError(fmt.Errorf("NX-OS decoding failed: missing dn field"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should stop processing the attributes and return an error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I will add a return here. This is checking for a possible corner case I haven't encountered in practice yet. I will add a return statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So parsing can continue with the next valid field.


extraTags := c.extraTags[path+"/"+name]

if value := decodeValue(field); value != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an error, can we return early if decode fails? We should try to do this whenever we can, if the message isn't valid then lets skip it instead of trying to produce something which may not be right.

value, err := decodeValue(field)
if err != nil
    return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

value being nil is legitimate when the field has children. usually a field has either children or a value (e.g. a tree-structure where only leafs carry values). I have added a return statement inside this branch, i.e. if it is a value, stop processing children.

}

namebuf.Truncate(namelen)
if nxAttributes != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Return early if unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay reorganized this part a bit, to unindent some longer code branches.

@sbyx
Copy link
Contributor Author

sbyx commented Sep 14, 2019

Tried to address all open points with the new commit. Please let me know if anything still remains to be changed.

@russorat russorat added this to the 1.13.0 milestone Sep 16, 2019
@danielnelson danielnelson merged commit ee9d0fc into influxdata:master Sep 16, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants