Ticket21315 #3
Ticket21315 #3
Conversation
Set the resolution of metrics data collection to be every 24 hours
Added three new metrics: - proxyIdleCount counts the number of times a proxy polls and receives no snowflakes - clientDeniedCount counts the number of times a client requested a snowflake but none were available - clientProxyMatchCount counts the number of times a client successfully received a snowflake
Output is now printed out in Tor Directory Protocol Format, as specified in https://trac.torproject.org/projects/tor/ticket/21315#comment:19.
Change it so that we log the geoip country code of proxies if they poll within the current metrics epoch. We make sure we log by unique IP address
Added unit tests for metrics logging. Refactored the logMetrics() function to allow for easier testing
|
Looks like Travis doesn't like the convey package. The branch is passing CI, even though this PR isn't. |
The patch looks generally good to me! I only have a number of nitpicks.
broker/metrics.go
Outdated
| type CountryStats struct { | ||
| ips map[string]bool |
Nitpick: we are using both ip and addr to refer to IP addresses. I think we should be consistent in our naming. (Personally, I like addr more.)
broker/metrics.go
Outdated
| //update map of countries and counts | ||
| m.countryStats.counts[country]++ | ||
| //update map of unique ips and counts | ||
| if !m.countryStats.ips[addr] { |
To save some CPU cycles, I think we could move this check to the beginning of the function, so we don't have to find the country code if we already have the IP address.
broker/snowflake-broker_test.go
Outdated
| <-done | ||
|
|
||
| ctx.metrics.printMetrics() | ||
| So(buf.String(), ShouldResemble, "snowflake-stats-end "+time.Now().UTC().Format("2006-01-02 15:04:05")+" ( 86400 s)\nsnowflake-ips CA=1,\nsnowflake-idle-count 8\nclient-denied-count 0\nclient-snowflake-match-count 0\n") |
Huh, where's the extra space in ( 86400 s) coming from? Our metrics format requires (NSEC s), so we should fix this.
| @@ -289,7 +292,7 @@ func main() { | |||
| metricsFile = os.Stdout | |||
| } | |||
|
|
|||
| metricsLogger := log.New(metricsFile, "", log.LstdFlags|log.LUTC) | |||
| metricsLogger := log.New(metricsFile, "", 0) | |||
Why are we getting rid of the flags here? I'm not saying we shouldn't; I'm just curious.
The flags for loggers are mostly about formatting timestamps and prefixes for log messages. We don't want any prefixes for the log file, only the information in the specification.
Also moved the geoip check to occur after we've make sure the proxy IP hasn't yet been recorded. This is will cut down on unecessary computation.
- removed trailing ","s - removed unecessary space before seconds
Looks good to me!
| @@ -82,6 +82,10 @@ func (m *Metrics) UpdateCountryStats(addr string) { | |||
| var country string | |||
| var ok bool | |||
|
|
|||
| if m.countryStats.addrs[addr] { | |||
Go suggests the use of two-value assignments to test for the existence of a key, but I think relying on the zero value of a bool should also be fine: https://blog.golang.org/go-maps-in-action#TOC_3.
Added another metrics item that counts the total availabel snowflakes (unique by IP address)
Updated the tests to pass with our new snowflake-ips-total stat
No description provided.
The text was updated successfully, but these errors were encountered: