cisco_telemetry_mdt enhancement #8661
Conversation
|
This pull request introduces 1 alert when merging d17e725 into 910b726 - view on LGTM.com new alerts:
|
|
@jagularr do you mind take a look at the change? |
| v1 := c.propMap["auto-prop-xfrom"](field, value) | ||
| if v1 != nil { | ||
| return v1 | ||
| } |
There was a problem hiding this comment.
What do you return if v1 is nil? If I interpret the logic correctly you will return nil don't you? If so, you can simplify to
| v1 := c.propMap["auto-prop-xfrom"](field, value) | |
| if v1 != nil { | |
| return v1 | |
| } | |
| return c.propMap["auto-prop-xfrom"](field, value) |
and in turn remove the else below.
| } else { | ||
| //Now check path based conversion. | ||
| //If mapping is found then do the required transformation. | ||
| if c.nxpathMap[path] != nil { |
There was a problem hiding this comment.
Again invert the logic
| if c.nxpathMap[path] != nil { | |
| if c.nxpathMap[path] == nil { | |
| return nil | |
| } |
and save some indention level.
| return int(value.(uint32)) | ||
| case *telemetry.TelemetryField_Uint64Value: | ||
| return int64(value.(uint64)) |
There was a problem hiding this comment.
Please check the type assertions before using them as otherwise telegraf will panic in case the assertion doesn't hold. You maybe should even rely on val instead of value!?
| } //switch | ||
| return nil | ||
| case "string": | ||
| return (xformValueString(field)) |
| key := "show processes memory physical" | ||
| c.nxpathMap[key] = make(map[string]string, 1) | ||
| c.nxpathMap[key]["processname"] = "string" | ||
|
|
||
| return nil |
There was a problem hiding this comment.
How about
| key := "show processes memory physical" | |
| c.nxpathMap[key] = make(map[string]string, 1) | |
| c.nxpathMap[key]["processname"] = "string" | |
| return nil | |
| c.nxpathMap["show processes memory physical"] = map[string]string{"processname": "string"} | |
| return nil |
There was a problem hiding this comment.
Btw. If there cannot be an error, please remove the return values!
| return nil | ||
| } | ||
|
|
||
| func (c *CiscoTelemetryMDT) InitMemPhys() error { |
There was a problem hiding this comment.
Please make this function private. Same for all other functions only used within this plugin.
There was a problem hiding this comment.
All these functions are updating nxpathMap that's why they are defined with
func (c *CiscoTelemetryMDT) InitMemPhys()
c.nxpathMap[key]["processname"] = "string"
There was a problem hiding this comment.
Did you mean following changes to make function private
-func (c *CiscoTelemetryMDT) InitProc() error {
+func InitProc(c *CiscoTelemetryMDT) {
There was a problem hiding this comment.
No I want to make it func (c *CiscoTelemetryMDT) initMemPhys() error do it will definitively stay inside of this package. In case a function starts with lower-case it will not be visible from outside the package.
| func (c *CiscoTelemetryMDT) InitBgpV4() error { | ||
| key := "show bgp ipv4 unicast" | ||
| c.nxpathMap[key] = make(map[string]string, 1) | ||
| c.nxpathMap[key]["aspath"] = "string" | ||
|
|
||
| return nil | ||
| } |
| c.InitPower() | ||
| c.InitMemPhys() | ||
| c.InitBgpV4() | ||
| c.InitCpu() | ||
| c.InitResources() | ||
| c.InitPtpCorrection() | ||
| c.InitTrans() | ||
| c.InitIgmp() | ||
| c.InitVrfAll() | ||
| c.InitIgmpSnoop() | ||
| c.InitIgmpSnoopGroups() | ||
| c.InitIgmpSnoopGroupDetails() | ||
| c.InitIgmpSnoopGroupsSumm() | ||
| c.InitMrouter() | ||
| c.InitSnoopStats() | ||
| c.InitPimInterface() | ||
| c.InitPimNeigh() | ||
| c.InitPimRoute() | ||
| c.InitPimRp() | ||
| c.InitPimStats() | ||
| c.InitIntfBrief() | ||
| c.InitPimVrf() | ||
| c.InitIpMroute() | ||
| c.InitIpv6Mroute() | ||
| c.InitVpc() | ||
| c.InitBgp() | ||
| c.InitCh() | ||
| c.InitIntf() | ||
| c.InitProcsys() | ||
| c.InitProc() | ||
| c.InitBfd() | ||
| c.InitLldp() |
There was a problem hiding this comment.
As seemingly those functions cannot error remove the error from the function signature of all those functions to indicate this fact.
| c.InitBfd() | ||
| c.InitLldp() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Same here. No error so remove the error return value.
|
This pull request introduces 1 alert when merging 94d1e94 into a790529 - view on LGTM.com new alerts:
|
| case "address", "vrfName": | ||
| key := "nextHop/" + ff.Name | ||
| tags[key] = decodeTag(ff) | ||
| } | ||
| if value := decodeValue(ff); value != nil { | ||
| name := "nextHop/" + ff.Name | ||
| grouper.Add(measurement, tags, timestamp, name, value) | ||
| } |
There was a problem hiding this comment.
Do you mean
| case "address", "vrfName": | |
| key := "nextHop/" + ff.Name | |
| tags[key] = decodeTag(ff) | |
| } | |
| if value := decodeValue(ff); value != nil { | |
| name := "nextHop/" + ff.Name | |
| grouper.Add(measurement, tags, timestamp, name, value) | |
| } | |
| case "address", "vrfName": | |
| key := "nextHop/" + ff.Name | |
| tags[key] = decodeTag(ff) | |
| continue | |
| } | |
| if value := decodeValue(ff); value != nil { | |
| name := "nextHop/" + ff.Name | |
| grouper.Add(measurement, tags, timestamp, name, value) | |
| } |
as otherwise you process ff twice in case of address and vrfName...
There was a problem hiding this comment.
Can you please move all nxosValueXform* functions to a separate file (e.g. cisco_telemetry_nxos.go) that also belongs to package cisco_telemetry_mdt! This way the main file is not so cluttered with the conversion and init functions for the new feature.
Please also check the issue brought up by LGTM!
|
This pull request introduces 1 alert when merging 2da8907 into f888136 - view on LGTM.com new alerts:
|
| return nil | ||
| } | ||
| if _, ok := c.propMap[field.Name]; ok { | ||
| return (c.propMap[field.Name](field, value)) |
There was a problem hiding this comment.
Please remove the extra parentheses
| return (c.propMap[field.Name](field, value)) | |
| return c.propMap[field.Name](field, value) |
| } | ||
| if _, ok := c.propMap[field.Name]; ok { | ||
| return (c.propMap[field.Name](field, value)) | ||
| } else { |
There was a problem hiding this comment.
You don't require an else here as you return in the if path. Please remove the else and save another indention level.
|
@dsx1123 very good, almost there. Can you please move all new functions starting with |
|
@dsx1123 please fix the formatting errors using |
| @@ -50,13 +52,48 @@ type CiscoTelemetryMDT struct { | |||
|
|
|||
| // Internal state | |||
| aliases map[string]string | |||
| dmes map[string]string | |||
There was a problem hiding this comment.
Can you please rename this (e.g. to dmesFuncs or similar) or Dmes above to avoid confusion between the two.
| //xform Field to string | ||
| func xformValueString(field *telemetry.TelemetryField) string { | ||
| var str string | ||
| switch val := field.ValueByType.(type) { | ||
| case *telemetry.TelemetryField_StringValue: | ||
| if len(val.StringValue) > 0 { | ||
| return val.StringValue | ||
| } | ||
| case *telemetry.TelemetryField_Uint32Value: | ||
| str = strconv.FormatUint(uint64(val.Uint32Value), 10) | ||
| return str | ||
| case *telemetry.TelemetryField_Uint64Value: | ||
| str = strconv.FormatUint(val.Uint64Value, 10) | ||
| return str | ||
| case *telemetry.TelemetryField_Sint32Value: | ||
| str = strconv.FormatInt(int64(val.Sint32Value), 10) | ||
| return str | ||
| case *telemetry.TelemetryField_Sint64Value: | ||
| str = strconv.FormatInt(val.Sint64Value, 10) | ||
| return str | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Should this also move to the _util.go file?
| if path == "uint64 to int" { | ||
| c.propMap[dme] = nxosValueXformUint64Toint64 | ||
| } else if path == "uint64 to string" { | ||
| c.propMap[dme] = nxosValueXformUint64ToString | ||
| } else if path == "string to float64" { | ||
| c.propMap[dme] = nxosValueXformStringTofloat | ||
| } else if path == "string to uint64" { | ||
| c.propMap[dme] = nxosValueXformStringToUint64 | ||
| } else if path == "string to int64" { | ||
| c.propMap[dme] = nxosValueXformStringToInt64 | ||
| } else if path == "auto-float-xfrom" { | ||
| c.propMap[dme] = nxosValueAutoXformFloatProp | ||
| } else if dme[0:6] == "dnpath" { //path based property map | ||
| js := []byte(path) | ||
| var jsStruct NxPayloadXfromStructure | ||
|
|
||
| err := json.Unmarshal(js, &jsStruct) | ||
| if err == nil { | ||
| //Build 2 level Hash nxpathMap Key = jsStruct.Name, Value = map of jsStruct.Prop | ||
| //It will override the default of code if same path is provided in configuration. | ||
| c.nxpathMap[jsStruct.Name] = make(map[string]string, len(jsStruct.Prop)) | ||
| for _, prop := range jsStruct.Prop { | ||
| c.nxpathMap[jsStruct.Name][prop.Key] = prop.Value | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We should make this a switch statement
| if path == "uint64 to int" { | |
| c.propMap[dme] = nxosValueXformUint64Toint64 | |
| } else if path == "uint64 to string" { | |
| c.propMap[dme] = nxosValueXformUint64ToString | |
| } else if path == "string to float64" { | |
| c.propMap[dme] = nxosValueXformStringTofloat | |
| } else if path == "string to uint64" { | |
| c.propMap[dme] = nxosValueXformStringToUint64 | |
| } else if path == "string to int64" { | |
| c.propMap[dme] = nxosValueXformStringToInt64 | |
| } else if path == "auto-float-xfrom" { | |
| c.propMap[dme] = nxosValueAutoXformFloatProp | |
| } else if dme[0:6] == "dnpath" { //path based property map | |
| js := []byte(path) | |
| var jsStruct NxPayloadXfromStructure | |
| err := json.Unmarshal(js, &jsStruct) | |
| if err == nil { | |
| //Build 2 level Hash nxpathMap Key = jsStruct.Name, Value = map of jsStruct.Prop | |
| //It will override the default of code if same path is provided in configuration. | |
| c.nxpathMap[jsStruct.Name] = make(map[string]string, len(jsStruct.Prop)) | |
| for _, prop := range jsStruct.Prop { | |
| c.nxpathMap[jsStruct.Name][prop.Key] = prop.Value | |
| } | |
| } | |
| } | |
| switch path { | |
| case "uint64 to int": | |
| c.propMap[dme] = nxosValueXformUint64Toint64 | |
| case "uint64 to string": | |
| c.propMap[dme] = nxosValueXformUint64ToString | |
| case "string to float64": | |
| c.propMap[dme] = nxosValueXformStringTofloat | |
| case "string to uint64": | |
| c.propMap[dme] = nxosValueXformStringToUint64 | |
| case "string to int64": | |
| c.propMap[dme] = nxosValueXformStringToInt64 | |
| case "auto-float-xfrom" | |
| c.propMap[dme] = nxosValueAutoXformFloatProp | |
| default: | |
| if ! strings.HasPrefix(dme, "dnpath") { // not path based property map | |
| continue | |
| } | |
| var jsStruct NxPayloadXfromStructure | |
| if err := json.Unmarshal([]byte(path), &jsStruct); if err != nil { | |
| continue | |
| } | |
| // Build 2 level Hash nxpathMap Key = jsStruct.Name, Value = map of jsStruct.Prop | |
| // It will override the default of code if same path is provided in configuration. | |
| c.nxpathMap[jsStruct.Name] = make(map[string]string, len(jsStruct.Prop)) | |
| for _, prop := range jsStruct.Prop { | |
| c.nxpathMap[jsStruct.Name][prop.Key] = prop.Value | |
| } | |
| } |
| if msg.GetSubscriptionIdStr() != "" { | ||
| tags["subscription"] = msg.GetSubscriptionIdStr() | ||
| } |
There was a problem hiding this comment.
We can save a function call here
| if msg.GetSubscriptionIdStr() != "" { | |
| tags["subscription"] = msg.GetSubscriptionIdStr() | |
| } | |
| if msgID := msg.GetSubscriptionIdStr(); msgID != "" { | |
| tags["subscription"] = msgID | |
| } |
| if !isEVENT { | ||
| nxChildren = subfield | ||
| } else { | ||
| sub := subfield.Fields | ||
| if sub[0] != nil && sub[0].Fields[0].Name == "subscriptionId" && len(sub[0].Fields) >= 2 { | ||
| nxAttributes = sub[0].Fields[1].Fields[0].Fields[0].Fields[0].Fields[0].Fields[0] | ||
| } | ||
| } | ||
| //if nxAttributes == NULL then class based query. | ||
| if nxAttributes == nil { |
There was a problem hiding this comment.
It looks like nxChildren can sometimes not be set?
Required for all PRs:
Summary of Changes