Implement HTTP fullscrapes #462
Conversation
| } | ||
| remoteIP := net.ParseIP(host) | ||
| if remoteIP == nil { | ||
| return nil, errors.New("unable to parse HTTP remote address as IP") |
There was a problem hiding this comment.
We parse IPs in a bunch of places. Maybe we should create one error for all of the failure cases across the codebase.
There was a problem hiding this comment.
Well, it's kinda messy. Sometimes we return the IP and an error, sometimes we return an IP and a bool... then there's also the whole IP spoofing thing. UDP only copies around bytes/remote socket address and does no checks whatsoever.
I don't think we can unify all of those 😅
| // This is a fullscrape. | ||
| start := time.Now() | ||
| resp = ps.fullscrapeSwarms(addressFamily) | ||
| recordFullscrapeDuration(time.Since(start)) |
There was a problem hiding this comment.
Couldn't we pull the metric timing up to the caller of this function? Are we doing that for the non-full-scrape case? The caller can definitely make the check of whether the len(infoHashes) == 0
There was a problem hiding this comment.
Well, we're tracking some request durations in the frontends, if enabled via the config flag. This includes scrapes, although we currently only have one action "scrape", but that wouldn't be a problem to change...
I kinda see your point, as in: The prometheus metrics for the storage are all storage-internal, as in not visible to a frontend. I just added it here because I thought it would be interesting to see, similar to GC duration.
I expect fullscrapes to be an occasional thing, and I know from some people who didn't enable the request timing in the frontend (because it costs performance).
Opinion?
| }) | ||
| return | ||
| } | ||
| for i, ih := range infoHashes { |
There was a problem hiding this comment.
So redis doesn't support fullscrapes? :(
There was a problem hiding this comment.
It's been a while since I wrote this, but my gut feeling is that this had some complexity due to the possibility of asynchronous stuff happening.. I'd probably have to design another WATCH ... MULTI ... EXEC block and prove that to be correct. What's the priority on this? How about just a note in the docs that redis currently doesn't do fullscrapes, and an issue to keep track of it?
|
@jzelinskie I could need some input here too :) |
Fixes #369 (eventually)
WIP, but need travis