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.

Sign up for GitHub

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

Jump to bottom

fix: pika exporter error output due to versions dismatch #2951

Merged
merged 3 commits into from
Nov 18, 2024
Merged

fix: pika exporter error output due to versions dismatch #2951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1834250
fix pika_exporter warnf not found valuename
buzhimingyonghu Nov 13, 2024
d681cf6
Remove duplicate logs and add version management analysis
buzhimingyonghu Nov 13, 2024
67e92bc
fix
buzhimingyonghu Nov 15, 2024
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
153 changes: 143 additions & 10 deletions tools/pika_exporter/exporter/metrics/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,141 @@ const (
)

type ParseOption struct {
Version *semver.Version
Extracts map[string]string
ExtractsProxy map[string][]int64
Info string
Version *semver.Version
Extracts map[string]string
ExtractsProxy map[string][]int64
Info string
CurrentVersion VersionChecker
}
type VersionChecker interface {
CheckContainsEmptyValueName(key string) bool
CheckContainsEmptyRegexName(key string) bool
InitVersionChecker()
}
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
}
Comment on lines +31 to +75
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize version checker implementation

The current implementation has several areas for improvement:

  1. Thread safety issues during lazy initialization (as noted in past review)
  2. Linear search performance could be improved using maps
  3. 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:

  1. Eliminates thread-safety issues by removing lazy initialization
  2. Improves lookup performance from O(n) to O(1) using maps
  3. Could be extracted into a base struct to reduce code duplication

Committable suggestion skipped: line range outside the PR's diff.


type VersionChecker350 struct {
EmptyValueName []string
EmptyRegexName []string
}

func (v *VersionChecker350) InitVersionChecker() {
if v.EmptyValueName == nil {
v.EmptyValueName = []string{
"cache_db_num",
"cache_status",
"cache_memory",
"hits_per_sec",
"slow_logs_count",
}
}
if v.EmptyRegexName == nil {
v.EmptyRegexName = []string{
"hitratio_per_sec",
}
}
}
func (v *VersionChecker350) CheckContainsEmptyValueName(key string) bool {
for _, str := range v.EmptyValueName {
if str == key {
return true
}
}
return false
}
func (v *VersionChecker350) CheckContainsEmptyRegexName(key string) bool {
for _, str := range v.EmptyRegexName {
if str == key {
return true
}
}
return false
}

type VersionChecker355 struct {
EmptyValueName []string
EmptyRegexName []string
}

func (v *VersionChecker355) InitVersionChecker() {
if v.EmptyValueName == nil {
v.EmptyValueName = []string{
"cache_db_num",
"cache_status",
"cache_memory",
"hits_per_sec",
}
}
if v.EmptyRegexName == nil {
v.EmptyRegexName = []string{
"hitratio_per_sec",
"keyspace_info_>=3.1.0",
"keyspace_info_all_>=3.3.3",
"binlog_>=3.2.0",
"keyspace_last_start_time",
"is_scaning_keyspace",
}
}
}
func (v *VersionChecker355) CheckContainsEmptyValueName(key string) bool {
for _, str := range v.EmptyValueName {
if str == key {
return true
}
}
return false
}
func (v *VersionChecker355) CheckContainsEmptyRegexName(key string) bool {
for _, str := range v.EmptyRegexName {
if str == key {
return true
}
}
return false
}

type Parser interface {
Expand Down Expand Up @@ -112,9 +243,10 @@ func (p *regexParser) Parse(m MetricMeta, c Collector, opt ParseOption) {

matchMaps := p.regMatchesToMap(s)
if len(matchMaps) == 0 {
log.Warnf("regexParser::Parse reg find sub match nil. name:%s", p.name)
if opt.CurrentVersion == nil || !opt.CurrentVersion.CheckContainsEmptyRegexName(p.name) {
log.Warnf("regexParser::Parse reg find sub match nil. name:%s", p.name)
}
}

extracts := make(map[string]string)
for k, v := range opt.Extracts {
extracts[k] = v
Expand All @@ -136,7 +268,6 @@ func (p *regexParser) regMatchesToMap(s string) []map[string]string {

multiMatches := p.reg.FindAllStringSubmatch(s, -1)
if len(multiMatches) == 0 {
log.Errorf("regexParser::regMatchesToMap reg find sub match nil. name:%s", p.name)
return nil
}

Expand Down Expand Up @@ -172,8 +303,11 @@ func (p *normalParser) Parse(m MetricMeta, c Collector, opt ParseOption) {

if m.ValueName != "" {
if v, ok := findInMap(m.ValueName, opt.Extracts); !ok {
log.Warnf("normalParser::Parse not found value. metricName:%s valueName:%s", m.Name, m.ValueName)
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

} else {
metric.Value = convertToFloat64(v)
}
Expand Down Expand Up @@ -208,7 +342,6 @@ func (p *timeParser) Parse(m MetricMeta, c Collector, opt ParseOption) {

if m.ValueName != "" {
if v, ok := findInMap(m.ValueName, opt.Extracts); !ok {
log.Warnf("timeParser::Parse not found value. metricName:%s valueName:%s", m.Name, m.ValueName)
return
} else {
t, err := convertTimeToUnix(v)
Expand All @@ -227,14 +360,14 @@ func (p *timeParser) Parse(m MetricMeta, c Collector, opt ParseOption) {
}

func findInMap(key string, ms ...map[string]string) (string, bool) {

for _, m := range ms {
if v, ok := m[key]; ok {
return v, true
}
}
return "", false
}

func trimSpace(s string) string {
return strings.TrimRight(strings.TrimLeft(s, " "), " ")
}
Expand Down
38 changes: 35 additions & 3 deletions tools/pika_exporter/exporter/pika.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,10 @@ func (e *exporter) collectInfo(c *client, ch chan<- prometheus.Metric) error {
return nil
})
parseOpt := metrics.ParseOption{
Version: version,
Extracts: extracts,
Info: info,
Version: version,
Extracts: extracts,
Info: info,
CurrentVersion: selectversion(version.Original()),
Comment on lines +255 to +258
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

}
for _, m := range metrics.MetricConfigs {
m.Parse(m, collector, parseOpt)
Expand All @@ -263,6 +264,37 @@ func (e *exporter) collectInfo(c *client, ch chan<- prometheus.Metric) error {
return nil
}

const (
VERSION_336 = "3.3.6"
VERSION_350 = "3.5.0"
VERSION_355 = "3.5.5"
)

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
}
Comment on lines +273 to +291
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve version selection logic and error handling

The function can be improved for better error handling and cleaner code:

  1. Return error to explain why version is unsupported
  2. Simplify the code by removing unnecessary variable
  3. 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.


// 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
}
Comment on lines +293 to +297
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
// 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)
}

func (e *exporter) collectKeys(c *client) error {
allKeys := append([]dbKeyPair{}, e.keys...)
keys, err := getKeysFromPatterns(c, e.keyPatterns, e.scanCount)
Expand Down
Loading