fix: pika exporter error output due to versions dismatch #2951
Conversation
WalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
tools/pika_exporter/exporter/pika.go (2)
267-281: Add unit tests for version selection logicThe version selection logic is critical for the metrics collection functionality. Unit tests should be added to ensure correct behavior for various scenarios.
Would you like me to help generate comprehensive unit tests for the
selectversionfunction? The tests would cover:
- Valid versions (3.3.6, 3.5.0, 3.5.5)
- Invalid version formats
- Unsupported versions
- Edge cases
267-281: Consider a more maintainable version handling architectureThe current switch-based implementation might become unwieldy as more versions are supported. Consider these architectural improvements:
- Use a version registry pattern to dynamically register version checkers
- Implement version range support instead of exact version matching
- Consider using semantic versioning for more flexible version comparison
Example of a registry pattern:
type VersionRegistry struct { checkers map[string]metrics.VersionChecker } func (r *VersionRegistry) Register(version string, checker metrics.VersionChecker) { r.checkers[version] = checker } func (r *VersionRegistry) GetChecker(version string) metrics.VersionChecker { // Implementation with version range support }tools/pika_exporter/exporter/metrics/parser.go (5)
Line range hint
346-353: Handle errors appropriately inconvertTimeToUnixThe function
convertTimeToUnixreturns0, nilwhentime.Parsefails. This hides the parsing error from the caller and may lead to incorrect values being used without indication of the failure.Consider returning the error to the caller to allow proper error handling:
- return 0, nil + return 0, err
Line range hint
328-333: Propagate parsing errors intimeParser.ParseIn
timeParser.Parse, the error returned byconvertTimeToUnixis currently ignored. This can lead to incorrect metric values being used without awareness of the parsing failure.Modify the error handling to ensure parsing errors are addressed:
if err != nil { log.Warnf("Failed to parse time '%s': %v", v, err) return } metric.Value = float64(t)
Line range hint
435-437: Handle errors returned by recursive calls inStructToMapIn
StructToMap, the error returned by recursive calls is being ignored:subMap, subCmdMap, _ := StructToMap(value)Consider handling the error to capture and propagate any issues that may occur during recursion:
subMap, subCmdMap, err := StructToMap(value) if err != nil { return nil, nil, err }
Line range hint
372-378: Avoid usingpanicinmustNewVersionConstraintThe function
mustNewVersionConstraintusespanicto handle errors fromsemver.NewConstraint. Relying on panic for error handling can lead to unexpected crashes and is not recommended.Consider returning the error to the caller so it can be handled appropriately:
func newVersionConstraint(version string) (*semver.Constraints, error) { c, err := semver.NewConstraint(version) if err != nil { return nil, err } return c, nil }
Line range hint
299-308: Simplify conditional branching innormalParser.ParseThe
elseblock after thereturnstatement is unnecessary since the function will exit if the condition is met. Removing theelseblock can make the code cleaner and more readable.Refactor the code as follows:
if v, ok := findInMap(m.ValueName, opt.Extracts); !ok { if opt.CurrentVersion == nil || !opt.CurrentVersion.CheckContainsEmptyValueName(m.ValueName) { log.Warnf("normalParser::Parse not found value. metricName:%s valueName:%s", m.Name, m.ValueName) } return } metric.Value = convertToFloat64(v)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tools/pika_exporter/exporter/metrics/parser.go(4 hunks)tools/pika_exporter/exporter/pika.go(2 hunks)
🔇 Additional comments (6)
tools/pika_exporter/exporter/pika.go (1)
255-258:
Consider handling unsupported versions
The CurrentVersion field is initialized with the result of selectversion, which returns nil for unsupported versions. This might lead to nil pointer dereferences in the metrics parsing logic.
Let's verify the version handling in the metrics package:
Consider either:
- Setting a default version checker for unsupported versions
- Adding explicit nil checks before using CurrentVersion
tools/pika_exporter/exporter/metrics/parser.go (5)
60-72: Avoid data races in lazy initialization of VersionChecker fields
Similarly, the method CheckContainsEmptyRegexName in VersionChecker336 lazily initializes the EmptyRegexName slice, which can lead to data races under concurrent access.
79-95: Avoid data races in lazy initialization of VersionChecker fields
The method CheckContainsEmptyValueName in VersionChecker350 also performs lazy initialization of EmptyValueName, potentially causing data races if accessed concurrently.
96-108: Avoid data races in lazy initialization of VersionChecker fields
The method CheckContainsEmptyRegexName in VersionChecker350 lazily initializes EmptyRegexName, which might lead to data races with concurrent access.
115-130: Avoid data races in lazy initialization of VersionChecker fields
The method CheckContainsEmptyValueName in VersionChecker355 performs lazy initialization of EmptyValueName, which is not thread-safe under concurrent access.
131-147: Avoid data races in lazy initialization of VersionChecker fields
Similarly, CheckContainsEmptyRegexName in VersionChecker355 lazily initializes EmptyRegexName, causing potential data races.
tools/pika_exporter/exporter/pika.go
Outdated
| func selectversion(version string) metrics.VersionChecker { | ||
| var v metrics.VersionChecker | ||
|
|
||
| switch version { | ||
| case "3.3.6": | ||
| v = &metrics.VersionChecker336{} | ||
| case "3.5.5": | ||
| v = &metrics.VersionChecker355{} | ||
| case "3.5.0": | ||
| v = &metrics.VersionChecker350{} | ||
| default: | ||
| return nil | ||
| } | ||
| return v | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve version handling robustness
The current implementation has several areas for improvement:
- Hard-coded version strings are error-prone
- Limited version support might be too restrictive
- No validation of version string format
Consider these improvements:
+// Version constants to avoid typos and improve maintainability
+const (
+ VERSION_336 = "3.3.6"
+ VERSION_350 = "3.5.0"
+ VERSION_355 = "3.5.5"
+)
func selectversion(version string) metrics.VersionChecker {
- var v metrics.VersionChecker
+ // Validate version format
+ if !isValidVersion(version) {
+ log.Warnf("Invalid version format: %s", version)
+ return getDefaultVersionChecker()
+ }
switch version {
- case "3.3.6":
+ case VERSION_336:
return &metrics.VersionChecker336{}
- case "3.5.5":
+ case VERSION_355:
return &metrics.VersionChecker355{}
- case "3.5.0":
+ case VERSION_350:
return &metrics.VersionChecker350{}
default:
- return nil
+ // Use the closest supported version
+ return findClosestVersionChecker(version)
}
- return v
}
+// isValidVersion validates the version string format (e.g., x.y.z)
+func isValidVersion(version string) bool {
+ matched, _ := regexp.MatchString(`^\d+\.\d+\.\d+$`, version)
+ return matched
+}Committable suggestion skipped: line range outside the PR's diff.
| func (v *VersionChecker336) CheckContainsEmptyValueName(key string) bool { | ||
| if v.EmptyValueName == nil { | ||
| v.EmptyValueName = []string{ | ||
| "instantaneous_output_repl_kbps", | ||
| "total_net_output_bytes", | ||
| "cache_db_num", | ||
| "hits_per_sec", | ||
| "cache_status", | ||
| "total_net_input_bytes", | ||
| "instantaneous_output_kbps", | ||
| "instantaneous_input_kbps", | ||
| "total_net_repl_input_bytes", | ||
| "instantaneous_input_repl_kbps", | ||
| "slow_logs_count", | ||
| "total_net_repl_output_bytes", | ||
| "cache_memory", | ||
| } | ||
| } | ||
| for _, str := range v.EmptyValueName { | ||
| if str == key { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Avoid data races in lazy initialization of VersionChecker fields
The method CheckContainsEmptyValueName in VersionChecker336 performs lazy initialization of the EmptyValueName slice by modifying it during method calls. If instances of VersionChecker336 are accessed concurrently by multiple goroutines, this can lead to data races since the slice is modified without synchronization.
To fix this issue, consider initializing the EmptyValueName slice when creating the VersionChecker336 instance instead of during method calls. This ensures thread safety and avoids potential data races.
buzhimingyonghu
changed the title
Mixficsol
changed the title
chejinge
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
tools/pika_exporter/exporter/pika.go (1)
267-271: Improve version constants organization and documentationConsider these improvements for better maintainability:
- Add prefix to avoid naming conflicts
- Add documentation explaining version significance
const ( - VERSION_336 = "3.3.6" - VERSION_350 = "3.5.0" - VERSION_355 = "3.5.5" + // PIKA_VERSION_336 represents the first version with metrics support + PIKA_VERSION_336 = "3.3.6" + // PIKA_VERSION_350 introduces enhanced metrics collection + PIKA_VERSION_350 = "3.5.0" + // PIKA_VERSION_355 is the latest supported version with full metrics capability + PIKA_VERSION_355 = "3.5.5" )tools/pika_exporter/exporter/metrics/parser.go (3)
20-30: Add interface documentationThe
VersionCheckerinterface and its methods would benefit from documentation explaining:
- The purpose of the interface
- The contract for each method
- Expected behavior when a key is found/not found
Add documentation like this:
+// VersionChecker defines version-specific validation logic for metric names type VersionChecker interface { + // CheckContainsEmptyValueName returns true if the given key is expected to have + // an empty value for this version CheckContainsEmptyValueName(key string) bool + // CheckContainsEmptyRegexName returns true if the given key is expected to have + // empty regex matches for this version CheckContainsEmptyRegexName(key string) bool + // InitVersionChecker initializes the checker's internal state InitVersionChecker() }
115-154: Reconsider version numbers in regex namesIncluding version numbers in regex names (
keyspace_info_>=3.1.0,binlog_>=3.2.0, etc.) could make maintenance difficult as new versions are added.Consider:
- Using semantic version parsing to determine feature availability
- Creating a version-feature mapping configuration
- Implementing a more flexible feature detection mechanism
246-248: Enhance error messages with version contextWhen logging warnings about missing values or regex matches, include version information to help with debugging.
- log.Warnf("regexParser::Parse reg find sub match nil. name:%s", p.name) + log.Warnf("regexParser::Parse reg find sub match nil. name:%s, version:%v", p.name, opt.Version) - log.Warnf("normalParser::Parse not found value. metricName:%s valueName:%s", m.Name, m.ValueName) + log.Warnf("normalParser::Parse not found value. metricName:%s valueName:%s, version:%v", m.Name, m.ValueName, opt.Version)Also applies to: 306-308
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tools/pika_exporter/exporter/metrics/parser.go(4 hunks)tools/pika_exporter/exporter/pika.go(2 hunks)
🔇 Additional comments (2)
tools/pika_exporter/exporter/pika.go (1)
273-291: Verify version checker usage across codebase
Let's verify that all version checker usage patterns are consistent with these changes.
✅ Verification successful
Let me gather more specific information about the version checker implementations and their usage.
Based on the gathered information, I can now provide the final response:
Version checker implementation is consistent and properly encapsulated
The version checker implementation follows a clean interface pattern where:
- The interface and implementations are properly defined in
metrics/parser.go - Each version (3.3.6, 3.5.0, 3.5.5) has its own checker implementation with specific empty value handling
- Version selection is centralized in
selectversion()with proper validation - Usage is limited to metrics collection initialization with proper version constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version checker usage patterns across the codebase
# Find all files that might use version checkers
echo "Files potentially using version checkers:"
rg -l "VersionChecker|CurrentVersion" --type go
# Check for potential version string usage
echo -e "\nPotential version string usage:"
rg "3\.[0-9]+\.[0-9]+" --type go
# Look for other version-related functions
echo -e "\nOther version-related functions:"
ast-grep --pattern 'func $name($params) {
$$$
version
$$$
}'
Length of output: 8110
Script:
#!/bin/bash
# Check version checker implementations and usage
echo "Version checker implementations:"
rg -A 5 "type VersionChecker" tools/pika_exporter/exporter/metrics/
echo -e "\nVersion checker initialization and usage:"
rg -A 5 "InitVersionChecker|selectversion" --type go
echo -e "\nVersion constants and validation:"
rg "VERSION_|isValidVersion" tools/pika_exporter/exporter/pika.go
Length of output: 5288
tools/pika_exporter/exporter/metrics/parser.go (1)
77-113: Apply the same optimizations as VersionChecker336
The implementation has the same optimization opportunities as discussed for VersionChecker336.
| // isValidVersion validates the version string format (e.g., x.y.z) | ||
| func isValidVersion(version string) bool { | ||
| matched, _ := regexp.MatchString(`^\d+\.\d+\.\d+$`, version) | ||
| return matched | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize version validation with pre-compiled regex
The current implementation compiles the regex on every call and ignores potential compilation errors. Consider pre-compiling the regex for better performance and error handling.
+var versionRegex = regexp.MustCompile(`^\d+\.\d+\.\d+$`)
+
// isValidVersion validates the version string format (e.g., x.y.z)
func isValidVersion(version string) bool {
- matched, _ := regexp.MatchString(`^\d+\.\d+\.\d+$`, version)
- return matched
+ return versionRegex.MatchString(version)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // isValidVersion validates the version string format (e.g., x.y.z) | |
| func isValidVersion(version string) bool { | |
| matched, _ := regexp.MatchString(`^\d+\.\d+\.\d+$`, version) | |
| return matched | |
| } | |
| var versionRegex = regexp.MustCompile(`^\d+\.\d+\.\d+$`) | |
| // isValidVersion validates the version string format (e.g., x.y.z) | |
| func isValidVersion(version string) bool { | |
| return versionRegex.MatchString(version) | |
| } |
| Version: version, | ||
| Extracts: extracts, | ||
| Info: info, | ||
| CurrentVersion: selectversion(version.Original()), |
There was a problem hiding this comment.
Add error handling for unsupported versions
The CurrentVersion field might be nil if selectversion returns nil for unsupported versions. This could lead to nil pointer dereferences in the metrics parsing logic.
Consider adding error handling:
- CurrentVersion: selectversion(version.Original()),
+ CurrentVersion: selectversion(version.Original())
+ }
+ if parseOpt.CurrentVersion == nil {
+ return fmt.Errorf("unsupported Pika version: %s", version.Original())Committable suggestion skipped: line range outside the PR's diff.
| func selectversion(version string) metrics.VersionChecker { | ||
| if !isValidVersion(version) { | ||
| log.Warnf("Invalid version format: %s", version) | ||
| return nil | ||
| } | ||
| var v metrics.VersionChecker | ||
| switch version { | ||
| case VERSION_336: | ||
| v = &metrics.VersionChecker336{} | ||
| case VERSION_355: | ||
| v = &metrics.VersionChecker355{} | ||
| case VERSION_350: | ||
| v = &metrics.VersionChecker350{} | ||
| default: | ||
| return nil | ||
| } | ||
| v.InitVersionChecker() | ||
| return v | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve version selection logic and error handling
The function can be improved for better error handling and cleaner code:
- Return error to explain why version is unsupported
- Simplify the code by removing unnecessary variable
- Use the renamed constants
-func selectversion(version string) metrics.VersionChecker {
+func selectversion(version string) (metrics.VersionChecker, error) {
if !isValidVersion(version) {
log.Warnf("Invalid version format: %s", version)
- return nil
+ return nil, fmt.Errorf("invalid version format: %s", version)
}
- var v metrics.VersionChecker
switch version {
- case VERSION_336:
- v = &metrics.VersionChecker336{}
- case VERSION_355:
- v = &metrics.VersionChecker355{}
- case VERSION_350:
- v = &metrics.VersionChecker350{}
+ case PIKA_VERSION_336:
+ checker := &metrics.VersionChecker336{}
+ checker.InitVersionChecker()
+ return checker, nil
+ case PIKA_VERSION_355:
+ checker := &metrics.VersionChecker355{}
+ checker.InitVersionChecker()
+ return checker, nil
+ case PIKA_VERSION_350:
+ checker := &metrics.VersionChecker350{}
+ checker.InitVersionChecker()
+ return checker, nil
default:
- return nil
+ return nil, fmt.Errorf("unsupported version: %s", version)
}
- v.InitVersionChecker()
- return v
}Committable suggestion skipped: line range outside the PR's diff.
| type VersionChecker336 struct { | ||
| EmptyValueName []string | ||
| EmptyRegexName []string | ||
| } | ||
|
|
||
| func (v *VersionChecker336) InitVersionChecker() { | ||
| if v.EmptyValueName == nil { | ||
| v.EmptyValueName = []string{ | ||
| "instantaneous_output_repl_kbps", | ||
| "total_net_output_bytes", | ||
| "cache_db_num", | ||
| "hits_per_sec", | ||
| "cache_status", | ||
| "total_net_input_bytes", | ||
| "instantaneous_output_kbps", | ||
| "instantaneous_input_kbps", | ||
| "total_net_repl_input_bytes", | ||
| "instantaneous_input_repl_kbps", | ||
| "slow_logs_count", | ||
| "total_net_repl_output_bytes", | ||
| "cache_memory", | ||
| } | ||
| } | ||
| if v.EmptyRegexName == nil { | ||
| v.EmptyRegexName = []string{ | ||
| "hitratio_per_sec", | ||
| } | ||
| } | ||
| } | ||
| func (v *VersionChecker336) CheckContainsEmptyValueName(key string) bool { | ||
| for _, str := range v.EmptyValueName { | ||
| if str == key { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
| func (v *VersionChecker336) CheckContainsEmptyRegexName(key string) bool { | ||
| for _, str := range v.EmptyRegexName { | ||
| if str == key { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize version checker implementation
The current implementation has several areas for improvement:
- Thread safety issues during lazy initialization (as noted in past review)
- Linear search performance could be improved using maps
- Duplicate code pattern across version checkers
Consider these improvements:
type VersionChecker336 struct {
- EmptyValueName []string
- EmptyRegexName []string
+ EmptyValueName map[string]struct{}
+ EmptyRegexName map[string]struct{}
}
func (v *VersionChecker336) InitVersionChecker() {
- if v.EmptyValueName == nil {
- v.EmptyValueName = []string{
+ v.EmptyValueName = map[string]struct{}{
- "instantaneous_output_repl_kbps",
+ "instantaneous_output_repl_kbps": {},
// ... other values ...
}
- }
- if v.EmptyRegexName == nil {
- v.EmptyRegexName = []string{
+ v.EmptyRegexName = map[string]struct{}{
- "hitratio_per_sec",
+ "hitratio_per_sec": {},
}
- }
}
func (v *VersionChecker336) CheckContainsEmptyValueName(key string) bool {
- for _, str := range v.EmptyValueName {
- if str == key {
- return true
- }
- }
- return false
+ _, ok := v.EmptyValueName[key]
+ return ok
}This refactoring:
- Eliminates thread-safety issues by removing lazy initialization
- Improves lookup performance from O(n) to O(1) using maps
- Could be extracted into a base struct to reduce code duplication
Committable suggestion skipped: line range outside the PR's diff.
cheniujh
changed the title
cheniujh
changed the title
问题描述：
由于不同的版本字段不同，导致一直触发log
解决方案：
根据不同的版本,增加对应的处理方式
提取info种的version，根据version选择不同的版本
定义抽象接口,实现对多版本的控制。
接口的实现，定义了两个check方式，方便后续根据不同parse做补充
只有在opt.CurrentVersion ==nil and 当前字段不在当前版本的check列表才打印日志
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
collectInfomethod to include version validation and tailored handling for different Pika versions.Bug Fixes