{"action":"created","issue":{"url":"https://api.github.com/repos/go-statsd/statsd/issues/2","labels_url":"https://api.github.com/repos/go-statsd/statsd/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/go-statsd/statsd/issues/2/comments","events_url":"https://api.github.com/repos/go-statsd/statsd/issues/2/events","html_url":"https://github.com/go-statsd/statsd/pull/2","id":58066252,"number":2,"title":"lots of opinionated changes!","user":{"login":"rogpeppe","id":66491,"avatar_url":"https://avatars.githubusercontent.com/u/66491?v=3","gravatar_id":"","url":"https://api.github.com/users/rogpeppe","html_url":"https://github.com/rogpeppe","followers_url":"https://api.github.com/users/rogpeppe/followers","following_url":"https://api.github.com/users/rogpeppe/following{/other_user}","gists_url":"https://api.github.com/users/rogpeppe/gists{/gist_id}","starred_url":"https://api.github.com/users/rogpeppe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rogpeppe/subscriptions","organizations_url":"https://api.github.com/users/rogpeppe/orgs","repos_url":"https://api.github.com/users/rogpeppe/repos","events_url":"https://api.github.com/users/rogpeppe/events{/privacy}","received_events_url":"https://api.github.com/users/rogpeppe/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-02-18T13:17:11Z","updated_at":"2015-02-18T16:11:03Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/go-statsd/statsd/pulls/2","html_url":"https://github.com/go-statsd/statsd/pull/2","diff_url":"https://github.com/go-statsd/statsd/pull/2.diff","patch_url":"https://github.com/go-statsd/statsd/pull/2.patch"},"body":"I like the general approach, but I had a bunch of ideas about how\r\nthings might be improved. This PR lumps 'em all together...\r\n\r\nThe main API changes are:\r\n-  lose the default client - unlike with http.Client, I don't think a global client\r\n    is the right thing here, because we are addressing one particular server\r\n    and producing stats designed for that server. In preference, I'd choose to\r\n    create domain-specific statsd.Client global variables where desired\r\n    outside of this package, making it possible for different components to\r\n    generate independent stats to different servers.\r\n- connect at initialisation time rather than on first write - \"connecting\" on a UDP socket\r\n    doesn't involve actually contacting the remote server (just a potential DNS lookup) so\r\n    I think it makes sense to do this earlier rather than later, as this is the most likely\r\n    time that we might see an error. This simplifies the code logic\r\n    too.\r\n- don't return errors from all methods. Since this is designed to be used as a global variable\r\n    and writes are amortised, there is unlikely to be anything a client can usefully do if\r\n    a stat call returns an error. Instead, we provide an error callback that can be used to\r\n    inform of errors on writes. In practice, this will probably never be invoked as UDP will\r\n    silently drop packets rather than return an error.\r\n\r\nOther more minor API changes:\r\n- dropped the Timing method - it seemed redundant with Duration, and a touch confusing.\r\n- lose the rate arguments from Unique and Gauge, as the server (the etsy statsd at least)\r\n  ignores them.\r\n\r\nI also made various changes to the implementation in the interests of efficiency (we\r\nwant to be able to generate stats with very low overhead). With a nop writer, the new code\r\nis about 80% faster than the old code (272ns vs 1398ns) and does not allocate at all (the old code incurs\r\nat least 4 allocs per update).\r\n\r\nEven if you think it seems reasonable, this PR is not yet ready for submission (I'd like a few more tests to bring test coverage up to 100%, and the docs need work) but I thought this is probably a good place to stop and ask for thoughts and reactions.\r\n\r\nFurther work:\r\n- I'd really like to see some tests that work against a live statsd server so that we have some idea of whether this code will actually work in reality.\r\n- I think there should be a flusher that periodically flushes stats after a timeout, so that we don't have stats sitting in the buffer and getting indefinitely stale.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/go-statsd/statsd/issues/comments/74891608","html_url":"https://github.com/go-statsd/statsd/pull/2#issuecomment-74891608","issue_url":"https://api.github.com/repos/go-statsd/statsd/issues/2","id":74891608,"user":{"login":"cmars","id":23741,"avatar_url":"https://avatars.githubusercontent.com/u/23741?v=3","gravatar_id":"","url":"https://api.github.com/users/cmars","html_url":"https://github.com/cmars","followers_url":"https://api.github.com/users/cmars/followers","following_url":"https://api.github.com/users/cmars/following{/other_user}","gists_url":"https://api.github.com/users/cmars/gists{/gist_id}","starred_url":"https://api.github.com/users/cmars/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cmars/subscriptions","organizations_url":"https://api.github.com/users/cmars/orgs","repos_url":"https://api.github.com/users/cmars/repos","events_url":"https://api.github.com/users/cmars/events{/privacy}","received_events_url":"https://api.github.com/users/cmars/received_events","type":"User","site_admin":false},"created_at":"2015-02-18T16:11:03Z","updated_at":"2015-02-18T16:11:03Z","body":"Lots of good stuff here, I really like where its going! Looking forward to landing many of these improvements.\r\n\r\n> -  lose the default client - unlike with http.Client, I don't think a global client\r\n    is the right thing here, because we are addressing one particular server\r\n    and producing stats designed for that server. In preference, I'd choose to\r\n    create domain-specific statsd.Client global variables where desired\r\n    outside of this package, making it possible for different components to\r\n    generate independent stats to different servers.\r\n\r\nGood point.. more freedom for the package importer there.\r\n\r\n> - connect at initialisation time rather than on first write - \"connecting\" on a UDP socket\r\n    doesn't involve actually contacting the remote server (just a potential DNS lookup) so\r\n    I think it makes sense to do this earlier rather than later, as this is the most likely\r\n    time that we might see an error. This simplifies the code logic\r\n    too.\r\n\r\nNot sure I agree with this one, but could use some advice. I've noticed in Go that if there is nothing listening at the configured UDP address when the client does net.Dial, the client connection is never able to write to this endpoint, even if it later shows up. Shouldn't be a problem since UDP is stateless, but it is for Go's UDP implementation. This was detected and fixed with my `TestReconnect` test.\r\n\r\nThis is why there's a \"reconnect\" on write error in log/syslog[1]. I think we'll need something similar. Our client needs to be resilient to server outages, including at startup.\r\n\r\n[1] http://golang.org/src/log/syslog/syslog.go?s=3915:3960#L251\r\n\r\n> I also made various changes to the implementation in the interests of efficiency (we\r\nwant to be able to generate stats with very low overhead). With a nop writer, the new code\r\nis about 80% faster than the old code (272ns vs 1398ns) and does not allocate at all (the old code incurs\r\nat least 4 allocs per update).\r\n\r\nAmazing! Thanks for this!\r\n\r\n> Further work:\r\n- I'd really like to see some tests that work against a live statsd server so that we have some idea of whether this code will actually work in reality.\r\n\r\nI decided to stand up a really simple one in my httpstats PR #1. We might want a \"real\" one for integration tests though :)\r\n\r\n> - I think there should be a flusher that periodically flushes stats after a timeout, so that we don't have stats sitting in the buffer and getting indefinitely stale.\r\n\r\n+100"}}