KacperPerschke commented 6 months ago

The real question is at the end.

Introduction

Attempts to analyze the bytesConverter and removeCommas functions contained in the collector/utils.go file led me to the conclusion that:

The artifactory string containing the number is fragmented.
- Be aware of what the part separator is for the strings.Fields.
The first fragment is treated as a number.
- Remember that slices are indexed from 0.
The second fragment is interpreted as a multiplier.
- There are some minor inconsistencies.
The remaining fragments are ignored.

My desire

I would like the code to show what it does a little more clearly, and the comments to possibly explain why such decisions were made.

Data

During testing for #121, we collected some responses from artifactory and managed to group them into the cases below.

Pure number with comma as thousands separator.
- [0-9],[0-9][0-9][0-9],[0-9][0-9][0-9]
An integer, or with a decimal fraction and a multiplier not separated by a space.
- [0-9]GB
- [0-9][0-9]GB
- [0-9][0-9][0-9]GB
- [0-9]TB
- [0-9]%
- [0-9].[0-9]%
- [0-9].[0-9][0-9]%
- [0-9][0-9].[0-9]%
- [0-9][0-9].[0-9][0-9]%
Number without or with a decimal. Multiplier separated by a space.
- [0-9] bytes
- [0-9][0-9] bytes
- [0-9].[0-9][0-9] KB
- [0-9].[0-9][0-9] MB
- [0-9].[0-9][0-9] GB
- [0-9].[0-9][0-9] TB
- [0-9][0-9].[0-9][0-9] KB
- [0-9][0-9].[0-9][0-9] MB
- [0-9][0-9].[0-9][0-9] GB
- [0-9][0-9].[0-9][0-9] TB
- [0-9][0-9][0-9].[0-9][0-9] KB
- [0-9][0-9][0-9].[0-9][0-9] MB
- [0-9][0-9][0-9].[0-9][0-9] GB
A flower in a buttonhole.
- [0-9].[0-9][0-9] TB ([0-9].[0-9]%)
- [0-9].[0-9][0-9] TB ([0-9].[0-9][0-9]%)
- [0-9][0-9].[0-9][0-9] TB ([0-9][0-9].[0-9]%)
- [0-9][0-9].[0-9][0-9] TB ([0-9][0-9].[0-9][0-9]%)

Proposal of a new algorithm.

We use regexp to recognize the case and split it into elements.
If we encounter a case not covered by regexp, we return an error.
If the multiplier is %, we convert number to the range 0–1.
If we have the last case, we return both numbers.

The Question

Do we stay with the current algorithm or switch to a new one?

github-actions[bot] commented 5 months ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

KacperPerschke commented 5 months ago

This is a green light request. I'll be happy to implement it.

But I'm not sure whether this idea fits the program author's concept. Hence I prefer to ask.

peimanja commented 5 months ago

@KacperPerschke Thanks for the observation and putting this together. I would be open to switch to the new model. I might have time this weekend to look at some of the overdue issues and PRs but yeah feel free to raise a PR to switch to the new model

KacperPerschke commented 5 months ago

A proposition that I haven't yet integrated with the rest of the program. That's why I don't even push to my fork repo.

package collector

import (
    "fmt"
    "regexp"
    "strconv"
    "strings"
)

const (
    mulPercent = 0.01
    mulKB      = 1024
    mulMB      = mulKB * 1024
    mulGB      = mulMB * 1024
    mulTB      = mulGB * 1024
)

// For Data Driven Development see https://github.com/peimanja/artifactory_exporter/issues/124
const (
    pattNumThouComma  = `^(?P<number>[[:digit:]]{1,3}(?:,[[:digit:]]{3})*)$`
    pattNumMult       = `^(?P<number>[[:digit:]]+(?:\.[[:digit:]]{1,2})?)(?P<multiplier>%|[GT]B)$`
    pattNumSpaceMult  = `^(?P<number>[[:digit:]]+(?:\.[[:digit:]]{1,2})?) (?P<multiplier>bytes|[KMGT]B)$`
    pattTBytesPercent = `^(?P<tbytes>[[:digit:]]+(?:\.[[:digit:]]{1,2})?) TB \((?P<percent>[[:digit:]]{1,2}(?:\.[[:digit:]]{1,2})?)%\)$`
)

var (
    reNumThouComma  = regexp.MustCompile(pattNumThouComma)
    reNumMult       = regexp.MustCompile(pattNumMult)
    reNumSpaceMult  = regexp.MustCompile(pattNumSpaceMult)
    reTBytesPercent = regexp.MustCompile(pattTBytesPercent)
)

type convResult []float64

func (e *Exporter) convArtiNumberToEpoch(artiNum string) (convResult, error) {
    emptyResult := convResult{}
    e.logger.Debug(
        "Attempting to convert a string from artifactory representing a number.",
        "artifactory.number", artiNum,
    )
    switch {
    case reNumThouComma.MatchString(artiNum):
        res := extractNamedGroups(artiNum, reNumThouComma)
        f, err := e.convNumber(
            strings.Replace(res["number"], `,`, ``, -1),
        )
        if err != nil {
            return emptyResult, err
        }
        return convResult{f}, nil
    case reNumMult.MatchString(artiNum):
        res := extractNamedGroups(artiNum, reNumMult)
        f, err := e.convNumber(res["number"])
        if err != nil {
            return emptyResult, err
        }
        m, err := e.convMultiplier(res["multiplier"])
        if err != nil {
            return emptyResult, err
        }
        return convResult{f * m}, nil
    case reNumSpaceMult.MatchString(artiNum):
        res := extractNamedGroups(artiNum, reNumSpaceMult)
        f, err := e.convNumber(res["number"])
        if err != nil {
            return emptyResult, err
        }
        m, err := e.convMultiplier(res["multiplier"])
        if err != nil {
            return emptyResult, err
        }
        return convResult{f * m}, nil
    case reTBytesPercent.MatchString(artiNum):
        res := extractNamedGroups(artiNum, reTBytesPercent)
        b, err := e.convNumber(res["tbytes"])
        if err != nil {
            return emptyResult, err
        }
        p, err := e.convNumber(res["percent"])
        if err != nil {
            return emptyResult, err
        }
        return convResult{b * mulTB, p * mulPercent}, nil
    default:
        e.logger.Debug(
            "The arti number did not match known templates.",
            "artifactory.number", artiNum,
        )
        err := fmt.Errorf(`The string '%s' does not match any known patterns.`, artiNum)
        return emptyResult, err
    }
}

func (e *Exporter) convMultiplier(m string) (float64, error) {
    switch {
    case m == `%`:
        return mulPercent, nil
    case m == `bytes`:
        return 1, nil
    case m == `KB`:
        return mulKB, nil
    case m == `MB`:
        return mulMB, nil
    case m == `GB`:
        return mulGB, nil
    case m == `TB`:
        return mulTB, nil
    default:
        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 capturedGroups map[string]string

func extractNamedGroups(artiNum string, re *regexp.Regexp) capturedGroups {
    match := re.FindStringSubmatch(artiNum)
    list := make(capturedGroups)
    for i, name := range re.SubexpNames() {
        if i != 0 && name != "" {
            list[name] = match[i]
        }
    }
    return list
}

Please comment, does this lead the next developer by the hand?

KacperPerschke commented 4 months ago

Malice towards Java programmers — experience with artifactory and keycloak leads me to believe that inconsistency is their sacred duty.

Now in to meritum. Collecting further cases brought me to a conclusion. The ways in which the artifactory api gives numbers can be divided into two groups.

One number with or without a multiplier.

[0-9] bytes
[0-9]%
[0-9],[0-9][0-9][0-9],[0-9][0-9][0-9]
[0-9],[0-9][0-9][0-9].[0-9][0-9] MB
[0-9].[0-9]%
[0-9].[0-9][0-9] GB
[0-9].[0-9][0-9] KB
[0-9].[0-9][0-9] MB
[0-9].[0-9][0-9] TB
[0-9].[0-9][0-9]%
[0-9]GB
[0-9]TB
[0-9][0-9] bytes
[0-9][0-9].[0-9]%
[0-9][0-9].[0-9][0-9] GB
[0-9][0-9].[0-9][0-9] KB
[0-9][0-9].[0-9][0-9] MB
[0-9][0-9].[0-9][0-9] TB
[0-9][0-9].[0-9][0-9]%
[0-9][0-9]GB
[0-9][0-9][0-9].[0-9][0-9] GB
[0-9][0-9][0-9].[0-9][0-9] KB
[0-9][0-9][0-9].[0-9][0-9] MB
[0-9][0-9][0-9]GB

Two numbers formatted in a specific way.

[0-9].[0-9][0-9] TB ([0-9].[0-9]%)
[0-9].[0-9][0-9] TB ([0-9].[0-9][0-9]%)
[0-9][0-9].[0-9][0-9] TB ([0-9][0-9].[0-9]%)
[0-9][0-9].[0-9][0-9] TB ([0-9][0-9].[0-9][0-9]%)

135 follows.

KacperPerschke commented 4 months ago

@peimanja thank you for approving #135, but we should be aware of the effect of this code change.

I managed to check in Prometheus that the values representing percentages have changed from [0, 100] to [0.00, 1.00]. Please take into account this information.

I don't know Grafana well enough to suggest a change in the configuration of the dashboard supplied with the program. For this reason, I neglected to make a necessary correction. I'm sorry.

peimanja commented 4 months ago

no problem, we definitely want to avoid introducing a breaking change like that. So we should probably either revert that before the next release or fix it if possible.

Unfortunately I've been quite busy and don't have much time other than some weekends to take a look

KacperPerschke commented 3 months ago

I've changed grafana dashboard at my workplace. It was necessary to change "unit": "percent" to "unit": "percentunit". But I can't find "unit": "percent" in dashboard.json file.

peimanja commented 3 months ago

is it this ?