- Notifications
You must be signed in to change notification settings - Fork913
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
db9b135 to325314bCompare8ed47fb to7085aa0CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This concept LGTM, assuming we send one final ping that simply describes that telemetry has been disabled for the deployment with no other data in the payload. We can always remove PII via secure erase requests and this does not send any additional PII, or any information for that matter besides telemetry being disabled.
This will improve the quality of our analytics, helping us differentiate deployments that simply get shut down versus telemetry being disabled when considering metrics such as retention.
cli/server.go Outdated
| } 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 comment
The reason will be displayed to describe this comment to others.Learn more.
Should this not just be in the else block?
| }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, | |
| }) | |
| iferr!=nil { | |
| logger.Debug(ctx,"create telemetry reporter (disabled)",slog.Error(err)) | |
| }else { | |
| gofunc() { | |
| deferreporter.Close() | |
| iferr:=reporter.ReportDisabledIfNeeded();err!=nil { | |
| logger.Debug(ctx,"failed to report disabled telemetry",slog.Error(err)) | |
| } | |
| }() | |
| } | |
| } | |
| }else { | |
| logger.Warn(ctx,fmt.Sprintf(`telemetry disabled, unable to notify of security issues. Read more: %s/admin/setup/telemetry`,vals.DocsURL.String())) | |
| reporter,err:= telemetry.New(telemetry.Options{ | |
| DeploymentID:deploymentID, | |
| Database:options.Database, | |
| Logger:logger.Named("telemetry"), | |
| URL:vals.Telemetry.URL.Value(), | |
| DisableReportOnClose:true, | |
| }) | |
| iferr!=nil { | |
| logger.Debug(ctx,"create telemetry reporter (disabled)",slog.Error(err)) | |
| }else { | |
| gofunc() { | |
| deferreporter.Close() | |
| iferr:=reporter.ReportDisabledIfNeeded();err!=nil { | |
| logger.Debug(ctx,"failed to report disabled telemetry",slog.Error(err)) | |
| } | |
| }() | |
| } | |
| } | |
Is there a way we can just configuretelemetry.New and pass in theenabled flag into the telemetry struct? I ask because this init code is now ~60 lines long, wondering we can push theelse condition just into theRunSnapshotter.
Just a thought.
coderd/telemetry/telemetry.go Outdated
| 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 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 comment
The 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:
| Description | telemetryEnabled | telemetryDisabled | Report Telemetry Disabled ||----------------------------|-------------------|-------------------|---------------------------|| New deployment | <null> | <null> | No || Enabled | exists @ time = 0 | <null> | Yes || Disabled,but never enabled | <null> | exists @ time = 0 | No || Enabled after disabled | exists @ time = 1 | exists @ time = 0 | No || Disabled after enabled | exists @ time = 0 | exists @ time = 1 | Yes |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 comment
The 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
coderd/telemetry/telemetry.go Outdated
| 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 comment
The 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 storetelemetryEnabled = true/false
Then you'd have to mergerecordTelemetryEnabled and this function that sends teh final packet.
| Description | telemetryEnabled (db) | telemetryEnabled (is) | Report Telemetry Disabled ||----------------------------------------|-----------------------|-----------------------|---------------------------|| New deployment | <null> | true | No || New deployment, but disabled | <null> | false | No || Telemetry enabled, and still is | true | true | No || Telemetry enabled, but disabled | true | false | Yes || Telemetry was disabled, now is enabled | false | true | Yes || Disabled, still disabled | false | false | No |If there is some reason the logic that saves theenabled prop and the logic that sends the packet cannot be combined, then this suggestion would not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
coderd/telemetry/telemetry.go Outdated
| newValue := "0" | ||
| if telemetryEnabled { | ||
| newValue = "1" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Feels odd to use numbers, but I'm guessing the table column must be a string.strconv has aParseBool func. Your call though since the value is only used here.
a68d115 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Addresseshttps://github.com/coder/nexus/issues/116.
Core Concept
Send one final telemetry report after the user disables telemetry with the message that the telemetry was disabled. No other information about the deployment is sent in this report.
This final report is submitted only if the deployment ever had telemetry on.
Changes
TelemetryEnabledtelemetry item, which allows to decide whether a final report should be sent.RecordTelemetryStatustelemetry method, which decides whether a final report should be sent and updates the telemetry item.