From f85164764fcd367da237085923c41abfd3c56b05 Mon Sep 17 00:00:00 2001 From: KAcper Perschke Date: Fri, 1 Mar 2024 16:54:19 +0100 Subject: [PATCH] Issue #124. (#135) * Issue #124. An attempt to clean up the code that converts Artifactory strings to numbers accepted in Prometheus. * Unit tests plus sonarcloud suggestion. * Names unification. --- collector/converter.go | 118 +++++++++++++++++++++++++++++++ collector/converter_internals.go | 54 ++++++++++++++ collector/converter_test.go | 96 +++++++++++++++++++++++++ collector/replication.go | 2 +- collector/storage.go | 21 ++++-- collector/system.go | 2 +- collector/utils.go | 86 ---------------------- 7 files changed, 286 insertions(+), 93 deletions(-) create mode 100644 collector/converter.go create mode 100644 collector/converter_internals.go create mode 100644 collector/converter_test.go delete mode 100644 collector/utils.go diff --git a/collector/converter.go b/collector/converter.go new file mode 100644 index 0000000..3efa377 --- /dev/null +++ b/collector/converter.go @@ -0,0 +1,118 @@ +package collector + +import ( + "fmt" + "regexp" + "strings" +) + +const ( + logDbgKeyArtNum = `artifactory.number` +) + +// convArtiToPromBool is a very interesting appendix. +// Something needs it, but what and why? +// It would be quite nice if it was written here why such a thing is needed. +func convArtiToPromBool(b bool) float64 { + if b { + return 1 + } + return 0 +} + +const ( + pattOneNumber = `^(?P[[:digit:]]{1,3}(?:,[[:digit:]]{3})*)(?:\.[[:digit:]]{1,2})? ?(?P%|bytes|[KMGT]B)?$` +) + +var ( + reOneNumber = regexp.MustCompile(pattOneNumber) +) + +func (e *Exporter) convArtiToPromNumber(artiNum string) (float64, error) { + e.logger.Debug( + "Attempting to convert a string from artifactory representing a number.", + logDbgKeyArtNum, artiNum, + ) + + if !reOneNumber.MatchString(artiNum) { + e.logger.Debug( + "The arti number did not match known templates.", + logDbgKeyArtNum, artiNum, + ) + err := fmt.Errorf( + `The string '%s' does not match pattern '%s'.`, + artiNum, + pattOneNumber, + ) + return 0, err + } + + groups := extractNamedGroups(artiNum, reOneNumber) + + // The following `strings.replace` is for those cases that contain a comma + // thousands separator. In other cases, unnecessary, but cheaper than if. + // Sorry. + f, err := e.convNumber( + strings.Replace(groups["number"], `,`, ``, -1), + ) + if err != nil { + return 0, err + } + + mAsString, mIsPresent := groups["multiplier"] + if !mIsPresent { + return f, nil + } + m, err := e.convMultiplier(mAsString) + if err != nil { + return 0, err + } + + return f * m, nil +} + +const ( + pattTBytesPercent = `^(?P[[:digit:]]+(?:\.[[:digit:]]{1,2})?) TB \((?P[[:digit:]]{1,2}(?:\.[[:digit:]]{1,2})?)%\)$` +) + +var ( + reTBytesPercent = regexp.MustCompile(pattTBytesPercent) +) + +func (e *Exporter) convArtiToPromSizeAndUsage(artiSize string) (float64, float64, error) { + e.logger.Debug( + "Attempting to convert a string from artifactory representing a number.", + logDbgKeyArtNum, artiSize, + ) + + if !reTBytesPercent.MatchString(artiSize) { + e.logger.Debug( + "The arti number did not match known templates.", + logDbgKeyArtNum, artiSize, + ) + err := fmt.Errorf( + `The string '%s' does not match '%s' pattern.`, + artiSize, + pattTBytesPercent, + ) + return 0, 0, err + } + + groups := extractNamedGroups(artiSize, reTBytesPercent) + + b, err := e.convNumber(groups["tbytes"]) + if err != nil { + return 0, 0, err + } + mulTB, _ := e.convMultiplier(`TB`) + size := b * mulTB + + p, err := e.convNumber(groups["percent"]) + if err != nil { + return 0, 0, err + } + mulPercent, _ := e.convMultiplier(`%`) + percent := p * mulPercent + + return size, percent, nil +} diff --git a/collector/converter_internals.go b/collector/converter_internals.go new file mode 100644 index 0000000..333b8d2 --- /dev/null +++ b/collector/converter_internals.go @@ -0,0 +1,54 @@ +package collector + +import ( + "fmt" + "math" + "regexp" + "strconv" +) + +var mulConvDriver = map[string]float64{ + `%`: 0.01, + `bytes`: 1, + `KB`: math.Exp2(10), + `MB`: math.Exp2(20), + `GB`: math.Exp2(30), + `TB`: math.Exp2(40), +} + +func (e *Exporter) convMultiplier(m string) (float64, error) { + mul, present := mulConvDriver[m] + if present { + return mul, nil + } + e.logger.Error( + "The string was not recognized as a known multiplier.", + "artifactory.number.multiplier", m, + ) + return 0, fmt.Errorf(`Could not recognise '%s; as multiplier`, m) +} + +func (e *Exporter) convNumber(n string) (float64, error) { + f, err := strconv.ParseFloat(n, 64) + if err != nil { + e.logger.Error( + "String not convertible to float64", + "string", n, + ) + return 0, err + } + return f, nil +} + +type reCaptureGroups map[string]string + +func extractNamedGroups(artiNum string, re *regexp.Regexp) reCaptureGroups { + match := re.FindStringSubmatch(artiNum) + groupsFound := make(reCaptureGroups) + for i, name := range re.SubexpNames() { + if i != 0 && name != "" { + groupsFound[name] = match[i] + } + } + return groupsFound +} diff --git a/collector/converter_test.go b/collector/converter_test.go new file mode 100644 index 0000000..3a9d950 --- /dev/null +++ b/collector/converter_test.go @@ -0,0 +1,96 @@ +package collector + +// The purpose of this test is to check whether the given regular pattern +// captures cases from the application logs? +// They are not complete nor comprehensive. +// They also don't test the negative path. +// Fell free to make them better. + +import ( + "testing" + + l "github.com/peimanja/artifactory_exporter/logger" +) + +var fakeExporter = Exporter{ + logger: l.New( + l.Config{ + Format: l.FormatDefault, + Level: l.LevelDefault, + }, + ), +} + +func TestConvNum(t *testing.T) { + tests := []struct { + input string + want float64 + }{ + { + input: `8 bytes`, + want: 8, + }, + { + input: `8,888.88 MB`, + want: 9319743488.00, + }, + { + input: `88.88 GB`, + want: 94489280512.00, + }, + { + input: `888.88 GB`, + want: 953482739712.00, + }, + } + for _, tc := range tests { + got, err := fakeExporter.convArtiToPromNumber(tc.input) + if err != nil { + t.Fatalf(`An error '%v' occurred during conversion.`, err) + } + if tc.want != got { + t.Fatalf(`Want %f, but got %f.`, tc.want, got) + } + } +} + +func TestConvTwoNum(t *testing.T) { + tests := []struct { + input string + want []float64 + }{ + { + input: `3.33 TB (3.3%)`, + want: []float64{3661373720494.080078, 0.033}, + }, + + { + input: `6.66 TB (6.66%)`, + want: []float64{7322747440988.160156, 0.0666}, + }, + + { + input: `11.11 TB (11.1%)`, + want: []float64{12215574184591.359375, 0.111}, + }, + + { + input: `99.99 TB (99.99%)`, + want: []float64{109940167661322.234375, 0.9999}, + }, + } + for _, tc := range tests { + gotSize, gotPercent, err := fakeExporter.convArtiToPromSizeAndUsage(tc.input) + if err != nil { + t.Fatalf(`An error '%v' occurred during conversion.`, err) + } + wantSize := tc.want[0] + if wantSize != gotSize { + t.Fatalf(`Problem with size. Want %f, but got %f.`, wantSize, gotSize) + } + wantPercent := tc.want[1] + if wantPercent != gotPercent { + t.Fatalf(`Problem with percentage. Want %f, but got %f.`, wantPercent, gotPercent) + } + } +} diff --git a/collector/replication.go b/collector/replication.go index cc79e9a..4360934 100644 --- a/collector/replication.go +++ b/collector/replication.go @@ -25,7 +25,7 @@ func (e *Exporter) exportReplications(ch chan<- prometheus.Metric) error { for metricName, metric := range replicationMetrics { switch metricName { case "enabled": - enabled := b2f(replication.Enabled) + enabled := convArtiToPromBool(replication.Enabled) repo := replication.RepoKey rType := strings.ToLower(replication.ReplicationType) rURL := strings.ToLower(replication.URL) diff --git a/collector/storage.go b/collector/storage.go index 9f77ded..add2d03 100644 --- a/collector/storage.go +++ b/collector/storage.go @@ -15,7 +15,7 @@ func (e *Exporter) exportCount(metricName string, metric *prometheus.Desc, count e.jsonParseFailures.Inc() return } - value, err := e.removeCommas(count) + value, err := e.convArtiToPromNumber(count) if err != nil { e.jsonParseFailures.Inc() e.logger.Error( @@ -38,7 +38,7 @@ func (e *Exporter) exportSize(metricName string, metric *prometheus.Desc, size s e.jsonParseFailures.Inc() return } - value, err := e.bytesConverter(size) + value, err := e.convArtiToPromNumber(size) if err != nil { e.jsonParseFailures.Inc() e.logger.Error( @@ -61,7 +61,11 @@ func (e *Exporter) exportFilestore(metricName string, metric *prometheus.Desc, s e.jsonParseFailures.Inc() return } - value, err := e.bytesConverter(size) + value, percent, err := e.convArtiToPromSizeAndUsage(size) + /* + * What should you use the percentage for? + * Maybe Issue #126? + */ if err != nil { e.jsonParseFailures.Inc() e.logger.Warn( @@ -75,6 +79,7 @@ func (e *Exporter) exportFilestore(metricName string, metric *prometheus.Desc, s logDbgMsgRegMetric, "metric", metricName, "value", value, + "percent", percent, ) ch <- prometheus.MustNewConstMetric(metric, prometheus.GaugeValue, value, fileStoreType, fileStoreDir, nodeId) } @@ -112,7 +117,7 @@ func (e *Exporter) extractRepo(storageInfo artifactory.StorageInfo) ([]repoSumma rs.FilesCount = float64(repo.FilesCount) rs.ItemsCount = float64(repo.ItemsCount) rs.PackageType = strings.ToLower(repo.PackageType) - rs.UsedSpace, err = e.bytesConverter(repo.UsedSpace) + rs.UsedSpace, err = e.convArtiToPromNumber(repo.UsedSpace) if err != nil { e.logger.Warn( "There was an issue parsing repo UsedSpace", @@ -125,7 +130,13 @@ func (e *Exporter) extractRepo(storageInfo artifactory.StorageInfo) ([]repoSumma if repo.Percentage == "N/A" { rs.Percentage = 0 } else { - rs.Percentage, err = e.removeCommas(repo.Percentage) + /* WARNING! + * Previous e.removeCommas have been returning float from range [0.0, 100.0] + * Actual convNumArtiToProm returns float from range [0.0, 1.0] + * The application's behavior in this matter requires + * close observation in the near future. + */ + rs.Percentage, err = e.convArtiToPromNumber(repo.Percentage) if err != nil { e.logger.Warn( "There was an issue parsing repo Percentage", diff --git a/collector/system.go b/collector/system.go index 774d91b..f72ce65 100644 --- a/collector/system.go +++ b/collector/system.go @@ -33,7 +33,7 @@ func (e *Exporter) exportSystem(license artifactory.LicenseInfo, ch chan<- prome for metricName, metric := range systemMetrics { switch metricName { case "healthy": - ch <- prometheus.MustNewConstMetric(metric, prometheus.GaugeValue, b2f(health.Healthy), health.NodeId) + ch <- prometheus.MustNewConstMetric(metric, prometheus.GaugeValue, convArtiToPromBool(health.Healthy), health.NodeId) case "version": ch <- prometheus.MustNewConstMetric(metric, prometheus.GaugeValue, 1, buildInfo.Version, buildInfo.Revision, buildInfo.NodeId) case "license": diff --git a/collector/utils.go b/collector/utils.go deleted file mode 100644 index 6764cc0..0000000 --- a/collector/utils.go +++ /dev/null @@ -1,86 +0,0 @@ -package collector - -import ( - "fmt" - "regexp" - "strconv" - "strings" -) - -const ( - mulKB = 1024 - mulMB = mulKB * 1024 - mulGB = mulMB * 1024 - mulTB = mulGB * 1024 -) - -// A one-time regex compilation is far cheaper -// than repeating on each function entry. -// `MustCompile` is used because regex value is hardcoded -// i.e. may have been previously verified by the author. -var regNumber = regexp.MustCompile("[^0-9.]+") - -func (e *Exporter) removeCommas(str string) (float64, error) { - e.logger.Debug("Removing other characters to extract number from string") - /* - * I am very concerned about the “magic” used here. - * The code does not in any way explain why this particular - * method of extracting content from the text was adopted. - * Kacper Perschke - */ - strArray := strings.Fields(str) - strTrimmed := regNumber.ReplaceAllString(strArray[0], "") - num, err := strconv.ParseFloat(strTrimmed, 64) - if err != nil { - e.logger.Debug( - "Had problem extracting number", - "string", str, - "err", err.Error(), - ) - return 0, err - } - e.logger.Debug( - "Successfully converted string to number", - "string", str, - "number", num, - ) - return num, nil -} - -func (e *Exporter) bytesConverter(str string) (float64, error) { - e.logger.Debug("Converting size to bytes") - num, err := e.removeCommas(str) - if err != nil { - return 0, err - } - var bytesValue float64 - if strings.Contains(str, "bytes") { - bytesValue = num - } else if strings.Contains(str, "KB") { - bytesValue = num * mulKB - } else if strings.Contains(str, "MB") { - bytesValue = num * mulMB - } else if strings.Contains(str, "GB") { - bytesValue = num * mulGB - } else if strings.Contains(str, "TB") { - bytesValue = num * mulTB - } else { - return 0, fmt.Errorf("Could not convert %s to bytes", str) - } - e.logger.Debug( - "Successfully converted string to bytes", - "string", str, - "value", bytesValue, - ) - return bytesValue, nil -} - -// b2f is a very interesting appendix. -// Something needs it, but what and why? -// It would be quite nice if it was written here why such a thing is needed. -func b2f(b bool) float64 { - if b { - return 1 - } - return 0 -}