Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions inputs/snmp_zabbix/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ func (c *SNMPCollector) collectFromAgent(ctx context.Context, agent string, item

func (c *SNMPCollector) collectIndividually(client *gosnmp.GoSNMP, agent string, items []MonitorItem, resultChan chan<- CollectionResult) {
for _, item := range items {
// Skip dependent items in individual collection, they are handled by their masters
if strings.EqualFold(item.Type, "dependent") {
continue
}

// 使用 Get 而不是 BulkGet
pduResult, err := client.Get([]string{item.OID})

Expand Down Expand Up @@ -220,11 +225,14 @@ func (c *SNMPCollector) processSingleResult(agent string, item MonitorItem, resu
Tags: c.buildTags(agent, item),
}

var masterValue interface{}

if result.Error != nil {
collectionResult.Error = result.Error
} else {
rawValue := c.convertSNMPValue(result.Value, item)
processedValue := rawValue

if len(item.Preprocessing) > 0 {
processed, err := ApplyPreprocessingWithContext(
rawValue,
Expand All @@ -243,6 +251,8 @@ func (c *SNMPCollector) processSingleResult(agent string, item MonitorItem, resu
}
}

masterValue = processedValue

finalValue, fields, err := c.processValue(processedValue, item)
if err != nil {
collectionResult.Error = err
Expand All @@ -259,6 +269,66 @@ func (c *SNMPCollector) processSingleResult(agent string, item MonitorItem, resu
}
}
resultChan <- collectionResult

if len(item.DependentItems) > 0 && result.Error == nil {
c.processDependentItems(agent, item.DependentItems, masterValue, resultChan)
Comment on lines +273 to +274

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

When master item collection fails, dependent items are silently skipped without any indication. Consider logging a message at debug level when dependent items are skipped due to master collection failure to aid in troubleshooting.

Suggested change
if len(item.DependentItems) > 0 && result.Error == nil {
c.processDependentItems(agent, item.DependentItems, masterValue, resultChan)
if len(item.DependentItems) > 0 {
if result.Error == nil {
c.processDependentItems(agent, item.DependentItems, masterValue, resultChan)
} else if c.config.DebugMode {
log.Printf("D! skipping %d dependent items for %s on agent %s due to master collection error: %v",
len(item.DependentItems), item.Key, agent, result.Error)
}

Copilot uses AI. Check for mistakes.
}
}

func (c *SNMPCollector) processDependentItems(agent string, dependents []MonitorItem, masterValue interface{}, resultChan chan<- CollectionResult) {
for _, depItem := range dependents {
Comment on lines +278 to +279

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

Inconsistent variable naming: The parameter is named dependents but elsewhere in the codebase uses DependentItems. Consider renaming to dependentItems for consistency with the field name and other similar variables in the codebase.

Suggested change
func (c *SNMPCollector) processDependentItems(agent string, dependents []MonitorItem, masterValue interface{}, resultChan chan<- CollectionResult) {
for _, depItem := range dependents {
func (c *SNMPCollector) processDependentItems(agent string, dependentItems []MonitorItem, masterValue interface{}, resultChan chan<- CollectionResult) {
for _, depItem := range dependentItems {

Copilot uses AI. Check for mistakes.
c.processSingleDependentItem(agent, depItem, masterValue, resultChan)
}
}

func (c *SNMPCollector) processSingleDependentItem(agent string, item MonitorItem, inputValue interface{}, resultChan chan<- CollectionResult) {
processedValue := inputValue
var err error

if len(item.Preprocessing) > 0 {
processedValue, err = ApplyPreprocessingWithContext(
inputValue,
item.Preprocessing,
c.preprocessingCtx,
item.Key,
agent,
)
}

collectionResult := CollectionResult{
Agent: agent,
Key: item.Key,
Time: time.Now(),
Tags: c.buildTags(agent, item),
}

if err != nil {
if c.config.DebugMode {
log.Printf("D! dependent preprocessing failed for %s: %v", item.Key, err)
}
Comment on lines +306 to +308

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

The error from preprocessing is checked but only logged in debug mode, then the function returns early without sending any result to the channel. This means dependent items with preprocessing errors will silently disappear from metrics. Consider sending an error result to the channel or at least logging at a higher level than debug.

Suggested change
if c.config.DebugMode {
log.Printf("D! dependent preprocessing failed for %s: %v", item.Key, err)
}
log.Printf("E! dependent preprocessing failed for %s: %v", item.Key, err)
collectionResult.Error = err
resultChan <- collectionResult

Copilot uses AI. Check for mistakes.

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

Incomplete error handling. When preprocessing fails for a dependent item, the function returns early without sending any error result through the channel. This means the caller won't be aware of the failure. Consider sending a CollectionResult with the error set before returning.

Suggested change
}
}
collectionResult.Error = err
resultChan <- collectionResult

Copilot uses AI. Check for mistakes.
return
}

finalValue, fields, err := c.processValue(processedValue, item)
if err != nil {
collectionResult.Error = err
} else {
if floatVal, convErr := ConvertToFloat64(finalValue); convErr == nil {
fields["value"] = floatVal
} else {
fields["value"] = finalValue
}

collectionResult.Value = finalValue
collectionResult.Fields = fields
collectionResult.Type = item.Type
}

resultChan <- collectionResult

if len(item.DependentItems) > 0 && err == nil {

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

Duplicate statement detected. The line resultChan <- collectionResult appears twice consecutively, which will send the same result to the channel twice. This will cause duplicate metrics to be collected for dependent items. Remove one of the duplicate lines.

Copilot uses AI. Check for mistakes.
c.processDependentItems(agent, item.DependentItems, processedValue, resultChan)
}
}

type BulkGetResult struct {
Expand Down
129 changes: 124 additions & 5 deletions inputs/snmp_zabbix/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ import (
"github.com/gosnmp/gosnmp"
)

const (
// DurationNever 代表永不执行 (-1s)
DurationNever = -1 * time.Second
// DurationImmediately 代表立即执行 (0s)
DurationImmediately = 0
)

type DiscoveryEngine struct {
client *SNMPClientManager
template *ZabbixTemplate
Expand Down Expand Up @@ -55,6 +62,9 @@ type MonitorItem struct {
DiscoveryIndex string // 从 {#SNMPINDEX} 等宏解析出的唯一索引

Preprocessing []PreprocessStep `json:"preprocessing,omitempty"`

// DependentItems 存储依赖于该 Item 的子 Item
DependentItems []MonitorItem
}

func extractLabelKey(itemKey string) string {
Expand Down Expand Up @@ -404,6 +414,10 @@ func (d *DiscoveryEngine) extractIndexFromOID(fullOID, baseOID string) string {
if !strings.HasPrefix(fullOID, baseOID) {
return ""
}
// 如果比 baseOID 长,必须紧跟切分符(.)
if len(fullOID) > len(baseOID) && fullOID[len(baseOID)] != '.' {
return ""
}

// 提取索引部分
if len(fullOID) <= len(baseOID) {
Expand Down Expand Up @@ -529,6 +543,12 @@ func (d *DiscoveryEngine) ApplyItemPrototypes(discoveries []DiscoveryItem, rule
var items []MonitorItem
prototypes := rule.ItemPrototypes

// Flat list to scope item creation per discovery iteration
type GeneratedItem struct {
MonitorItem
MasterKey string
}

for _, discovery := range discoveries {
discoveryIndex := ""
if idx, ok := discovery.Macros["{#SNMPINDEX}"]; ok {
Expand All @@ -538,9 +558,10 @@ func (d *DiscoveryEngine) ApplyItemPrototypes(discoveries []DiscoveryItem, rule
discoveryIndex = idx
}

var currentScopeItems []GeneratedItem

for _, prototype := range prototypes {
if prototype.Status == "DISABLED" ||
prototype.Status == "UNSUPPORTED" {
if prototype.Status == "DISABLED" || prototype.Status == "UNSUPPORTED" {
continue
}
delay := parseZabbixDelay(prototype.Delay)
Expand All @@ -549,8 +570,15 @@ func (d *DiscoveryEngine) ApplyItemPrototypes(discoveries []DiscoveryItem, rule
tags[tag.Tag] = tag.Value
}

// Expand macros in keys to properly link masters and dependents
expandedKey := d.expandMacros(prototype.Key, discovery.Macros)
expandedMasterKey := ""
if prototype.MasterItem.Key != "" {
expandedMasterKey = d.expandMacros(prototype.MasterItem.Key, discovery.Macros)
}

item := MonitorItem{
Key: d.expandMacros(prototype.Key, discovery.Macros),
Key: expandedKey,
OID: d.expandMacros(prototype.SNMPOID, discovery.Macros),
Type: ConvertZabbixItemType(prototype.Type),
Name: d.expandMacros(prototype.Name, discovery.Macros),
Expand All @@ -571,10 +599,55 @@ func (d *DiscoveryEngine) ApplyItemPrototypes(discoveries []DiscoveryItem, rule
default:
item.IsLabelProvider = false
}
items = append(items, item)

currentScopeItems = append(currentScopeItems, GeneratedItem{
MonitorItem: item,
MasterKey: expandedMasterKey,
})
}

// Map for O(1) lookups during dependency linkage
localMap := make(map[string]*MonitorItem)
var localList []*MonitorItem
var dependentList []*GeneratedItem

for i := range currentScopeItems {
ptr := &currentScopeItems[i].MonitorItem
localMap[ptr.Key] = ptr

if currentScopeItems[i].MasterKey != "" {
dependentList = append(dependentList, &currentScopeItems[i])
} else {
localList = append(localList, ptr)
}
}
}

depMap := make(map[string][]string)
for _, dep := range dependentList {
depMap[dep.MasterKey] = append(depMap[dep.MasterKey], dep.Key)
}

var buildTree func(key string) MonitorItem
buildTree = func(key string) MonitorItem {
item := *localMap[key]

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

Potential nil pointer dereference. The code dereferences localMap[key] without checking if the key exists in the map. If the key is not found, this will panic. Consider adding a check to ensure the key exists before dereferencing.

Suggested change
item := *localMap[key]
itemPtr, ok := localMap[key]
if !ok || itemPtr == nil {
// If the key is not present in the local map, return a zero-value item
return MonitorItem{}
}
item := *itemPtr

Copilot uses AI. Check for mistakes.
children := depMap[key]
for _, childKey := range children {
if _, exists := localMap[childKey]; exists {
childItem := buildTree(childKey)
item.DependentItems = append(item.DependentItems, childItem)
}
}
return item
}
Comment on lines +630 to +641

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

Potential stack overflow risk. The buildTree function is recursive and doesn't have any protection against circular dependencies. If the dependency graph contains a cycle (A depends on B, B depends on A), this will cause infinite recursion and a stack overflow. Add cycle detection or a visited set to prevent this.

Copilot uses AI. Check for mistakes.

// Only return root items; dependents are nested within them
for i := range currentScopeItems {
if currentScopeItems[i].MasterKey == "" {
root := buildTree(currentScopeItems[i].Key)
items = append(items, root)
}
}
}
return items
}

Expand Down Expand Up @@ -608,6 +681,52 @@ func parseZabbixDelay(delayStr string) time.Duration {
return 60 * time.Second
}

// ParseLLDLifetimes 解析 Zabbix 7.0+ 的生命周期配置
// 返回值: (deleteTTL, disableTTL)
// -1 (DurationNever): 永不
// 0 (DurationImmediately): 立即
// >0: 延迟执行的时长
func ParseLLDLifetimes(rule DiscoveryRule) (time.Duration, time.Duration) {
// 1. 解析 Delete 策略 (彻底删除)
deleteTTL := parseStrategy(rule.LifetimeType, rule.Lifetime, 7*24*time.Hour) // 默认删除时间 7d

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

The default value in the comment doesn't match the code. The comment states "默认删除时间 30d" (default deletion time 30d), but the code uses 7*24*time.Hour which is 7 days, not 30 days. Either update the comment to say "7d" or change the code to use 30 days if that's the intended default.

Suggested change
deleteTTL := parseStrategy(rule.LifetimeType, rule.Lifetime, 7*24*time.Hour) // 默认删除时间 7d
deleteTTL := parseStrategy(rule.LifetimeType, rule.Lifetime, 7*24*time.Hour) // 默认删除时间 7d

Copilot uses AI. Check for mistakes.

// 2. 解析 Disable 策略 (停止采集)
// 默认禁用策略:如果未配置,通常 Zabbix 行为是立即禁用(0)或者永不禁用。
// 这里我们设定:如果未显式配置,且旧版 logic (lifetime) 存在,则 Disable 默认为 0 (立即禁用,保持旧版行为一致性)
disableTTL := parseStrategy(rule.EnabledLifetimeType, rule.EnabledLifetime, DurationImmediately)

return deleteTTL, disableTTL
}

// 辅助函数:通用策略解析
// typeStr: DELETE_NEVER, DISABLE_AFTER 等
// durationStr: "7d", "1h"
// defaultValue: 当 typeStr 为空时的默认 duration
Comment thread
kongfei605 marked this conversation as resolved.
Outdated
func parseStrategy(typeStr, durationStr string, defaultValue time.Duration) time.Duration {
// 归一化
typeStr = strings.ToUpper(strings.TrimSpace(typeStr))

switch typeStr {
case "DELETE_NEVER", "DISABLE_NEVER":
return DurationNever
case "DELETE_IMMEDIATELY", "DISABLE_IMMEDIATELY":
return DurationImmediately
case "DELETE_AFTER", "DISABLE_AFTER":
if durationStr == "" {
return defaultValue
}
return parseZabbixDelay(durationStr)
default:
// 兼容旧版配置或空配置
// 如果 explicit type 为空,但 durationStr 有值,视为 AFTER
if typeStr == "" && durationStr != "" {
return parseZabbixDelay(durationStr)
}
// 否则返回默认值
return defaultValue
}
}
Comment on lines +689 to +737

Copilot AI Dec 23, 2025

Copy link

Choose a reason for hiding this comment

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

The new ParseLLDLifetimes function lacks test coverage. Given that this function is responsible for parsing critical lifecycle configuration with multiple edge cases (NEVER, IMMEDIATELY, AFTER with durations, empty values, fallback logic), it would benefit from comprehensive unit tests to ensure correct behavior across all scenarios.

Copilot uses AI. Check for mistakes.

func (d *DiscoveryEngine) expandMacros(text string, macros map[string]string) string {
result := text
for macro, value := range macros {
Expand Down
Loading
Loading