http, logs: only count GET requests as downloads. #171
Conversation
Other request methods such as POST or HEAD or anything else than GET should not be counted as a new download.
| dlogger.l.Printf("%s %d \"%s\" ip:%s mirror:%s%s %sasn:%d distance:%skm countries:%s", | ||
| typ, statuscode, p.FileInfo.Path, p.IP, m.Name, fallback, sameASNum, m.Asnum, distance, countries) | ||
| dlogger.l.Printf("%s %d \"%s\" method:%s ip:%s mirror:%s%s %sasn:%d distance:%skm countries:%s", | ||
| typ, statuscode, p.FileInfo.Path, method, p.IP, m.Name, fallback, sameASNum, m.Asnum, distance, countries) |
There was a problem hiding this comment.
Why not log method:%s in other cases, while you're at it? Ie. in the cases below, with statuscode == 404, 500 and the final else?
| logs.LogDownload(resultRenderer.Type(), status, results, err) | ||
| if len(mlist) > 0 { | ||
| logs.LogDownload(resultRenderer.Type(), r.Method, status, results, err) | ||
| if len(mlist) > 0 && r.Method == "GET" { |
There was a problem hiding this comment.
I have the impression that you could tighten it a bit more, and check the resultRenderer. In case it's a JSONRenderer, it's not really a download.
So it would be about adding && resultRenderer.Type() == "REDIRECT" I think
There was a problem hiding this comment.
Another comment: why not move the check r.Method == "GET" two lines above, at the level of if !ctx.IsMirrorlist()? This way, there's no need to modify logs.LogDownload at all.
It could be argued that the file downloads.log is meant to log downloads, and HEAD requests are not downloads, so shouldn't be logged in this file. I have the impression that it was the original intention. You can always look at HEAD requests in the nginx logs (assuming you run mirrorbits being a reverse proxy).
OTOH it could be argued that the file downloads.log was misnamed from the start, and it's useful to log HEAD requests in there, even if it's not a download?
This is just for the sake of discussion, I don't think it matters much.
Other request methods such as POST or HEAD or anything else than GET should not be counted as a new download.
GNOME admins updated our MirrorBits instance for GIMP now that #128 is merged, but I was disappointed to still see a lot of downloads (too many that I would believe, i.e. 2.6 million downloads a day!). I looked closer at MirrorBits code and realized it is counting as download any request. In particular, I am guessing that
HEADmay be quite a common request run by download accelerators or torrent clients on webseeds… well I am mostly guessing that this may be another source of false positives which may explain our huge stats.In any case, whether it's a reason or not for our over-high stats, I think it does make sense that only GET requests should be counted as downloads, right?
Note that it would still log all requests (adding an additional "method:GET" or "method:HEAD" for instance in the log file), but only the GET requests would now increase the download stats.