Stats collection. #2447

ldez · 2017-11-22T09:17:18Z

What does this PR do?

#2369

This feature is disabled by default.

In the future, as discussed in the proposal, this may be enabled by default.

Motivation

#2369

More

Added/updated tests
Added/updated documentation

nmengin

LGTM 👏 👍

juliens

LGTM

SantoDE

LGTM 👼

mmatur

LGTM 👏

vdemeester · 2017-11-22T15:47:03Z

cmd/traefik/traefik.go

+func collect(globalConfiguration *configuration.GlobalConfiguration) {
+	ticker := time.NewTicker(24 * time.Hour)
+	safe.Go(func() {
+		time.Sleep(10 * time.Minute)


I'm not sure understand why this first call of Collect is required 👼 I guess you want the first one after 10min and then each and every day, right.

exactly, we wait 10 min for eliminate some quick restart.

Can we document the delay in basics.md as well?

vdemeester · 2017-11-22T15:51:10Z

collector/collector.go

+	"strconv"
+	"time"
+
+	"github.com/containous/traefik/cmd/traefik/anonymize"


🤔 why anonymize is under cmd/traefik package ?

(or why collector is in a different parent package that anonimize ?)

Because before this PR it was use only by traefik bug command.

I can move the package anonymize at the root of the project.

dtomcej

LGTM

dtomcej · 2017-11-22T16:07:39Z

collector/collector.go

+	"github.com/mitchellh/hashstructure"
+)
+
+const containousURL = "https://collect.traefik.io"


This is probably fine, but we might want this overridable for testing, and to allow people to post the stats to their own endpoint if they want. This would also improve public perception, as they can see EXACTLY what is being sent by posting to their own endpoint.

dtomcej · 2017-11-22T16:09:03Z

collector/collector.go

+	buf := new(bytes.Buffer)
+	json.NewEncoder(buf).Encode(data)
+
+	_, err = makeHTTPClient().Post(containousURL, "application/json; charset=utf-8", buf)


This might be a dumb question, but do we want this to retry/or log the data on failure? rather than just the error?

It's not critical for the application, we think the retry/log response is not needed in this case.

dtomcej · 2017-11-22T16:10:22Z

docs/basics.md

+
+```bash
+./traefik --sendAnonymousUsage=true
+```


Might want to note that in the future, that this will be enabled by default.

emilevauge

Thanks @ldez for this great PR. Few comments though ;)

emilevauge · 2017-11-24T10:34:19Z

collector/collector.go

+		BuildDate:     version.BuildDate,
+		Hash:          strconv.FormatUint(hashConf, 10),
+		Configuration: base64.StdEncoding.EncodeToString([]byte(anonConfig)),
+		EmitDate:      time.Now().Format(time.RFC3339),


Is-it needed ?

emilevauge · 2017-11-24T10:40:53Z

collector/collector.go

+	}
+
+	buf := new(bytes.Buffer)
+	json.NewEncoder(buf).Encode(data)


error is shadowed here

emilevauge · 2017-11-24T10:45:43Z

docs/basics.md

+Once a day, we collect:
+- the Træfik version
+- a hash of the configuration
+- an **obfuscated and anonymous version** of the static configuration:


s/obfuscated and anonymous version/anonymous version

emilevauge · 2017-11-24T10:47:10Z

docs/basics.md

+
+By default we anonymize all configuration fields, except fields tagged with `export=true`.
+
+You can check all fields in the [godoc](https://godoc.org/github.com/containous/traefik).


Maybe we should give a this link instead https://godoc.org/github.com/containous/traefik/configuration#GlobalConfiguration

emilevauge

LGTM 👏

m3co-code

LGTM, I only left two very small nit-picks :)

m3co-code · 2017-11-24T16:47:46Z

docs/basics.md

+- an **anonymous version** of the static configuration:
+    - token, user name, password, URL, IP, domain, email, etc, are removed
+
+!!! warning


I think this is rather another note than a warning.

m3co-code · 2017-11-24T16:50:00Z

cmd/traefik/traefik.go

+			log.Debug(err)
+		}
+		for {
+			select {


You don't really need the select case here, because <-ticker.C will block anyway.

timoreimann

I'd call out the typo fixes as mandatory, everything else as recommended / points of discussion.

timoreimann · 2017-11-24T21:37:00Z

cmd/traefik/traefik.go

+	if globalConfiguration.SendAnonymousUsage {
+		log.Info(`
+Stats collection is enabled.
+Many thanks to contribute to Traefik improvement by allowing us to receive anonymous information from your configuration.


I think it must say (emphasis by me)

Many thanks for contributing to Traefik's improvement [...]

timoreimann · 2017-11-24T21:48:51Z

cmd/traefik/traefik.go

+}
+
+func collect(globalConfiguration *configuration.GlobalConfiguration) {
+	ticker := time.NewTicker(24 * time.Hour)


I'd make it explicit that we want a never-ending ticker (we never call ticker.Stop()) by invoking time.Tick() here.

timoreimann · 2017-11-24T21:55:25Z

collector/collector.go

+	"github.com/mitchellh/hashstructure"
+)
+
+const containousURL = "https://collect.traefik.io/619df80498b60f985d766ce62f912b7c"


Can we have a more expressive name to indicate what's happening behind this URL (like collectorURL), possibly even a GoDoc?

timoreimann · 2017-11-24T21:55:44Z

collector/collector.go

+		return err
+	}
+
+	log.Infof("Stats sent on %s: %s", containousURL, anonConfig)


timoreimann · 2017-11-24T21:56:17Z

collector/collector.go

+		return err
+	}
+
+	log.Infof("Stats sent on %s: %s", containousURL, anonConfig)


I'd write Anonymous stats sent to [...].

(Emphasis by me.)

timoreimann · 2017-11-24T21:59:01Z

cmd/traefik/traefik.go

+func collect(globalConfiguration *configuration.GlobalConfiguration) {
+	ticker := time.NewTicker(24 * time.Hour)
+	safe.Go(func() {
+		time.Sleep(10 * time.Minute)


Can we document the delay in basics.md as well?

timoreimann · 2017-11-24T22:03:34Z

cmd/traefik/traefik.go

+		for range ticker.C {
+			err := collector.Collect(globalConfiguration)
+			if err != nil {
+				log.Debug(err)


It is possible to have just one call to Collect like this (slightly adopted from here):

for time.Sleep(10 * time.Minute); ; <-ticker.C { if err := collector.Collect(globalConfiguration); err != nil { log.Debug(err) } }

(Simplified Go Playground example.)

ldez · 2017-11-25T00:18:48Z

@timoreimann I have take account of all your remarks.

timoreimann

LGTM 👏

ldez added kind/enhancement a new or improved feature. status/2-needs-review labels Nov 22, 2017

ldez added this to the 1.5 milestone Nov 22, 2017

traefiker added the size/L label Nov 22, 2017

emilevauge mentioned this pull request Nov 22, 2017

Collecting stats #2369

Closed

ldez force-pushed the feature/collect-anonymous-usage branch 3 times, most recently from c1f2e4e to 6116f62 Compare November 22, 2017 10:54

nmengin approved these changes Nov 22, 2017

View reviewed changes

ldez force-pushed the feature/collect-anonymous-usage branch from 6116f62 to 8ad23d0 Compare November 22, 2017 14:17

juliens approved these changes Nov 22, 2017

View reviewed changes

SantoDE approved these changes Nov 22, 2017

View reviewed changes

mmatur approved these changes Nov 22, 2017

View reviewed changes

mmatur added status/3-needs-merge and removed status/2-needs-review labels Nov 22, 2017

traefiker added the status/4-merge-in-progress label Nov 22, 2017

traefiker force-pushed the feature/collect-anonymous-usage branch from 8ad23d0 to b264b07 Compare November 22, 2017 14:46

emilevauge added bot/no-merge and removed status/4-merge-in-progress status/3-needs-merge labels Nov 22, 2017

ldez added the status/2-needs-review label Nov 22, 2017

ldez requested review from timoreimann, m3co-code and dtomcej November 22, 2017 14:52

ldez force-pushed the feature/collect-anonymous-usage branch from b264b07 to 3b8509d Compare November 22, 2017 15:49

vdemeester reviewed Nov 22, 2017

View reviewed changes

dtomcej approved these changes Nov 22, 2017

View reviewed changes

ldez force-pushed the feature/collect-anonymous-usage branch 2 times, most recently from 4ecad76 to ddcd87e Compare November 24, 2017 09:23

emilevauge requested changes Nov 24, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Nov 24, 2017

ldez force-pushed the feature/collect-anonymous-usage branch from ddcd87e to ba322d7 Compare November 24, 2017 11:05

emilevauge approved these changes Nov 24, 2017

View reviewed changes

traefiker removed the contributor/waiting-for-corrections label Nov 24, 2017

m3co-code approved these changes Nov 24, 2017

View reviewed changes

ldez force-pushed the feature/collect-anonymous-usage branch from ba322d7 to 4e7500f Compare November 24, 2017 17:00

timoreimann suggested changes Nov 24, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Nov 24, 2017

timoreimann reviewed Nov 24, 2017

View reviewed changes

ldez removed the contributor/waiting-for-corrections label Nov 25, 2017

timoreimann approved these changes Nov 25, 2017

View reviewed changes

ldez added status/3-needs-merge and removed bot/no-merge status/2-needs-review labels Nov 25, 2017

traefiker added the status/4-merge-in-progress label Nov 25, 2017

ldez added 2 commits November 25, 2017 12:22

feat: stats collection.

ee329a1

refactor: review comments.

8d13309

traefiker force-pushed the feature/collect-anonymous-usage branch from f4000db to 8d13309 Compare November 25, 2017 12:22

traefiker removed the status/4-merge-in-progress label Nov 25, 2017

traefiker merged commit 0ca65f9 into traefik:master Nov 25, 2017

traefiker removed the status/3-needs-merge label Nov 25, 2017

ldez deleted the feature/collect-anonymous-usage branch November 25, 2017 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats collection. #2447

Stats collection. #2447

ldez commented Nov 22, 2017 •

edited

Loading

nmengin left a comment

juliens left a comment

SantoDE left a comment

mmatur left a comment

vdemeester Nov 22, 2017

ldez Nov 22, 2017

timoreimann Nov 24, 2017

vdemeester Nov 22, 2017

vdemeester Nov 22, 2017

ldez Nov 22, 2017

dtomcej left a comment

dtomcej Nov 22, 2017

dtomcej Nov 22, 2017

ldez Nov 22, 2017

dtomcej Nov 22, 2017

emilevauge left a comment

emilevauge Nov 24, 2017

emilevauge Nov 24, 2017

emilevauge Nov 24, 2017

emilevauge Nov 24, 2017

emilevauge left a comment

m3co-code left a comment

m3co-code Nov 24, 2017

m3co-code Nov 24, 2017

timoreimann left a comment •

edited

Loading

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017

timoreimann Nov 24, 2017 •

edited

Loading

ldez commented Nov 25, 2017

timoreimann left a comment


		By default we anonymize all configuration fields, except fields tagged with `export=true`.

		You can check all fields in the [godoc](https://godoc.org/github.com/containous/traefik).

Stats collection. #2447

Stats collection. #2447

Conversation

ldez commented Nov 22, 2017 • edited Loading

What does this PR do?

Motivation

More

nmengin left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

SantoDE left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

m3co-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

ldez commented Nov 25, 2017

timoreimann left a comment

Choose a reason for hiding this comment

ldez commented Nov 22, 2017 •

edited

Loading

timoreimann left a comment •

edited

Loading

timoreimann Nov 24, 2017 •

edited

Loading