- Notifications
You must be signed in to change notification settings - Fork928
chore: track disabled telemetry#16347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes from9 commits
Commits
Show all changes
22 commits Select commitHold shift + click to select a range
b44071a ReportDisabledIfNeeded
hugodutkabb08f9d add a test for TestReportDisabledIfNeeded
hugodutkadb99f01 remove unused values
hugodutka7293db5 fix data race
hugodutka2ceaf0d disable report on close when reporting telemetry disabled
hugodutka9cccf4d fmt
hugodutkace2b563 simplify the telemetry disabled reporting logic
hugodutka7085aa0 more accurate comments
hugodutka16ba740 clearer comment
hugodutka3b62bf9 fix test
hugodutka43b7e6b clarify comment
hugodutkaaa56a6e nit
hugodutkaa29b7f2 add another comment
hugodutkaf3cb1b8 shorten comment
hugodutka75d231f fix log message
hugodutkaa1c12cf add an extra integration test
hugodutkaa0fce8d use only a single telemetry item to track telemetry enabled status
hugodutka9a86ec9 rename RecordTelemetryStatusChange to RecordTelemetryStatus
hugodutkae032cce update comment
hugodutkaa7fe670 lint
hugodutkab4768fa fix test
hugodutkaa2846d5 replace "1" and "0" with "true" and "false"
hugodutkaFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
21 changes: 21 additions & 0 deletionscli/server.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -814,11 +814,32 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return xerrors.Errorf("create telemetry reporter: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| go options.Telemetry.RunSnapshotter() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer options.Telemetry.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.Warn(ctx, fmt.Sprintf(`telemetry disabled, unable to notify of security issues. Read more: %s/admin/setup/telemetry`, vals.DocsURL.String())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !vals.Telemetry.Enable.Value() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| reporter, err := telemetry.New(telemetry.Options{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DeploymentID: deploymentID, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Database: options.Database, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Logger: logger.Named("telemetry"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| URL: vals.Telemetry.URL.Value(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DisableReportOnClose: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.Debug(ctx, "create telemetry reporter (disabled)", slog.Error(err)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| go func() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer reporter.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := reporter.ReportDisabledIfNeeded(); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.Debug(ctx, "failed to report disabled telemetry", slog.Error(err)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Should this not just be in the else block? Suggested change
Is there a way we can just configure Just a thought. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This prevents the pprof import from being accidentally deleted. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _ = pprof.Handler | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if vals.Pprof.Enable { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
101 changes: 88 additions & 13 deletionscoderd/telemetry/telemetry.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -47,10 +47,11 @@ type Options struct { | ||
| // URL is an endpoint to direct telemetry towards! | ||
| URL *url.URL | ||
| DeploymentID string | ||
| DeploymentConfig *codersdk.DeploymentValues | ||
| BuiltinPostgres bool | ||
| Tunnel bool | ||
| DisableReportOnClose bool | ||
| SnapshotFrequency time.Duration | ||
| ParseLicenseJWT func(lic *License) error | ||
| @@ -59,6 +60,9 @@ type Options struct { | ||
| // New constructs a reporter for telemetry data. | ||
| // Duplicate data will be sent, it's on the server-side to index by UUID. | ||
| // Data is anonymized prior to being sent! | ||
| // | ||
| // The returned Reporter should be started with RunSnapshotter() to begin | ||
| // reporting. | ||
| func New(options Options) (Reporter, error) { | ||
| if options.SnapshotFrequency == 0 { | ||
| // Report once every 30mins by default! | ||
| @@ -83,7 +87,6 @@ func New(options Options) (Reporter, error) { | ||
| snapshotURL: snapshotURL, | ||
| startedAt: dbtime.Now(), | ||
| } | ||
| return reporter, nil | ||
| } | ||
| @@ -101,6 +104,14 @@ type Reporter interface { | ||
| Report(snapshot *Snapshot) | ||
| Enabled() bool | ||
| Close() | ||
| // RunSnapshotter runs reporting in a loop. It should be called in a | ||
| // goroutine to avoid blocking the caller. | ||
| RunSnapshotter() | ||
| // ReportDisabledIfNeeded reports that the telemetry was disabled in the following scenarios: | ||
| // 1. The telemetry was enabled at some point, and we haven't reported the disabled telemetry yet. | ||
| // 2. The telemetry was enabled at some point, we reported the disabled telemetry, the telemetry | ||
| // was enabled again, then disabled again, and we haven't reported it yet. | ||
| ReportDisabledIfNeeded() error | ||
| } | ||
| type remoteReporter struct { | ||
| @@ -161,10 +172,12 @@ func (r *remoteReporter) Close() { | ||
| close(r.closed) | ||
| now := dbtime.Now() | ||
| r.shutdownAt = &now | ||
| if !r.options.DisableReportOnClose { | ||
| // Report a final collection of telemetry prior to close! | ||
| // This could indicate final actions a user has taken, and | ||
| // the time the deployment was shutdown. | ||
| r.reportWithDeployment() | ||
| } | ||
| r.closeFunc() | ||
| } | ||
| @@ -177,7 +190,18 @@ func (r *remoteReporter) isClosed() bool { | ||
| } | ||
| } | ||
| func (r *remoteReporter) recordTelemetryEnabled() { | ||
| if err := r.options.Database.UpsertTelemetryItem(r.ctx, database.UpsertTelemetryItemParams{ | ||
| Key: string(TelemetryItemKeyTelemetryEnabled), | ||
| Value: "", | ||
| }); err != nil { | ||
| r.options.Logger.Debug(r.ctx, "upsert last telemetry report", slog.Error(err)) | ||
| } | ||
| } | ||
| func (r *remoteReporter) RunSnapshotter() { | ||
| r.recordTelemetryEnabled() | ||
| first := true | ||
| ticker := time.NewTicker(r.options.SnapshotFrequency) | ||
| defer ticker.Stop() | ||
| @@ -330,6 +354,53 @@ func checkIDPOrgSync(ctx context.Context, db database.Store, values *codersdk.De | ||
| return syncConfig.Field != "", nil | ||
| } | ||
| func (r *remoteReporter) ReportDisabledIfNeeded() error { | ||
| db := r.options.Database | ||
| telemetryEnabled, telemetryEnabledErr := db.GetTelemetryItem(r.ctx, string(TelemetryItemKeyTelemetryEnabled)) | ||
| if telemetryEnabledErr != nil && !errors.Is(telemetryEnabledErr, sql.ErrNoRows) { | ||
| r.options.Logger.Debug(r.ctx, "get telemetry enabled", slog.Error(telemetryEnabledErr)) | ||
| } | ||
| telemetryDisabled, telemetryDisabledErr := db.GetTelemetryItem(r.ctx, string(TelemetryItemKeyTelemetryDisabled)) | ||
| if telemetryDisabledErr != nil && !errors.Is(telemetryDisabledErr, sql.ErrNoRows) { | ||
| r.options.Logger.Debug(r.ctx, "get telemetry disabled", slog.Error(telemetryDisabledErr)) | ||
| } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Is there a benefit to making this 2 rows? If it was just 1 row, with the last state being recorded, would that be sufficient? As in, just store Then you'd have to merge If there is some reason the logic that saves the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. You're right, I think it would work, and it would simplify the logic too. We'd only need to report telemetry disabled when the telemetryEnabled in db switches from true to false. I'll refactor this. | ||
| // There are 2 scenarios in which we want to report the disabled telemetry: | ||
| // 1. The telemetry was enabled at some point, and we haven't reported the disabled telemetry yet. | ||
| // 2. The telemetry was enabled at some point, we reported the disabled telemetry, the telemetry | ||
| // was enabled again, then disabled again, and we haven't reported it yet. | ||
| // | ||
| // - In both cases, the TelemetryEnabled item will be present. | ||
| // - In case 1. the TelemetryDisabled item will not be present. | ||
| // - In case 2. the TelemetryDisabled item will be present, and the TelemetryEnabled item will | ||
| // be more recent than the TelemetryDisabled item. | ||
| shouldReportDisabledTelemetry := telemetryEnabledErr == nil && | ||
| (errors.Is(telemetryDisabledErr, sql.ErrNoRows) || | ||
| (telemetryDisabledErr == nil && telemetryEnabled.UpdatedAt.After(telemetryDisabled.UpdatedAt))) | ||
| if !shouldReportDisabledTelemetry { | ||
| return nil | ||
| } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Can we extract this to it's own function, maybe throw some quick unit tests around just this logic? I know you have a test that tests the full flow, but this PR comes down to this conditional which is a bit hard to read. We can define the full space of input values and test it easily if we extract it to a function. If I am reading the comments correctly, this is the truth table: If my understanding is correct, then this code is correct as well. Just a suggestion if we extract this to a function, we can implement the logic in easier to read conditionals. funcshouldReportDisabledTelemetry(/* ... */) {iferrors.Is(telemetryEnabled,sql.ErrNoRows) {returnfalse// Telemetry was never enabled, so do not report }iferrors.Is(telemetryDisabledErr,sql.ErrNoRows) {returntrue// Never reported teletry was disabled, so send the report }returntelemetryEnabled.UpdatedAt.After(telemetryDisabled.UpdatedAt)} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I think I'd prefer it being 1 row as mentioned above it possible | ||
| if err := db.UpsertTelemetryItem(r.ctx, database.UpsertTelemetryItemParams{ | ||
| Key: string(TelemetryItemKeyTelemetryDisabled), | ||
| Value: "", | ||
| }); err != nil { | ||
| return xerrors.Errorf("upsert telemetry disabled: %w", err) | ||
| } | ||
| item, err := db.GetTelemetryItem(r.ctx, string(TelemetryItemKeyTelemetryDisabled)) | ||
| if err != nil { | ||
| return xerrors.Errorf("get telemetry disabled: %w", err) | ||
| } | ||
| r.reportSync( | ||
| &Snapshot{ | ||
| TelemetryItems: []TelemetryItem{ | ||
| ConvertTelemetryItem(item), | ||
| }, | ||
| }, | ||
| ) | ||
| return nil | ||
| } | ||
| // createSnapshot collects a full snapshot from the database. | ||
| func (r *remoteReporter) createSnapshot() (*Snapshot, error) { | ||
| var ( | ||
| @@ -1567,6 +1638,8 @@ type telemetryItemKey string | ||
| //revive:disable:exported | ||
| const ( | ||
| TelemetryItemKeyHTMLFirstServedAt telemetryItemKey = "html_first_served_at" | ||
| TelemetryItemKeyTelemetryEnabled telemetryItemKey = "telemetry_enabled" | ||
| TelemetryItemKeyTelemetryDisabled telemetryItemKey = "telemetry_disabled" | ||
| ) | ||
| type TelemetryItem struct { | ||
| @@ -1578,6 +1651,8 @@ type TelemetryItem struct { | ||
| type noopReporter struct{} | ||
| func (*noopReporter) Report(_ *Snapshot) {} | ||
| func (*noopReporter) Enabled() bool { return false } | ||
| func (*noopReporter) Close() {} | ||
| func (*noopReporter) RunSnapshotter() {} | ||
| func (*noopReporter) ReportDisabledIfNeeded() error { return nil } | ||
78 changes: 75 additions & 3 deletionscoderd/telemetry/telemetry_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.