Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
515993003:
Minimalistic analytics library used by Dart Server and try.dartlang.org (Closed)
|
Created:
6 years, 3 months ago by lukechurch Modified:
6 years, 3 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/lukechurch/dart-mircolytics.git@master Visibility:
Public. |
DescriptionFirst cut of basic functionality
This CL addresses comments from the previous Pull Request https://github.com/lukechurch/dart-microlytics/pull/1
It can be used to reporting usage and performance data to Google Analytics.
Known issues:
-> unittest library is still used
-> some comments are still not in full sentences
R=ahe@google.com, danrubel@google.com
Committed: https://code.google.com/p/dart/source/detail?r=39914
Patch Set 1 #
Total comments: 80
Patch Set 2 : Address review comments #
Total comments: 19
Patch Set 3 : Move code to SDK #Patch Set 4 : Merged with r39803 #Patch Set 5 : Ignore .settings file #Patch Set 6 : Remove unused io_channels import #Patch Set 7 : Fix whitespace issues. Again. #Patch Set 8 : Address review comments #
Total comments: 11
Patch Set 9 : Address review comments #
Messages
Total messages: 13 (1 generated)
lukechurch@google.com changed reviewers: + ahe@google.com, danrubel@google.com
More style feedback. Right now, I get lost in style issues, so I haven't carefully reviewed the functionality. However, I'm concerned that the test isn't testing real functionality. Not sure how to test this, as we probably don't want to ping the analytics server each time we run tests. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... File dart_microlytics/example/simple.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:5: import '../lib/channels.dart'; I don't think this import is correct. It should probably be import 'package:dart_microlytics/channels.dart'; https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:6: import '../lib/io_channels.dart'; Ditto. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:7: import '../lib/microlytics.dart'; Ditto. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:10: Remove extra line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:11: // Create the channel that will be used to communicate to analytics Add period. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:15: // Create the logger Period. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:16: var lg = new AnalyticsLogger(channel, "555", "UA-XXXXX-X", "test", "1.2"); Pass analytics ID in as command-line argument? https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:17: Remove extra line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:19: // Send some messages Period. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:22: Remove extra line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... File dart_microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:3: // BSD-style license that can be found in the LICENSE file. Add line between copyright and library declaration. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:15: /// See http://en.wikipedia.org/wiki/Leaky_bucket Period. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:19: int _bufferSizeLimit; Is this value going to change over the lifetime of this object? https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:20: Timer _timer; Same question. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:24: { bufferSizeLimit: 10, double packetsPerSecond: 1.0}) { One line per parameter. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:24: { bufferSizeLimit: 10, double packetsPerSecond: 1.0}) { bufferSizeLimit isn't typed. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:25: Remove extra line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:27: throw new ArgumentError("packetsPerSecond must be positive"); Period. Most people with a long education probably knows that zero isn't a positive number, but I suspect that many people would prefer to be told that packetsPerSecond must be larger than zero. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:29: this._bufferSizeLimit = bufferSizeLimit; Use initializer. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... File dart_microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:3: // BSD-style license that can be found in the LICENSE file. Add line between copyright and library declaration. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; Would "//www.google-analytics.com/collect" work here? It works in HTML, I don't know if it works in HttpRequest. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; In Dart, constants are "const". https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:14: .then((HttpRequest rest) { } There is no reason to install a then block when you don't intend to react to the result. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... File dart_microlytics/lib/io_channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:3: // BSD-style license that can be found in the LICENSE file. Add line between copyright and library declaration. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; Can you share these constants to avoid defining the value twice? https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:18: print(response.statusCode); Debug code? https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:21: Remove extra line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... File dart_microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:3: // BSD-style license that can be found in the LICENSE file. Add line between copyright and library declaration. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:44: "&av=${this._appVersion}" Can you share (some of) this string between the two methods? https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:45: ); Move to previous line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:60: ); Move to previous line. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/pubspec.yaml File dart_microlytics/pubspec.yaml (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/pubspec.yam... dart_microlytics/pubspec.yaml:1: name: dart_microlytics Missing copyright. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... File dart_microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:3: // BSD-style license that can be found in the LICENSE file. Add line between copyright and library declaration. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:6: import 'package:unittest/unittest.dart'; We need to discuss this. I'm of the opinion that unittest makes it harder to debug failing tests. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:28: "&av=0.42")); This code seem to to serve only the purpose of ensuring that logAnonymousEvent creates a nasty string. I'm not sure that adds a lot of value. Generally, the tests in this file seems to be rather lengthy tests of logAnonymousTiming that adds little value. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/test_c... File dart_microlytics/test/test_channel.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/test_c... dart_microlytics/test/test_channel.dart:3: // BSD-style license that can be found in the LICENSE file. Add line between copyright and library declaration.
Thanks Peter. PTAL again. I renamed the master folder to make it shorter. I apologise that it means that the comments appear to have come unhooked from the source. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... File dart_microlytics/example/simple.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:5: import '../lib/channels.dart'; On 2014/09/02 13:44:23, ahe wrote: > I don't think this import is correct. It should probably be > > import 'package:dart_microlytics/channels.dart'; Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:6: import '../lib/io_channels.dart'; On 2014/09/02 13:44:22, ahe wrote: > Ditto. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:7: import '../lib/microlytics.dart'; On 2014/09/02 13:44:23, ahe wrote: > Ditto. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:10: On 2014/09/02 13:44:23, ahe wrote: > Remove extra line. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:11: // Create the channel that will be used to communicate to analytics On 2014/09/02 13:44:22, ahe wrote: > Add period. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:15: // Create the logger On 2014/09/02 13:44:22, ahe wrote: > Period. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:16: var lg = new AnalyticsLogger(channel, "555", "UA-XXXXX-X", "test", "1.2"); On 2014/09/02 13:44:23, ahe wrote: > Pass analytics ID in as command-line argument? Good suggestion - I think in this case it will only clutter up the example with parsing code, I'll extract it out into a constant https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:17: On 2014/09/02 13:44:23, ahe wrote: > Remove extra line. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:19: // Send some messages On 2014/09/02 13:44:23, ahe wrote: > Period. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/example/sim... dart_microlytics/example/simple.dart:22: On 2014/09/02 13:44:23, ahe wrote: > Remove extra line. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... File dart_microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:3: // BSD-style license that can be found in the LICENSE file. On 2014/09/02 13:44:23, ahe wrote: > Add line between copyright and library declaration. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:15: /// See http://en.wikipedia.org/wiki/Leaky_bucket On 2014/09/02 13:44:23, ahe wrote: > Period. That will cause the URL to break, I'll use parens instead https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:19: int _bufferSizeLimit; On 2014/09/02 13:44:23, ahe wrote: > Is this value going to change over the lifetime of this object? There's a language feature interaction problem here. If this is set to final, it can't be used with the optional params, as they may not start with an _. This also interferes with the initializer syntax. I've solved all of these problems by removing the _privacy. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:20: Timer _timer; On 2014/09/02 13:44:23, ahe wrote: > Same question. No, but, making this final would imply moving the whole of the setup code into initializer which harms readability. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:24: { bufferSizeLimit: 10, double packetsPerSecond: 1.0}) { On 2014/09/02 13:44:23, ahe wrote: > bufferSizeLimit isn't typed. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:25: On 2014/09/02 13:44:23, ahe wrote: > Remove extra line. This makes is really hard to see where the init ends and the body starts, esp after splitting the line above into two. I've made the adjustment, but this doesn't seem like a good idea IMHO. Is it mandated in the style guide? I think I use quite a bit more whitespace by default than you do. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:27: throw new ArgumentError("packetsPerSecond must be positive"); On 2014/09/02 13:44:23, ahe wrote: > Period. > > Most people with a long education probably knows that zero isn't a positive > number, but I suspect that many people would prefer to be told that > packetsPerSecond must be larger than zero. Good point. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:29: this._bufferSizeLimit = bufferSizeLimit; On 2014/09/02 13:44:23, ahe wrote: > Use initializer. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... File dart_microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:3: // BSD-style license that can be found in the LICENSE file. On 2014/09/02 13:44:24, ahe wrote: > Add line between copyright and library declaration. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; On 2014/09/02 13:44:23, ahe wrote: > Would "//www.google-analytics.com/collect" work here? > > It works in HTML, I don't know if it works in HttpRequest. Does that mandate SSL? Given that I don't know what it does, I'd suggest keeping as is. Any particular reason you's suggest the above? https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:14: .then((HttpRequest rest) { } On 2014/09/02 13:44:23, ahe wrote: > There is no reason to install a then block when you don't intend to react to the > result. I thought that there was an issue similar to the one mentioned in https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart:io.HttpC... I was incorrect. Removing. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... File dart_microlytics/lib/io_channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:3: // BSD-style license that can be found in the LICENSE file. On 2014/09/02 13:44:24, ahe wrote: > Add line between copyright and library declaration. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; On 2014/09/02 13:44:24, ahe wrote: > Can you share these constants to avoid defining the value twice? Done. I wanted to do this, but it breaks the _privacy. Seems like the right tradeoff. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:18: print(response.statusCode); On 2014/09/02 13:44:24, ahe wrote: > Debug code? /red face. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/io_chan... dart_microlytics/lib/io_channels.dart:21: On 2014/09/02 13:44:24, ahe wrote: > Remove extra line. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... File dart_microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:44: "&av=${this._appVersion}" On 2014/09/02 13:44:24, ahe wrote: > Can you share (some of) this string between the two methods? Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:45: ); On 2014/09/02 13:44:24, ahe wrote: > Move to previous line. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:60: ); On 2014/09/02 13:44:24, ahe wrote: > Move to previous line. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/pubspec.yaml File dart_microlytics/pubspec.yaml (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/pubspec.yam... dart_microlytics/pubspec.yaml:1: name: dart_microlytics On 2014/09/02 13:44:24, ahe wrote: > Missing copyright. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... File dart_microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:3: // BSD-style license that can be found in the LICENSE file. On 2014/09/02 13:44:24, ahe wrote: > Add line between copyright and library declaration. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:6: import 'package:unittest/unittest.dart'; On 2014/09/02 13:44:24, ahe wrote: > We need to discuss this. I'm of the opinion that unittest makes it harder to > debug failing tests. Understood. Lets talk tomorrow https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:28: "&av=0.42")); On 2014/09/02 13:44:24, ahe wrote: > This code seem to to serve only the purpose of ensuring that logAnonymousEvent > creates a nasty string. I'm not sure that adds a lot of value. > > Generally, the tests in this file seems to be rather lengthy tests of > logAnonymousTiming that adds little value. I agree. I think this will be make more sense when I start doing the same thing with the rate limiter tests. I wanted to submit those in a follow on CL. I do welcome suggestions as to how to improve the tests here. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/test_c... File dart_microlytics/test/test_channel.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/test_c... dart_microlytics/test/test_channel.dart:3: // BSD-style license that can be found in the LICENSE file. On 2014/09/02 13:44:24, ahe wrote: > Add line between copyright and library declaration. Done.
I think the style issues are mostly addressed now. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... File dart_microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:19: int _bufferSizeLimit; On 2014/09/02 19:52:36, lukechurch wrote: > On 2014/09/02 13:44:23, ahe wrote: > > Is this value going to change over the lifetime of this object? > > There's a language feature interaction problem here. You can write: RateLimitingBufferedChannel( this._innerChannel, {int bufferSizeLimit: 10, double packetsPerSecond: 1.0}) : _bufferSizeLimit = bufferSizeLimit { https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:20: Timer _timer; On 2014/09/02 19:52:36, lukechurch wrote: > On 2014/09/02 13:44:23, ahe wrote: > > Same question. > > No, but, making this final would imply moving the whole of the setup code into > initializer which harms readability. The current code isn't readable, as you're mixing up initializing the object with Java-style constructors. However, you can't make this field final without introducing a factory, and that is probably overkill. On the other hand, it really stinks to create an object in a bad state. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:25: On 2014/09/02 19:52:36, lukechurch wrote: > On 2014/09/02 13:44:23, ahe wrote: > > Remove extra line. > > This makes is really hard to see where the init ends and the body starts, esp > after splitting the line above into two. > > I've made the adjustment, but this doesn't seem like a good idea IMHO. Is it > mandated in the style guide? I think I use quite a bit more whitespace by > default than you do. I can live with the extra line in those cases. However, it isn't the style that we have adopted elsewhere, and I expect that some people on our team would complain. As for finding the start of the body, the real problem is that your constructor is too complicated. If you stick to simple constructors you don't have this problem. In addition, there is a difference in indentation that should help you find the body. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... File dart_microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; On 2014/09/02 19:52:37, lukechurch wrote: > On 2014/09/02 13:44:23, ahe wrote: > > Would "//www.google-analytics.com/collect" work here? > > > > It works in HTML, I don't know if it works in HttpRequest. > > Does that mandate SSL? Yes, if the page uses SSL. > > Given that I don't know what it does, I'd suggest keeping as is. Any particular > reason you's suggest the above? Because I use this trick elsewhere in index.html in Try Dart. However, I looked at the analytics code in Try Dart. It looks like you're supposed to use https://ssl.google-analytics.com/, not https://www.google-analytics.com/. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... File dart_microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:28: "&av=0.42")); On 2014/09/02 19:52:37, lukechurch wrote: > On 2014/09/02 13:44:24, ahe wrote: > > This code seem to to serve only the purpose of ensuring that logAnonymousEvent > > creates a nasty string. I'm not sure that adds a lot of value. > > > > Generally, the tests in this file seems to be rather lengthy tests of > > logAnonymousTiming that adds little value. > > I agree. I think this will be make more sense when I start doing the same thing > with the rate limiter tests. I wanted to submit those in a follow on CL. > > I do welcome suggestions as to how to improve the tests here. The basic problem is that the HttpX channels aren't tested. We could probably test those by mocking up a server and making the analytics URI configurable. This only works in the dart:io case, not in the browser. However, next time you visit, we can allocate a few hours to build the necessary test infrastructure. https://codereview.chromium.org/515993003/diff/20001/.gitignore File .gitignore (right): https://codereview.chromium.org/515993003/diff/20001/.gitignore#newcode1 .gitignore:1: /dart_microlytics/packages/ Forgot to update this file? https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... File microlytics/example/simple.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... microlytics/example/simple.dart:12: new HttpClientChannel(),packetsPerSecond: 1.0); Add space before packetsPerSecond. https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... microlytics/example/simple.dart:14: const String CLIENT_ID = "UA-XXXXXXXX-1"; //Replace this with your client ID In Dart, it is possible to define compile-time constants from the command line: const String.fromEnvironment("CLIENT_ID") However, passing in one option is even easier: * Change the declaration of main to "main(List<String> arguments)" * Extract the single command-line argument with "arguments.single". https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... File microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:9: final String ANALYTICS_URL = "https://www.google-analytics.com/collect"; I'm not sure this is related to channels. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:18: /// See (http://en.wikipedia.org/wiki/Leaky_bucket) Add period, and use [] instead of (). https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:27: { this.bufferSizeLimit: 10, Remove space after {. https://codereview.chromium.org/515993003/diff/20001/microlytics/test/dart_mi... File microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/test/dart_mi... microlytics/test/dart_microlytics_test.dart:6: import 'package:unittest/unittest.dart'; Still not a fan.
https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/html_cha... File microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/html_cha... microlytics/lib/html_channels.dart:12: HttpRequest.request(ANALYTICS_URL, method: "POST", sendData: data); Do you want to handle the response here similar to how you have handled it in io_channels.dart? https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... File microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... microlytics/lib/microlytics.dart:21: /// [_channel] represents how this is going to be sent, this would typically Either remove the '_' on the comments to match the arguments or switch the arguments to match the comments. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... microlytics/lib/microlytics.dart:27: AnalyticsLogger(channel, clientID, analyticsID, appName, appVersion) all public API should be fully typed... https://www.dartlang.org/articles/style-guide/#type-annotations
https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... File dart_microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:19: int _bufferSizeLimit; On 2014/09/03 08:51:31, ahe wrote: > On 2014/09/02 19:52:36, lukechurch wrote: > > On 2014/09/02 13:44:23, ahe wrote: > > > Is this value going to change over the lifetime of this object? > > > > There's a language feature interaction problem here. > > You can write: > > RateLimitingBufferedChannel( > this._innerChannel, > {int bufferSizeLimit: 10, double packetsPerSecond: 1.0}) > : _bufferSizeLimit = bufferSizeLimit { Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:20: Timer _timer; On 2014/09/03 08:51:31, ahe wrote: > On 2014/09/02 19:52:36, lukechurch wrote: > > On 2014/09/02 13:44:23, ahe wrote: > > > Same question. > > > > No, but, making this final would imply moving the whole of the setup code into > > initializer which harms readability. > > The current code isn't readable, as you're mixing up initializing the object > with Java-style constructors. > > However, you can't make this field final without introducing a factory, and that > is probably overkill. On the other hand, it really stinks to create an object in > a bad state. Acknowledged. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/channel... dart_microlytics/lib/channels.dart:25: On 2014/09/03 08:51:30, ahe wrote: > On 2014/09/02 19:52:36, lukechurch wrote: > > On 2014/09/02 13:44:23, ahe wrote: > > > Remove extra line. > > > > This makes is really hard to see where the init ends and the body starts, esp > > after splitting the line above into two. > > > > I've made the adjustment, but this doesn't seem like a good idea IMHO. Is it > > mandated in the style guide? I think I use quite a bit more whitespace by > > default than you do. > > I can live with the extra line in those cases. However, it isn't the style that > we have adopted elsewhere, and I expect that some people on our team would > complain. > > As for finding the start of the body, the real problem is that your constructor > is too complicated. If you stick to simple constructors you don't have this > problem. In addition, there is a difference in indentation that should help you > find the body. Acknowledged. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... File dart_microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/html_ch... dart_microlytics/lib/html_channels.dart:9: final String _ANALYTICS_URL = "https://www.google-analytics.com/collect"; On 2014/09/03 08:51:31, ahe wrote: > On 2014/09/02 19:52:37, lukechurch wrote: > > On 2014/09/02 13:44:23, ahe wrote: > > > Would "//www.google-analytics.com/collect" work here? > > > > > > It works in HTML, I don't know if it works in HttpRequest. > > > > Does that mandate SSL? > > Yes, if the page uses SSL. > > > > > Given that I don't know what it does, I'd suggest keeping as is. Any > particular > > reason you's suggest the above? > > Because I use this trick elsewhere in index.html in Try Dart. > > However, I looked at the analytics code in Try Dart. It looks like you're > supposed to use https://ssl.google-analytics.com/, not > https://www.google-analytics.com/. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... File dart_microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/lib/microly... dart_microlytics/lib/microlytics.dart:3: // BSD-style license that can be found in the LICENSE file. On 2014/09/02 13:44:24, ahe wrote: > Add line between copyright and library declaration. Done. https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... File dart_microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/1/dart_microlytics/test/dart_m... dart_microlytics/test/dart_microlytics_test.dart:28: "&av=0.42")); On 2014/09/03 08:51:31, ahe wrote: > On 2014/09/02 19:52:37, lukechurch wrote: > > On 2014/09/02 13:44:24, ahe wrote: > > > This code seem to to serve only the purpose of ensuring that > logAnonymousEvent > > > creates a nasty string. I'm not sure that adds a lot of value. > > > > > > Generally, the tests in this file seems to be rather lengthy tests of > > > logAnonymousTiming that adds little value. > > > > I agree. I think this will be make more sense when I start doing the same > thing > > with the rate limiter tests. I wanted to submit those in a follow on CL. > > > > I do welcome suggestions as to how to improve the tests here. > > The basic problem is that the HttpX channels aren't tested. We could probably > test those by mocking up a server and making the analytics URI configurable. > > This only works in the dart:io case, not in the browser. However, next time you > visit, we can allocate a few hours to build the necessary test infrastructure. Acknowledged. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/html_cha... File microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/html_cha... microlytics/lib/html_channels.dart:12: HttpRequest.request(ANALYTICS_URL, method: "POST", sendData: data); On 2014/09/03 11:19:48, danrubel wrote: > Do you want to handle the response here similar to how you have handled it in > io_channels.dart? The libraries place different requirements on the handler. The HttpClient leaks resources if it isn't drained, this response doesn't have the same needs, or indeed the same API for doing so. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... File microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... microlytics/lib/microlytics.dart:27: AnalyticsLogger(channel, clientID, analyticsID, appName, appVersion) On 2014/09/03 11:19:48, danrubel wrote: > all public API should be fully typed... > https://www.dartlang.org/articles/style-guide/#type-annotations Arrg, yes, this was caused by the adjustment to using this style of ctor that no longer implicitly types the arguments from the backing variables. Thanks.
PTAL
lgtm
thanks Dan, I'm going to hold for @ahe on this as well.
https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... File microlytics/example/simple.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... microlytics/example/simple.dart:12: new HttpClientChannel(),packetsPerSecond: 1.0); On 2014/09/03 08:51:31, ahe wrote: > Add space before packetsPerSecond. Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... microlytics/example/simple.dart:14: const String CLIENT_ID = "UA-XXXXXXXX-1"; //Replace this with your client ID On 2014/09/03 08:51:31, ahe wrote: > In Dart, it is possible to define compile-time constants from the command line: > > const String.fromEnvironment("CLIENT_ID") > > However, passing in one option is even easier: > > * Change the declaration of main to "main(List<String> arguments)" > > * Extract the single command-line argument with "arguments.single". Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... File microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:9: final String ANALYTICS_URL = "https://www.google-analytics.com/collect"; On 2014/09/03 08:51:31, ahe wrote: > I'm not sure this is related to channels. True. I thought that having a separate class to declare a single constant was design overkill. This was a pragmatic tradeoff. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:18: /// See (http://en.wikipedia.org/wiki/Leaky_bucket) On 2014/09/03 08:51:31, ahe wrote: > Add period, and use [] instead of (). Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:27: { this.bufferSizeLimit: 10, On 2014/09/03 08:51:31, ahe wrote: > Remove space after {. Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... File microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... microlytics/lib/microlytics.dart:21: /// [_channel] represents how this is going to be sent, this would typically On 2014/09/03 11:19:48, danrubel wrote: > Either remove the '_' on the comments to match the arguments or switch the > arguments to match the comments. Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/test/dart_mi... File microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/test/dart_mi... microlytics/test/dart_microlytics_test.dart:6: import 'package:unittest/unittest.dart'; On 2014/09/03 08:51:31, ahe wrote: > Still not a fan. Removed.
https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... File microlytics/example/simple.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... microlytics/example/simple.dart:12: new HttpClientChannel(),packetsPerSecond: 1.0); On 2014/09/03 08:51:31, ahe wrote: > Add space before packetsPerSecond. Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/example/simp... microlytics/example/simple.dart:14: const String CLIENT_ID = "UA-XXXXXXXX-1"; //Replace this with your client ID On 2014/09/03 08:51:31, ahe wrote: > In Dart, it is possible to define compile-time constants from the command line: > > const String.fromEnvironment("CLIENT_ID") > > However, passing in one option is even easier: > > * Change the declaration of main to "main(List<String> arguments)" > > * Extract the single command-line argument with "arguments.single". Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... File microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:9: final String ANALYTICS_URL = "https://www.google-analytics.com/collect"; On 2014/09/03 08:51:31, ahe wrote: > I'm not sure this is related to channels. True. I thought that having a separate class to declare a single constant was design overkill. This was a pragmatic tradeoff. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:18: /// See (http://en.wikipedia.org/wiki/Leaky_bucket) On 2014/09/03 08:51:31, ahe wrote: > Add period, and use [] instead of (). Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/channels... microlytics/lib/channels.dart:27: { this.bufferSizeLimit: 10, On 2014/09/03 08:51:31, ahe wrote: > Remove space after {. Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... File microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/lib/microlyt... microlytics/lib/microlytics.dart:21: /// [_channel] represents how this is going to be sent, this would typically On 2014/09/03 11:19:48, danrubel wrote: > Either remove the '_' on the comments to match the arguments or switch the > arguments to match the comments. Done. https://codereview.chromium.org/515993003/diff/20001/microlytics/test/dart_mi... File microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/20001/microlytics/test/dart_mi... microlytics/test/dart_microlytics_test.dart:6: import 'package:unittest/unittest.dart'; On 2014/09/03 08:51:31, ahe wrote: > Still not a fan. Removed.
There's a few formatting issues that I think you should fix before submitting. Otherwise, LGTM provided you follow up with more tests. No need to wait for another LGTM for the style issues, but please do upload a new patch set before committing your changes. https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/ex... File dart/pkg/microlytics/example/simple.dart (right): https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/ex... dart/pkg/microlytics/example/simple.dart:18: final String CLIENT_ID = arguments.single; Not a constant anymore, so use lowercaseCamelCase. https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... File dart/pkg/microlytics/lib/channels.dart (right): https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/channels.dart:9: const String ANALYTICS_URL = "https://ssl.google-analytics.com/collect"; Sorry, I know I suggested using ssl. here, but then I read something else on Google Analytics site that led me to believe it is now deprecated. https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/channels.dart:18: /// See [http://en.wikipedia.org/wiki/Leaky_bucket]. Just a thought. You might want to consider this algorithm's impact on battery usage. https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/channels.dart:20: final List<String> _buffer = <String>[]; Wouldn't a Queue be a better fit? https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/channels.dart:31: throw new ArgumentError("packetsPerSecond must be larger than zero"); Not a proper sentence (no period). https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/channels.dart:36: (_) => _onTimerTick() ); Weird formatting, and extra space between ) ). Suggest this formatting: _timer = new Timer.periodic( new Duration(milliseconds: transmitDelay), _onTimerTick); (Add ignored argument to _onTimerTick). https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... File dart/pkg/microlytics/lib/html_channels.dart (right): https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/html_channels.dart:12: HttpRequest.request(ANALYTICS_URL, method: "POST", sendData: data); When you start writing tests of this, you'll have to return the future. Next CL? https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... File dart/pkg/microlytics/lib/microlytics.dart (right): https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/microlytics.dart:38: "&av=$appVersion"; The type inference algorithm in dart2js loves when you write code like this. Personally, I like it better as well, as final fields are easier to reason about for humans as well. https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/li... dart/pkg/microlytics/lib/microlytics.dart:40: void logAnonymousTiming(String category, String variable, int ms) { These two methods probably need to return a future. Next CL? https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/te... File dart/pkg/microlytics/test/dart_microlytics_test.dart (right): https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/te... dart/pkg/microlytics/test/dart_microlytics_test.dart:20: TestChannel c = new TestChannel(); Indent by two. https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/te... File dart/pkg/microlytics/test/test_channel.dart (right): https://codereview.chromium.org/515993003/diff/140001/dart/pkg/microlytics/te... dart/pkg/microlytics/test/test_channel.dart:17: return _channelLog.contains(data); It occurred to me that you might want to change this method to count the number of occurrences instead?
Message was sent while issue was closed.
Committed patchset #9 (id:160001) manually as 39914 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
