Draft: Statistics monitoring #100
Conversation
Skantes
force-pushed
the
monitoring
branch
3 times, most recently
from
052e44c
to
14828b6
Compare
Skantes
force-pushed
the
monitoring
branch
2 times, most recently
from
8c6c383
to
46575ae
Compare
| for i, v := range values { | ||
| value, okValue := v.([]byte) | ||
| if !okValue { | ||
| return nil, errors.New("invalid type for file") |
There was a problem hiding this comment.
Probably not the error you want to return.
| return nil, err | ||
| } | ||
|
|
||
| files := make([]string, len(values)) |
There was a problem hiding this comment.
You probably want countries more than files.
database/utils.go
Outdated
| return err | ||
| } | ||
| defer conn.Close() | ||
| conn.Do("SADD", "COUNTRIES", country) |
There was a problem hiding this comment.
I would probably return the error to the calling function.
| if clientInfo.Country != "" { | ||
| err = h.redis.AddCountry(clientInfo.Country) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Careful here with your scope. Since err = h.redis.AddCountry(clientInfo.Country) is within a totally different scope, you may end up checking a previous error that was already handled.
http/stats.go
Outdated
| if m.Name == "" { | ||
| return errUnknownMirror | ||
| } | ||
| if fileinfo.Path == "" { | ||
| return errEmptyFileError | ||
| } | ||
| if clientInfo.Country == "" { | ||
| return errUnknownCountry |
There was a problem hiding this comment.
Nope. The country might be empty in various cases (in a unknown IP block for example). You can't assume that you'll always have a country in clientInfo.
| metrics.Year, _ = redis.Int(stats[index+3], err) | ||
| output += statsToPrometheusFormat(metrics, "country", name) | ||
| index += 4 | ||
| // Get all download count information from the previous redis querries |
http/metrics.go
Outdated
| func removeOccurence(slice []string, value string) []string { | ||
| for i, val := range slice { | ||
| if val == value { | ||
| if i < len(slice)-1 { | ||
| copy(slice[i:], slice[i+1:]) | ||
| } | ||
| slice[len(slice)-1] = "" | ||
| return slice[:len(slice)-1] | ||
| } | ||
| } | ||
| return slice | ||
| } |
There was a problem hiding this comment.
If the slice gets pretty big you can probably squeeze some CPU cycles by using a sorted slice during insertion and deletion instead of looping on the whole slice.
http/metrics.go
Outdated
| if i < len(slice)-1 { | ||
| copy(slice[i:], slice[i+1:]) | ||
| } | ||
| slice[len(slice)-1] = "" |
There was a problem hiding this comment.
This is not needed because of the resize below.
config/config.go
Outdated
| @@ -165,6 +168,9 @@ func ReloadConfig() error { | |||
| if c.RepositoryScanInterval < 0 { | |||
| c.RepositoryScanInterval = 0 | |||
| } | |||
| if c.MetricsTopFilesRetention < 1 { | |||
| return fmt.Errorf("Invalid retention duration in days") | |||
There was a problem hiding this comment.
What if you don't care about this feature? I would allow 0 and disable the feature entirely.
cli/commands.go
Outdated
| @@ -99,11 +99,14 @@ func (c *cli) CmdHelp() error { | |||
| help += fmt.Sprintf("CLI commands:\n") | |||
| for _, command := range [][]string{ | |||
| {"add", "Add a new mirror"}, | |||
| {"addMetric", "Add a tracked file to the metrics route"}, | |||
There was a problem hiding this comment.
meh. Can you add a metrics subcommand instead?
There was a problem hiding this comment.
About your last commit Metrics: Add auto tracked files mode:
This entire feature is awkward. If you want to track past and future files without manual intervention, modify your metrics add to take a pattern instead of a filename; or just track all files. But this feature will only lead to confusion and tears.
|
Please rebase @Skantes |
Add entries in the database to count downloads per country. This is not accessible through the stats endpoint. This is intended for a future metrics integration.
This adds a metrics route to monitoring metrics using prometheus. It exports mirror and files stats, as well as the newly introduced country downloads counter.
This adds a more detailed metrics category for files served by mirrorbits. These tracked files will detail their downloads per country, but due to the increased number of fields required for this, only files selected through the command line will be tracked. This also adds command line arguments to add, delete, and list the files to monitore closely.
Add export of current daily download stats for all files that have been downloaded in the current day. With this is also added a retention period for these stats as they can be heavy for the database and should be removed after use.
When enabled, files will be automatically added to tracked files.
This is to return a 503 Service Unavailable error instead of a 404 Not Found, a more helpfull error.
The aim of this pull request is to add statistics that can be monitored through prometheus as time-series metrics.