do not send diagnostics if organization is cockroach labs #20790

dianasaur323 · 2017-12-18T06:04:36Z

Not too sure if this is the right approach, but basically adding logic where we check for whether or not a user has enabled diagnostics to also check for if the user has an organization that contains Cockroach Labs. This way, we can stop counting backup / restores that are related to internal work.

Fixes https://github.com/cockroachlabs/registration/issues/91

cockroach-teamcity · 2017-12-18T06:04:40Z

This change is

dt · 2017-12-18T16:19:52Z

I think we might actually want to report from our own clusters but just want to indicate that they are ours, so we can filter them server-side. That way we can validate that our reporting is working as expected?

dianasaur323 · 2017-12-18T18:48:05Z

Well, the reason I didn't go with that approach was because then we would have to send everyone's organization with each update? If that's not a big deal, I guess we could just send it and then filter on the server side? In that case, we might even want to add a column or something to mark if a cluster is Cockroach or not? That seems heavy handed.

dt · 2017-12-18T21:41:59Z

discussed offline and think sending a testing flag is the way to go, and we can set that client side if the org contains "Cockroach Labs" or whatever, then exclude testing=true rows server side when doing analytics in a follow up.

dianasaur323 · 2017-12-19T15:48:27Z

Cool, also going to do a schema change on the reg cluster by adding in a testing column in both the clusters and updates table.

nstewart · 2018-01-22T22:20:54Z

small nit, any reason not to call this internal (or something else)? Only because we have user stories around testing clusters that aren't limited to CRL. Just thinking about understandability for future employees.

dianasaur323 · 2018-01-27T02:34:30Z

Correction, I don't think we need to also include this in updates, since if at any point, a cluster becomes a CRL cluster, we should just omit it.

Okay on calling this column internal.

andreimatei · 2018-01-28T20:38:34Z

last commit is badly named and superfluous?

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

dianasaur323 · 2018-01-28T20:48:30Z

@andreimatei I'll fix and squash that last commit with a better message once this passes tests - I botched a rebase and pulled a hail mary to get rid of a bunch of random commits.

dianasaur323 · 2018-01-31T13:43:09Z

@dt could I get a review on this? thank you!

couchand · 2018-01-31T15:56:17Z

pkg/server/updates_test.go

@@ -224,6 +233,9 @@ func TestReportUsage(t *testing.T) {
 		if minExpected, actual := len(params.StoreSpecs), len(r.last.Stores); minExpected > actual {
 			return errors.Errorf("expected at least %v stores got %v", minExpected, actual)
 		}
+		if expected, actual := "true", r.last.internal; expected != actual {


probably want to have a test case where this is false

dianasaur323 · 2018-02-01T22:52:16Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

pkg/server/updates_test.go, line 236 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

probably want to have a test case where this is false

So the reason why I do it this way is that in this TestReportUsage function, I actually set the organization to Cockroach Labs, and once that's done, it can't go back to false anymore. I do test whether or not things are initialized as false in the test case above under TestCheckVersion? Is that adequate?

Comments from Reviewable

couchand · 2018-02-05T15:45:38Z

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

pkg/server/updates_test.go, line 236 at r1 (raw file):

Previously, dianasaur323 (Diana Hsieh) wrote…

So the reason why I do it this way is that in this TestReportUsage function, I actually set the organization to Cockroach Labs, and once that's done, it can't go back to false anymore. I do test whether or not things are initialized as false in the test case above under TestCheckVersion? Is that adequate?

Oh, I had missed that. Thanks!

Comments from Reviewable

dianasaur323 · 2018-02-09T00:54:15Z

ok for me to merge? Could I get an LGTM from someone? thanks!

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

bdarnell · 2018-02-09T17:11:02Z

, although this starts the long process of updating all our tools that create clusters to set this setting.

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

dianasaur323 · 2018-02-10T13:57:14Z

Awesome, thanks! I'm going to go ahead and merge this, and perhaps we can start that process after code freeze.

knz · 2018-02-10T21:37:07Z

This broke master, looking into it

- server: fix TestReportUsage (broken by cockroachdb#20790) - cli/interactive_tests: do not check for proper termination by `cockroach quit` until cockroachdb#22536 is fixed. Release note: none

dianasaur323 · 2018-02-13T22:57:17Z

thanks, @knz!

dianasaur323 requested a review from a team December 18, 2017 06:04

dianasaur323 requested a review from dt December 18, 2017 06:04

dianasaur323 force-pushed the update-diagnostics branch from 51ba537 to 1500675 Compare December 18, 2017 13:59

dianasaur323 requested review from a team January 28, 2018 20:23

dianasaur323 requested a review from a team as a code owner January 28, 2018 20:23

dianasaur323 requested a review from a team January 28, 2018 20:23

dianasaur323 requested a review from a team as a code owner January 28, 2018 20:23

dianasaur323 requested review from a team and removed request for a team January 28, 2018 20:23

dianasaur323 removed request for a team January 28, 2018 20:24

dianasaur323 force-pushed the update-diagnostics branch 4 times, most recently from c9422c2 to e2443fd Compare January 28, 2018 20:32

dianasaur323 force-pushed the update-diagnostics branch 5 times, most recently from 3ccf11f to 04ac51a Compare January 31, 2018 12:50

server: do not send diagonistics if internal to crdb

04ac51a

couchand reviewed Jan 31, 2018

View reviewed changes

dianasaur323 merged commit 058a964 into cockroachdb:master Feb 10, 2018

dianasaur323 deleted the update-diagnostics branch February 10, 2018 13:58

knz mentioned this pull request Feb 10, 2018

server, cli/interactive_tests: un-break master #22568

Merged

dianasaur323 mentioned this pull request Mar 21, 2018

server: tag internal clusters as internal #24111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not send diagnostics if organization is cockroach labs #20790

do not send diagnostics if organization is cockroach labs #20790

dianasaur323 commented Dec 18, 2017 •

edited

Loading

cockroach-teamcity commented Dec 18, 2017

dt commented Dec 18, 2017

dianasaur323 commented Dec 18, 2017 •

edited

Loading

dt commented Dec 18, 2017

dianasaur323 commented Dec 19, 2017 •

edited

Loading

nstewart commented Jan 22, 2018

dianasaur323 commented Jan 27, 2018

andreimatei commented Jan 28, 2018

dianasaur323 commented Jan 28, 2018

dianasaur323 commented Jan 31, 2018

couchand Jan 31, 2018

dianasaur323 commented Feb 1, 2018

couchand commented Feb 5, 2018

dianasaur323 commented Feb 9, 2018

bdarnell commented Feb 9, 2018

dianasaur323 commented Feb 10, 2018

knz commented Feb 10, 2018

dianasaur323 commented Feb 13, 2018

do not send diagnostics if organization is cockroach labs #20790

do not send diagnostics if organization is cockroach labs #20790

Conversation

dianasaur323 commented Dec 18, 2017 • edited Loading

cockroach-teamcity commented Dec 18, 2017

dt commented Dec 18, 2017

dianasaur323 commented Dec 18, 2017 • edited Loading

dt commented Dec 18, 2017

dianasaur323 commented Dec 19, 2017 • edited Loading

nstewart commented Jan 22, 2018

dianasaur323 commented Jan 27, 2018

andreimatei commented Jan 28, 2018

dianasaur323 commented Jan 28, 2018

dianasaur323 commented Jan 31, 2018

couchand Jan 31, 2018

Choose a reason for hiding this comment

dianasaur323 commented Feb 1, 2018

couchand commented Feb 5, 2018

dianasaur323 commented Feb 9, 2018

bdarnell commented Feb 9, 2018

dianasaur323 commented Feb 10, 2018

knz commented Feb 10, 2018

dianasaur323 commented Feb 13, 2018

dianasaur323 commented Dec 18, 2017 •

edited

Loading

dianasaur323 commented Dec 18, 2017 •

edited

Loading

dianasaur323 commented Dec 19, 2017 •

edited

Loading