refactor(snmp_zabbix): native javascript support #1376
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the snmp_zabbix input plugin to leverage native JavaScript JSON support from the goja library, simplifying the implementation by removing custom JSON handling code.
Key Changes:
- Removes custom
JSON.parseandJSON.stringifyimplementations that were manually injected into the JavaScript VM - Removes automatic unwrapping of Zabbix LLD JSON
{"data": [...]}format, simplifying value injection logic - Adds comprehensive test coverage for JavaScript preprocessing with JSON operations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| inputs/snmp_zabbix/preprocessing.go | Removed ~20 lines of custom JSON handling and LLD auto-unwrapping logic; simplified value injection to pass values directly to the JavaScript VM |
| inputs/snmp_zabbix/preprocessing_test.go | Added new test file with test cases for JSON.parse, JSON.stringify, and invalid JSON handling to verify native JavaScript JSON support works correctly |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scriptStringify := ` | ||
| var obj = {a: 1, b: "test"}; | ||
| return JSON.stringify(obj); | ||
| ` | ||
| resStr, err := applyJavaScript(nil, []string{scriptStringify}) | ||
| if err != nil { | ||
| t.Errorf("JSON.stringify failed: %v", err) | ||
| } | ||
| // Verify naive string contains keys | ||
| s := resStr.(string) | ||
| if !reflect.DeepEqual(s, `{"a":1,"b":"test"}`) && !reflect.DeepEqual(s, `{"b":"test","a":1}`) { | ||
| t.Errorf("Unexpected JSON string: %s", s) | ||
| } |
There was a problem hiding this comment.
Consider adding a test case for JSON.stringify with edge cases like arrays, nested objects, null values, and special characters to ensure the native JSON implementation handles all scenarios correctly. This would help verify that the transition from custom JSON implementation to native support is complete.
| t.Errorf("JSON.stringify failed: %v", err) | ||
| } | ||
| // Verify naive string contains keys | ||
| s := resStr.(string) |
There was a problem hiding this comment.
The type assertion on line 41 is unsafe and will panic if resStr is not a string. Consider adding a safety check similar to the one used for numeric types in the test above (lines 19-28), or use the comma-ok idiom to verify the type before accessing it as a string.
| s := resStr.(string) | |
| s, ok := resStr.(string) | |
| if !ok { | |
| t.Errorf("Unexpected result type for JSON.stringify: %T %v", resStr, resStr) | |
| return | |
| } |
| if err != nil { | ||
| t.Errorf("JSON.stringify failed: %v", err) | ||
| } | ||
| // Verify naive string contains keys |
There was a problem hiding this comment.
The comment says "Verify naive string contains keys" but the actual check uses reflect.DeepEqual to verify exact JSON string equality (with two possible key orderings). The comment should be updated to accurately describe what the test is doing, such as "Verify JSON string matches expected format with proper key-value serialization".
| // Verify naive string contains keys | |
| // Verify JSON string matches expected format with proper key-value serialization (allowing for key order differences) |
| } | ||
| // Verify naive string contains keys | ||
| s := resStr.(string) | ||
| if !reflect.DeepEqual(s, `{"a":1,"b":"test"}`) && !reflect.DeepEqual(s, `{"b":"test","a":1}`) { |
There was a problem hiding this comment.
Using reflect.DeepEqual to compare strings is unnecessarily complex and less efficient than a simple string equality check. Consider replacing this with a direct comparison: if s != {"a":1,"b":"test"}&& s !={"b":"test","a":1} { for better readability and performance.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !ok { | ||
| t.Fatalf("Expected string result, got %T %v", res, res) | ||
| } | ||
| // Verify naive string contains keys (order may vary) |
There was a problem hiding this comment.
The comment says "Verify naive string contains keys" but the verification is checking for exact JSON string matches. This is misleading since the code is actually performing exact string comparison, not a "naive" check. Consider updating the comment to be more accurate, such as "Check both possible JSON key orderings".
| // Verify naive string contains keys (order may vary) | |
| // Check both possible JSON key orderings |
| // 如果解析失败或 'data' 键不存在, valueToInject 将保持为原始字符串，这是安全的 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This PR removes the automatic LLD JSON unwrapping logic that previously extracted the 'data' field from Zabbix LLD JSON format. Users who have JavaScript preprocessing steps that rely on this auto-unwrapping behavior will need to manually parse the JSON and access the 'data' field. Consider documenting this breaking change in the commit message or README, and providing migration guidance for existing users.
| // 兼容逻辑：如果传入的是 Zabbix LLD JSON 字符串，自动展开其中的 "data" 字段 | |
| if strVal, ok := value.(string); ok { | |
| var decoded map[string]interface{} | |
| if uerr := json.Unmarshal([]byte(strVal), &decoded); uerr == nil { | |
| if data, ok := decoded["data"]; ok { | |
| value = data | |
| } | |
| } | |
| } |
1 participant
No description provided.