cmd/syncthing: Add more stats to usage reports (ref #3628) #4347

AudriusButkevicius · 2017-09-03T13:53:33Z

Tested manually.
Not sure if we need to bump the version here.

AudriusButkevicius · 2017-09-06T09:06:47Z

Apart from tests, what else needs to be done here @calmh?

calmh · 2017-09-06T09:08:59Z

I think either implement what we discussed on IRC about retaining the previous report version until the new one is accepted, or a huge push to get all the stuff we really want into the report in in one swoop... Not necessarily in this PR of course, but before we let this into the wild...

AudriusButkevicius · 2017-09-09T17:43:50Z

So added incremental support, but did it without paying too much attention or thinking about it.
Tested here and there, seems to work, but give it a proper review pls.

calmh

Uh, it looks reasonable but my review time got interrupted by kids and concentration went out the window so I'll make another attempt soon

calmh · 2017-09-10T08:10:26Z

cmd/syncthing/usage_report.go

+
+func (s *usageReportingService) CommitConfiguration(from, to config.Configuration) bool {
+	if from.Options.URAccepted != to.Options.URAccepted || from.Options.URUniqueID != to.Options.URUniqueID || from.Options.URURL != to.Options.URURL {
+		s.timer.Reset(time.Duration(to.Options.URInitialDelayS) * time.Second)


I suspect this is a data race on s.timer

Why? s.timer is not touched once it's initialized.

AudriusButkevicius · 2017-09-10T09:14:08Z

@st-review retest

st-review · 2017-09-10T09:14:08Z

I'm sorry, @AudriusButkevicius. I'm afraid I don't know what you mean. I know how to don't, lgtm, merge, prevent, rebuild, squash, stop.

AudriusButkevicius · 2017-09-10T09:22:57Z

@st-review rebuild

calmh · 2017-09-10T09:38:40Z

It's used by Serve() and reset by the config commit. On 10 Sep 2017, at 11:14, Audrius Butkevicius <notifications@github.com<mailto:notifications@github.com>> wrote: @AudriusButkevicius commented on this pull request.

________________________________ In cmd/syncthing/usage_report.go<#4347 (comment)>:

}

} } +func (s *usageReportingService) VerifyConfiguration(from, to config.Configuration) error { + return nil +} + +func (s *usageReportingService) CommitConfiguration(from, to config.Configuration) bool { + if from.Options.URAccepted != to.Options.URAccepted || from.Options.URUniqueID != to.Options.URUniqueID || from.Options.URURL != to.Options.URURL { + s.timer.Reset(time.Duration(to.Options.URInitialDelayS) * time.Second) Why? s.timer is not touched once it's initialized. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#4347 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAHp8vHHMz3BSpM9p5JX3uLxf00cg4CYks5sg6hZgaJpZM4PLMOK>.

AudriusButkevicius · 2017-09-10T09:44:30Z

Sure but that's just reads, which means it should be fine?

calmh · 2017-09-10T10:09:28Z

It's not, but don't take my word for it

package main

import (
	"fmt"
	"testing"
	"time"
)

func TestTimer(t *testing.T) {
	test := &test{
		t: time.NewTimer(0),
	}
	go test.Serve()
	for i := 0; i < 5; i++ {
		test.Commit()
		time.Sleep(time.Second)
	}
}

type test struct {
	t *time.Timer
}

func (t *test) Serve() {
	t.t.Reset(0)
	for {
		<-t.t.C
		fmt.Println("tick")
		t.t.Reset(time.Second)
	}
}

func (t *test) Commit() {
	fmt.Println("resetting timer")
	t.t.Reset(10 * time.Millisecond)
}

jb@unu:~/t/timer $ go test -race
resetting timer
tick
tick
==================
WARNING: DATA RACE
Write at 0x00c420088650 by goroutine 6:
  time.(*Timer).Reset()
      /usr/local/go1.9/src/time/sleep.go:127 +0x98
  _/Users/jb/tmp/timer.(*test).Commit()
      /Users/jb/tmp/timer/timer_test.go:35 +0xb1
  _/Users/jb/tmp/timer.TestTimer()
      /Users/jb/tmp/timer/timer_test.go:15 +0xb8
  testing.tRunner()
      /usr/local/go1.9/src/testing/testing.go:746 +0x16c

Previous write at 0x00c420088650 by goroutine 8:
  time.(*Timer).Reset()
      /usr/local/go1.9/src/time/sleep.go:127 +0x98
  _/Users/jb/tmp/timer.(*test).Serve()
      /Users/jb/tmp/timer/timer_test.go:25 +0x59

AudriusButkevicius · 2017-09-10T10:12:55Z

Is that a race internally setting something in the timer or accessing the timer in general.
As in, if I wrap things in a lock, do I need to wrap the channel retrieval with a lock too?

calmh · 2017-09-10T10:14:48Z

I think it's just the Reset() that does internal unlocked writes, so protecting that should be enough. There's a whole discussion in the Reset() documentation that says you can't do Reset() concurrently with the channel read, but I think that is for correctness and not relevant as we don't really care if the timer fires at exactly the same time we call Reset(). (That stuff matters if the timer represents a timeout error that should not fire in case of Reset() etc but we don't care.)

The race detector should tell us if it's otherwise.

AudriusButkevicius · 2017-09-10T10:18:39Z

Anyway, this uses a channel now.

AudriusButkevicius · 2017-09-10T10:22:37Z

gui/default/syncthing/core/syncthingController.js

+                if ($scope.system && $scope.config.options.urSeen < $scope.system.urVersionMax) {
+                    // Usage reporting format has changed, prompt the user to re-accept.
+                    $('#ur').modal();
+                }


This is a bit racy, as we rely on $scope.system being populated first to actually do the prompt.

calmh · 2017-09-20T06:29:30Z

The block stats and connection types objects are shown also in the version two preview:

AudriusButkevicius · 2017-09-23T15:07:45Z

Fixed, I hope.

https://stackoverflow.com/questions/31200906/custom-child-directive-accessing-scope-of-parent

calmh

Otherwise lgtm

calmh · 2017-09-25T06:01:31Z

gui/default/syncthing/usagereport/usageReportPreviewModalView.html

@@ -6,7 +6,7 @@
    <p translate>The aggregated statistics are publicly available at the URL below.</p>
    <p><a href="https://data.syncthing.net/" target="_blank">https://data.syncthing.net/</a></p>
    <label translate>Version</label>
-    <select class="form-control" ng-model="reportDataPreviewVersion" ng-change="refreshReportDataPreview()">
+    <select id="urPreviewVersion" class="form-control" ng-model="$parent.$parent.reportDataPreviewVersion" ng-change="refreshReportDataPreview()" >


calmh · 2017-09-25T06:03:07Z

cmd/syncthing/usage_report.go

@@ -234,13 +234,14 @@ func (s *usageReportingService) sendUsageReport() error {

 func (s *usageReportingService) Serve() {
 	s.stop = make(chan struct{})
-	s.timer.Reset(time.Duration(s.cfg.Options().URInitialDelayS) * time.Second)
-
+	t := time.NewTimer(time.Duration(s.cfg.Options().URInitialDelayS) * time.Second)


defer t.Stop()

calmh · 2017-09-25T06:11:30Z

gui/default/syncthing/core/syncthingController.js

@@ -133,7 +135,11 @@ angular.module('syncthing.core')
            }).error($scope.emitHTTPError);

            $http.get(urlbase + '/svc/report').success(function (data) {
-                $scope.reportData = data;
+                $scope.reportDataPreview = $scope.reportData = data;
+                if ($scope.system && $scope.config.options.urSeen < $scope.system.urVersionMax) {


This might not work for candidate releases. For those we set urAccepted = urVersionMax (or the equivalent what the variables are a called) on startup, but we don't tweak urSeen so this dialog would be triggered unnecessarily.

AudriusButkevicius · 2017-10-05T19:22:16Z

Updated and added usage stats for features of the ignore patterns.

imsodin · 2017-10-05T19:38:24Z

lib/model/model.go

+				}
+
+				// Noops, remove
+				if strings.HasPrefix(line, "**") {


I don't think doube star prefixes are noops, e.g. **/temp isn't rooted, is it?

AudriusButkevicius · 2017-10-06T09:05:30Z

I need to remove **/ I guess as that is equivalent behaviour without the prefix.

AudriusButkevicius · 2017-10-06T21:18:29Z

@st-review rebuild

calmh · 2017-10-07T07:22:45Z

This is good to go I think?

AudriusButkevicius · 2017-10-07T11:24:16Z

Yes

calmh · 2017-10-12T06:16:41Z

@st-review merge

st-review · 2017-10-12T06:16:55Z

👌 Merged as 2760d03. Thanks, @AudriusButkevicius!

GitHub-Pull-Request: #4347

AudriusButkevicius · 2017-10-12T07:35:31Z

I assume this need schema changes in the db?

calmh · 2017-10-12T07:53:29Z

Yes, it needs handling on the receiving side. I think we have a couple of migrations in that code already, basically ALTER TABLE ... ADD COLUMN and new fields in the struct it unmarshals to etc

calmh · 2017-10-12T07:54:19Z

I guess if you're looking at a new challenge maybe refactor it to use a json document type in the database so that we'd just need to hack the various analytics parts in the future and not the whole storage part

cmd/syncthing: Add more stats to usage reports (ref syncthing#3628)

478db14

AudriusButkevicius added 2 commits September 9, 2017 14:13

Merge remote-tracking branch 'upstream/master' into usage

db0a6d0

Support incremental usage reports

83818e6

calmh reviewed Sep 10, 2017

View reviewed changes

Use a channel to signal rerun

e9f9f3f

AudriusButkevicius commented Sep 10, 2017

View reviewed changes

AudriusButkevicius added 2 commits September 23, 2017 15:28

Merge remote-tracking branch 'upstream/master' into usage

8c6f0c0

Screw web development

a193537

Add warning

a7a25d7

calmh reviewed Sep 25, 2017

View reviewed changes

AudriusButkevicius added 3 commits October 3, 2017 23:55

Merge remote-tracking branch 'upstream/master' into usage

533becd

Update seen when switching between releases

3e5edff

lib/model: Add ignore pattern usage stats

351bf97

imsodin reviewed Oct 5, 2017

View reviewed changes

gofmt

88a2f19

Remove one non-noop

62f6b86

AudriusButkevicius force-pushed the usage branch from 80e75f3 to 62f6b86 Compare October 6, 2017 18:03

st-review closed this Oct 12, 2017

st-review pushed a commit that referenced this pull request Oct 12, 2017

cmd/syncthing: Add more stats to usage reports (ref #3628)

2760d03

GitHub-Pull-Request: #4347

AudriusButkevicius deleted the usage branch October 12, 2017 07:11

st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Oct 12, 2018

syncthing locked and limited conversation to collaborators Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/syncthing: Add more stats to usage reports (ref #3628) #4347

cmd/syncthing: Add more stats to usage reports (ref #3628) #4347

AudriusButkevicius commented Sep 3, 2017

AudriusButkevicius commented Sep 6, 2017

calmh commented Sep 6, 2017

AudriusButkevicius commented Sep 9, 2017

calmh left a comment

calmh Sep 10, 2017

AudriusButkevicius Sep 10, 2017

AudriusButkevicius commented Sep 10, 2017

st-review commented Sep 10, 2017

AudriusButkevicius commented Sep 10, 2017

calmh commented Sep 10, 2017 via email

AudriusButkevicius commented Sep 10, 2017

calmh commented Sep 10, 2017

AudriusButkevicius commented Sep 10, 2017

calmh commented Sep 10, 2017 •

edited

AudriusButkevicius commented Sep 10, 2017

AudriusButkevicius Sep 10, 2017 •

edited

calmh commented Sep 20, 2017

AudriusButkevicius commented Sep 23, 2017

calmh left a comment

calmh Sep 25, 2017

calmh Sep 25, 2017

calmh Sep 25, 2017

AudriusButkevicius commented Oct 5, 2017

imsodin Oct 5, 2017

AudriusButkevicius commented Oct 6, 2017

AudriusButkevicius commented Oct 6, 2017

calmh commented Oct 7, 2017

AudriusButkevicius commented Oct 7, 2017

calmh commented Oct 12, 2017

st-review commented Oct 12, 2017

AudriusButkevicius commented Oct 12, 2017

calmh commented Oct 12, 2017

calmh commented Oct 12, 2017

cmd/syncthing: Add more stats to usage reports (ref #3628) #4347

cmd/syncthing: Add more stats to usage reports (ref #3628) #4347

Conversation

AudriusButkevicius commented Sep 3, 2017

AudriusButkevicius commented Sep 6, 2017

calmh commented Sep 6, 2017

AudriusButkevicius commented Sep 9, 2017

calmh left a comment

Choose a reason for hiding this comment

calmh Sep 10, 2017

Choose a reason for hiding this comment

AudriusButkevicius Sep 10, 2017

Choose a reason for hiding this comment

AudriusButkevicius commented Sep 10, 2017

st-review commented Sep 10, 2017

AudriusButkevicius commented Sep 10, 2017

calmh commented Sep 10, 2017 via email

AudriusButkevicius commented Sep 10, 2017

calmh commented Sep 10, 2017

AudriusButkevicius commented Sep 10, 2017

calmh commented Sep 10, 2017 • edited

AudriusButkevicius commented Sep 10, 2017

AudriusButkevicius Sep 10, 2017 • edited

Choose a reason for hiding this comment

calmh commented Sep 20, 2017

AudriusButkevicius commented Sep 23, 2017

calmh left a comment

Choose a reason for hiding this comment

calmh Sep 25, 2017

Choose a reason for hiding this comment

calmh Sep 25, 2017

Choose a reason for hiding this comment

calmh Sep 25, 2017

Choose a reason for hiding this comment

AudriusButkevicius commented Oct 5, 2017

imsodin Oct 5, 2017

Choose a reason for hiding this comment

AudriusButkevicius commented Oct 6, 2017

AudriusButkevicius commented Oct 6, 2017

calmh commented Oct 7, 2017

AudriusButkevicius commented Oct 7, 2017

calmh commented Oct 12, 2017

st-review commented Oct 12, 2017

AudriusButkevicius commented Oct 12, 2017

calmh commented Oct 12, 2017

calmh commented Oct 12, 2017

calmh commented Sep 10, 2017 •

edited

AudriusButkevicius Sep 10, 2017 •

edited