Add initial version of analytics and crash reporting package.

Add initial version of analytics and crash reporting package.

BUG=
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/8e45759a674615b792a8916c93ba1ce12960eeeb

[devoncarew, 2017-06-23 20:40:48]

devoncarew@google.com changed reviewers:
+ brianwilkerson@google.com, kevmoo@google.com

[devoncarew, 2017-06-23 20:40:49]

Nothing is hooked up to this lib yet, but a follow-up CL would do so.

[kevmoo, 2017-06-23 21:01:05]

A few things – defer to Brian for final approval

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
File pkg/telemetry/lib/crash_reporting.dart (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:32: class CrashReportSender {
Should likely have a way to close/dispose this – and propagate that close to
_httpClient

Otherwise, processes can hang...

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:36: port: 443,
A bit redundant w/ https?

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:41: http.Client _httpClient;
make final?

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:84: } catch (sendError, sendStackTrace) {
A tiny more conventional to use `on SocketException` then another blank catch

Also: good to avoid just throwing `String`

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:89: throw 'exception while sending crash
report: $sendError\n$sendStackTrace';
long line

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/telem...
File pkg/telemetry/lib/telemetry.dart (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/telem...
pkg/telemetry/lib/telemetry.dart:49: Directory getDartStorageDirectory() {
Could just be =>

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/telem...
pkg/telemetry/lib/telemetry.dart:89: //
https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Va...
long lines

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/pubspec.yaml
File pkg/telemetry/pubspec.yaml (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/pubspec.y...
pkg/telemetry/pubspec.yaml:3: version: 0.0.1
0.0.1-dev

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/pubspec.y...
pkg/telemetry/pubspec.yaml:12: path: ^1.4.0
sort: p before s

[Brian Wilkerson, 2017-06-23 21:41:09]

Is there a more general name? It seems odd to name a package 'telemetry' when it
supports both telemetry and crash reporting.

Other than that, lgtm.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
File pkg/telemetry/lib/crash_reporting.dart (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:31: /// Sends crash reports to Google.
If we might publish this as a package someday, consider adding a disclaimer
stating that clients aren't allowed to extend, mixin or implement this class, or
make this an interface with a factory constructor that creates an internally
defined class.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:41: http.Client _httpClient;
> make final?

It can't be because of the assignment on line 46.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:52: Future sendReport(dynamic error,
[StackTrace stackTrace]) async {
Consider making stackTrace a named parameter. Adding more parameters in the
future will be cleaner.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:70: req.fields['osVersion'] =
Platform.operatingSystem; // includes the vesion
"vesion" --> "version"

Should we parse out the name and version?

[kevmoo, 2017-06-23 21:43:31]

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
File pkg/telemetry/lib/crash_reporting.dart (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:41: http.Client _httpClient;
On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> > make final?
> 
> It can't be because of the assignment on line 46.

That can be done in the initializer. No worries.

[devoncarew, 2017-06-26 15:51:51]

On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> Is there a more general name? It seems odd to name a package 'telemetry' when
it
> supports both telemetry and crash reporting.

This was the best name I could think of that encompassed managing an Analytics
object, common error messages for the sdk tools, and a crash reporting library.
Happy to have a different name - I could think of no better to date :)

I don't think we'll ever publish this package, just have it in the sdk as a sdk
DEP.

[devoncarew, 2017-06-26 15:53:46]

Thanks for the comments! Addressed and updated.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
File pkg/telemetry/lib/crash_reporting.dart (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:31: /// Sends crash reports to Google.
On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> If we might publish this as a package someday, consider adding a disclaimer
> stating that clients aren't allowed to extend, mixin or implement this class,
or
> make this an interface with a factory constructor that creates an internally
> defined class.

Done.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:32: class CrashReportSender {
On 2017/06/23 21:01:04, kevmoo wrote:
> Should likely have a way to close/dispose this – and propagate that close
to
> _httpClient
> 
> Otherwise, processes can hang...

Good point! Added a dispose() method, doc'd it, and had it call through to
http.Client.close().

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:36: port: 443,
On 2017/06/23 21:01:04, kevmoo wrote:
> A bit redundant w/ https?

removed

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:41: http.Client _httpClient;
On 2017/06/23 21:43:31, kevmoo wrote:
> On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> > > make final?
> > 
> > It can't be because of the assignment on line 46.
> 
> That can be done in the initializer. No worries.

refactored to allow final

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:52: Future sendReport(dynamic error,
[StackTrace stackTrace]) async {
On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> Consider making stackTrace a named parameter. Adding more parameters in the
> future will be cleaner.

Makes sense, will do.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:70: req.fields['osVersion'] =
Platform.operatingSystem; // includes the vesion
On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> "vesion" --> "version"
> 
> Should we parse out the name and version?

On investigation, this field does not contain the operating system version (as
text or otherwise) :(

I filed https://github.com/dart-lang/sdk/issues/30018 to be able to get this
info.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:84: } catch (sendError, sendStackTrace) {
On 2017/06/23 21:01:04, kevmoo wrote:
> A tiny more conventional to use `on SocketException` then another blank catch
> 
> Also: good to avoid just throwing `String`

Done.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/crash...
pkg/telemetry/lib/crash_reporting.dart:89: throw 'exception while sending crash
report: $sendError\n$sendStackTrace';
On 2017/06/23 21:01:05, kevmoo wrote:
> long line

Done.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/telem...
File pkg/telemetry/lib/telemetry.dart (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/telem...
pkg/telemetry/lib/telemetry.dart:49: Directory getDartStorageDirectory() {
On 2017/06/23 21:01:05, kevmoo wrote:
> Could just be =>

Done.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/lib/telem...
pkg/telemetry/lib/telemetry.dart:89: //
https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Va...
On 2017/06/23 21:01:05, kevmoo wrote:
> long lines

Done.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/pubspec.yaml
File pkg/telemetry/pubspec.yaml (right):

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/pubspec.y...
pkg/telemetry/pubspec.yaml:3: version: 0.0.1
On 2017/06/23 21:01:05, kevmoo wrote:
> 0.0.1-dev

Done.

https://codereview.chromium.org/2954733002/diff/20001/pkg/telemetry/pubspec.y...
pkg/telemetry/pubspec.yaml:12: path: ^1.4.0
On 2017/06/23 21:01:05, kevmoo wrote:
> sort: p before s

Done.

[kevmoo, 2017-06-26 15:57:30]

On 2017/06/26 15:51:51, devoncarew wrote:
> On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> > Is there a more general name? It seems odd to name a package 'telemetry'
when
> it
> > supports both telemetry and crash reporting.
> 
> This was the best name I could think of that encompassed managing an Analytics
> object, common error messages for the sdk tools, and a crash reporting
library.
> Happy to have a different name - I could think of no better to date :)
> 
> I don't think we'll ever publish this package, just have it in the sdk as a
sdk
> DEP.

pub will need to depend on this, and it's floating in its own repo – but it is
not a package, either.

So I guess as long as we get https://github.com/dart-lang/pub/issues/1305 we can
always reference this package by path

[devoncarew, 2017-06-26 16:03:32]

On 2017/06/26 15:57:30, kevmoo wrote:
> On 2017/06/26 15:51:51, devoncarew wrote:
> > On 2017/06/23 21:41:09, Brian Wilkerson wrote:
> > > Is there a more general name? It seems odd to name a package 'telemetry'
> when
> > it
> > > supports both telemetry and crash reporting.
> > 
> > This was the best name I could think of that encompassed managing an
Analytics
> > object, common error messages for the sdk tools, and a crash reporting
> library.
> > Happy to have a different name - I could think of no better to date :)
> > 
> > I don't think we'll ever publish this package, just have it in the sdk as a
> sdk
> > DEP.
> 
> pub will need to depend on this, and it's floating in its own repo – but
it is
> not a package, either.
> 
> So I guess as long as we get https://github.com/dart-lang/pub/issues/1305 we
can
> always reference this package by path

https://github.com/dart-lang/pub/issues/1305 looks great! Pub could also move
into the repo - a mono-repo for things that ship in the sdk would simplify some
things.

[devoncarew, 2017-06-29 23:46:36]

Description was changed from

==========
Add initial version of analytics and crash reporting package.

BUG=
==========

to

==========
Add initial version of analytics and crash reporting package.

BUG=
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/8e45759a674615b792a8916c93ba1ce12960eeeb
==========

[devoncarew, 2017-06-29 23:46:37]

Committed patchset #3 (id:40001) manually as
8e45759a674615b792a8916c93ba1ce12960eeeb (presubmit successful).