Skip to content

Commit 363ea11

Browse files
committed
simplify the telemetry disabled reporting logic
1 parent b6d1f7e commit 363ea11

File tree

2 files changed

+44
-31
lines changed

2 files changed

+44
-31
lines changed

coderd/telemetry/telemetry.go

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -158,12 +158,6 @@ func (r *remoteReporter) reportSync(snapshot *Snapshot) {
158158
r.options.Logger.Debug(r.ctx, "bad response from telemetry server", slog.F("status", resp.StatusCode))
159159
return
160160
}
161-
if err := r.options.Database.UpsertTelemetryItem(r.ctx, database.UpsertTelemetryItemParams{
162-
Key: string(TelemetryItemKeyLastTelemetryUpdate),
163-
Value: "",
164-
}); err != nil {
165-
r.options.Logger.Debug(r.ctx, "upsert last telemetry update", slog.Error(err))
166-
}
167161
r.options.Logger.Debug(r.ctx, "submitted snapshot")
168162
}
169163

@@ -194,7 +188,18 @@ func (r *remoteReporter) isClosed() bool {
194188
}
195189
}
196190

191+
func (r *remoteReporter) recordTelemetryEnabled() {
192+
if err := r.options.Database.UpsertTelemetryItem(r.ctx, database.UpsertTelemetryItemParams{
193+
Key: string(TelemetryItemKeyTelemetryEnabled),
194+
Value: "",
195+
}); err != nil {
196+
r.options.Logger.Debug(r.ctx, "upsert last telemetry report", slog.Error(err))
197+
}
198+
}
199+
197200
func (r *remoteReporter) RunSnapshotter() {
201+
r.recordTelemetryEnabled()
202+
198203
first := true
199204
ticker := time.NewTicker(r.options.SnapshotFrequency)
200205
defer ticker.Stop()
@@ -349,17 +354,26 @@ func checkIDPOrgSync(ctx context.Context, db database.Store, values *codersdk.De
349354

350355
func (r *remoteReporter) ReportDisabledIfNeeded() error {
351356
db := r.options.Database
352-
lastTelemetryUpdate, telemetryUpdateErr := db.GetTelemetryItem(r.ctx, string(TelemetryItemKeyLastTelemetryUpdate))
353-
if telemetryUpdateErr != nil && !errors.Is(telemetryUpdateErr, sql.ErrNoRows) {
354-
r.options.Logger.Debug(r.ctx, "get last telemetry update at", slog.Error(telemetryUpdateErr))
357+
telemetryEnabled, telemetryEnabledErr := db.GetTelemetryItem(r.ctx, string(TelemetryItemKeyTelemetryEnabled))
358+
if telemetryEnabledErr != nil && !errors.Is(telemetryEnabledErr, sql.ErrNoRows) {
359+
r.options.Logger.Debug(r.ctx, "get telemetry enabled", slog.Error(telemetryEnabledErr))
355360
}
356361
telemetryDisabled, telemetryDisabledErr := db.GetTelemetryItem(r.ctx, string(TelemetryItemKeyTelemetryDisabled))
357362
if telemetryDisabledErr != nil && !errors.Is(telemetryDisabledErr, sql.ErrNoRows) {
358363
r.options.Logger.Debug(r.ctx, "get telemetry disabled", slog.Error(telemetryDisabledErr))
359364
}
360-
shouldReportDisabledTelemetry := telemetryUpdateErr == nil &&
361-
((telemetryDisabledErr == nil && lastTelemetryUpdate.UpdatedAt.Before(telemetryDisabled.UpdatedAt)) ||
362-
errors.Is(telemetryDisabledErr, sql.ErrNoRows))
365+
// There are 2 scenarios in which we want to report the disabled telemetry:
366+
// 1. The telemetry was enabled at some point, and we haven't reported the disabled telemetry yet.
367+
// 2. The telemetry was enabled at some point, we reported the disabled telemetry, the telemetry
368+
// was enabled again, and then disabled again.
369+
//
370+
// - In both cases, the TelemetryEnabled item will be present.
371+
// - In case 1. the TelemetryDisabled item will not be present.
372+
// - In case 2. the TelemetryDisabled item will be present, and the TelemetryEnabled item will
373+
// be more recent than the TelemetryDisabled item.
374+
shouldReportDisabledTelemetry := telemetryEnabledErr == nil &&
375+
(errors.Is(telemetryDisabledErr, sql.ErrNoRows) ||
376+
(telemetryDisabledErr == nil && telemetryEnabled.UpdatedAt.After(telemetryDisabled.UpdatedAt)))
363377
if !shouldReportDisabledTelemetry {
364378
return nil
365379
}
@@ -1616,9 +1630,9 @@ type Organization struct {
16161630
type TelemetryItemKey string
16171631

16181632
const (
1619-
TelemetryItemKeyHTMLFirstServedAt TelemetryItemKey = "html_first_served_at"
1620-
TelemetryItemKeyLastTelemetryUpdate TelemetryItemKey = "last_telemetry_update"
1621-
TelemetryItemKeyTelemetryDisabled TelemetryItemKey = "telemetry_disabled"
1633+
TelemetryItemKeyHTMLFirstServedAt TelemetryItemKey = "html_first_served_at"
1634+
TelemetryItemKeyTelemetryEnabled TelemetryItemKey = "telemetry_enabled"
1635+
TelemetryItemKeyTelemetryDisabled TelemetryItemKey = "telemetry_disabled"
16221636
)
16231637

16241638
type TelemetryItem struct {

coderd/telemetry/telemetry_test.go

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -365,27 +365,28 @@ func TestReportDisabledIfNeeded(t *testing.T) {
365365
t.Parallel()
366366
db, _ := dbtestutil.NewDB(t)
367367
ctx := testutil.Context(t, testutil.WaitMedium)
368-
serverURL, _, snapshotChan := setupTelemetryServer(t)
368+
serverURL, _, snapshotChan := mockTelemetryServer(t)
369369

370370
options := telemetry.Options{
371-
Database: db,
372-
Logger: testutil.Logger(t),
373-
URL: serverURL,
374-
DeploymentID: uuid.NewString(),
371+
Database: db,
372+
Logger: testutil.Logger(t),
373+
URL: serverURL,
374+
DeploymentID: uuid.NewString(),
375+
DisableReportOnClose: true,
375376
}
376377

377378
reporter, err := telemetry.New(options)
378379
require.NoError(t, err)
379380
t.Cleanup(reporter.Close)
380381

381-
// No telemetry updated item, so no report should be sent
382+
// No telemetry enabled item, so no report should be sent
382383
require.NoError(t, reporter.ReportDisabledIfNeeded())
383384
require.Empty(t, snapshotChan)
384385

385-
// Telemetry disabled item not present, and a telemetry update item present
386+
// Telemetry enabled item present, and a telemetry disabled item not present
386387
// Report should be sent
387388
_ = dbgen.TelemetryItem(t, db, database.TelemetryItem{
388-
Key: string(telemetry.TelemetryItemKeyLastTelemetryUpdate),
389+
Key: string(telemetry.TelemetryItemKeyTelemetryEnabled),
389390
Value: "",
390391
})
391392
require.NoError(t, reporter.ReportDisabledIfNeeded())
@@ -397,19 +398,17 @@ func TestReportDisabledIfNeeded(t *testing.T) {
397398
t.Fatal("timeout waiting for snapshot")
398399
}
399400

400-
// Telemetry disabled item present, and a telemetry update item present
401-
// with an updated at time equal to or after the telemetry disabled item
401+
// Telemetry enabled item present, and a more recent telemetry disabled item present
402402
// Report should not be sent
403403
require.NoError(t, reporter.ReportDisabledIfNeeded())
404404
require.Empty(t, snapshotChan)
405405

406-
// Telemetry disabled item present, and a telemetry update item present
407-
// with an updated at time before the telemetry disabled item
406+
// Telemetry enabled item present, and a less recent telemetry disabled item present
408407
// Report should be sent
409-
// Wait a bit to ensure UpdatedAt is bigger when we upsert the telemetry disabled item
408+
// Wait a bit to ensure UpdatedAt is bigger when we upsert the telemetry enabled item
410409
time.Sleep(100 * time.Millisecond)
411410
require.NoError(t, db.UpsertTelemetryItem(ctx, database.UpsertTelemetryItemParams{
412-
Key: string(telemetry.TelemetryItemKeyTelemetryDisabled),
411+
Key: string(telemetry.TelemetryItemKeyTelemetryEnabled),
413412
Value: "",
414413
}))
415414
require.NoError(t, reporter.ReportDisabledIfNeeded())
@@ -422,7 +421,7 @@ func TestReportDisabledIfNeeded(t *testing.T) {
422421
}
423422
}
424423

425-
func setupTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, chan *telemetry.Snapshot) {
424+
func mockTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, chan *telemetry.Snapshot) {
426425
t.Helper()
427426
deployment := make(chan *telemetry.Deployment, 64)
428427
snapshot := make(chan *telemetry.Snapshot, 64)
@@ -456,7 +455,7 @@ func setupTelemetryServer(t *testing.T) (*url.URL, chan *telemetry.Deployment, c
456455
func collectSnapshot(t *testing.T, db database.Store, addOptionsFn func(opts telemetry.Options) telemetry.Options) (*telemetry.Deployment, *telemetry.Snapshot) {
457456
t.Helper()
458457

459-
serverURL, deployment, snapshot := setupTelemetryServer(t)
458+
serverURL, deployment, snapshot := mockTelemetryServer(t)
460459

461460
options := telemetry.Options{
462461
Database: db,

0 commit comments

Comments
 (0)