Bug31157 #13
Bug31157 #13
Conversation
broker/snowflake-broker_test.go
Outdated
| @@ -408,7 +408,6 @@ func TestMetrics(t *testing.T) { | |||
| w := httptest.NewRecorder() | |||
| data := bytes.NewReader([]byte("{\"Sid\":\"ymbcCMto7KHNGYlp\",\"Version\":\"1.0\"}")) | |||
| r, err := http.NewRequest("POST", "snowflake.broker/proxy", data) | |||
| r.Header.Set("X-Session-ID", "test") | |||
There was a problem hiding this comment.
Out of curiosity, why are we removing the X-Session-ID headers in the tests?
There was a problem hiding this comment.
This isn't actually related to this commit, but we removed the use of X-Session-ID headers previously. See: https://gitweb.torproject.org/pluggable-transports/snowflake.git/commit/?id=c4ae64905b69512e50587c1ee749cddfc0937a4c
I caught that this was still there and figured I could remove it while I was modifying the file.
There was a problem hiding this comment.
Gotcha. I would suggest to move unrelated changes to separate commits in the future, to make the commit history easier to follow.
broker/metrics.go
Outdated
| @@ -19,6 +19,24 @@ We export metrics in the following format: | |||
| A count of the total number of unique IP addresses of snowflake | |||
| proxies that have polled. | |||
|
|
|||
| "snowflake-ips-standalone" NUM NL | |||
There was a problem hiding this comment.
Shouldn't we also add this to our specification in doc/broker-spec.txt? In fact, we may want to put the metrics specification in one place only, and link to it from other places.
There was a problem hiding this comment.
Agreed, I put it all in doc/broker-spec.txt and linked to it from metrics.go
broker/snowflake-heap.go
Outdated
| @@ -10,6 +10,7 @@ over the offer and answer channels. | |||
| */ | |||
| type Snowflake struct { | |||
| id string | |||
| ptype string | |||
There was a problem hiding this comment.
Nitpick: Can we call this (and other variables) proxyType instead of ptype? I think it would make the code a bit more accessible.
common/messages/proxy.go
Outdated
| ) | ||
|
|
||
| const version = "1.0" | ||
| const version = "1.1" |
There was a problem hiding this comment.
How do we decide if we increment the major or the minor version number here? I assume this patch set is backwards-compatible, so we're only increasing the minor version number?
There was a problem hiding this comment.
Yeah I was thinking of using major versions for changes that are not backwards compatible with proxies running a lower version.
common/messages/proxy.go
Outdated
| Version: 1.0 | ||
| Sid: [generated session id of proxy], | ||
| Version: 1.1, | ||
| Type: [badge|webext|standalone] |
There was a problem hiding this comment.
Nitpick: Maybe we want this to be Type: ["badge"|"webext"|"standalone"] instead, to make it clear that these are string literals.
common/messages/proxy.go
Outdated
| return json.Marshal(ProxyPollRequest{ | ||
| Sid: sid, | ||
| Version: version, | ||
| Type: ptype, | ||
| }) | ||
| } | ||
|
|
||
| // Decodes a poll message from a snowflake proxy and returns the | ||
| // sid of the proxy on success and an error if it failed |
There was a problem hiding this comment.
The comment should probably also mention the proxy type.
|
Okay I pushed an update, thanks! |
Implements https://trac.torproject.org/projects/tor/ticket/31157