diff --git a/go.mod b/go.mod index 1b38d92e5f..cbe9f9c9c5 100644 --- a/go.mod +++ b/go.mod @@ -61,7 +61,6 @@ require ( github.com/facebookgo/clock v0.0.0-20150410010913-600d898af40a github.com/fraugster/parquet-go v0.10.0 github.com/fsnotify/fsnotify v1.5.1 - github.com/getsentry/sentry-go v0.12.0 github.com/ghemawat/stream v0.0.0-20171120220530-696b145b53b9 github.com/go-sql-driver/mysql v1.6.0 github.com/go-swagger/go-swagger v0.26.1 @@ -199,6 +198,7 @@ require ( github.com/dimchansky/utfbom v1.1.1 // indirect github.com/docker/go-units v0.4.0 // indirect github.com/felixge/httpsnoop v1.0.1 // indirect + github.com/getsentry/sentry-go v0.12.0 // indirect github.com/ghodss/yaml v1.0.0 // indirect github.com/go-kit/log v0.1.0 // indirect github.com/go-logfmt/logfmt v0.5.1 // indirect diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go index 040b61cc80..a2d9adc9a1 100644 --- a/pkg/cli/cli.go +++ b/pkg/cli/cli.go @@ -125,11 +125,6 @@ func doMain(cmd *cobra.Command, cmdName string) error { } } - logcrash.SetupCrashReporter( - context.Background(), - cmdName, - ) - defer logcrash.RecoverAndReportPanic(context.Background(), &serverCfg.Settings.SV) return Run(os.Args[1:]) diff --git a/pkg/cli/context.go b/pkg/cli/context.go index dccc05d952..a9d18f0f2b 100644 --- a/pkg/cli/context.go +++ b/pkg/cli/context.go @@ -618,7 +618,6 @@ func setDemoContextDefaults() { demoCtx.RunWorkload = false demoCtx.Localities = nil demoCtx.GeoPartitionedReplicas = false - demoCtx.DisableTelemetry = false demoCtx.DefaultKeySize = defaultKeySize demoCtx.DefaultCALifetime = defaultCALifetime demoCtx.DefaultCertLifetime = defaultCertLifetime diff --git a/pkg/cli/demo.go b/pkg/cli/demo.go index 8f94592fbe..667ed38d31 100644 --- a/pkg/cli/demo.go +++ b/pkg/cli/demo.go @@ -20,7 +20,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/cli/cliflags" "github.com/cockroachdb/cockroach/pkg/cli/democluster" "github.com/cockroachdb/cockroach/pkg/server/serverpb" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/stop" @@ -40,10 +39,6 @@ subcommands: e.g. "cockroach demo startrek". See --help for a full list. By default, the 'movr' dataset is pre-loaded. You can also use --no-example-database to avoid pre-loading a dataset. - -cockroach demo attempts to connect to a Cockroach Labs server to send -telemetry back to Cockroach Labs. In order to disable this behavior, set the -environment variable "COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING" to true. `, Example: ` cockroach demo`, Args: cobra.NoArgs, @@ -141,8 +136,6 @@ func checkDemoConfiguration( return nil, errors.Newf("--%s cannot be used with --%s", cliflags.Global.Name, cliflags.DemoNodeLocality.Name) } - demoCtx.DisableTelemetry = cluster.TelemetryOptOut() - // Whether or not we enable enterprise feature is a combination of: // // - whether the user wants them (they can disable enterprise @@ -280,17 +273,6 @@ func runDemo(cmd *cobra.Command, gen workload.Generator) (resErr error) { ) } - // Only print details about the telemetry configuration if the - // user has control over it. - if demoCtx.DisableTelemetry { - cliCtx.PrintlnUnlessEmbedded("#\n# Telemetry disabled by configuration.") - } else { - cliCtx.PrintlnUnlessEmbedded("#\n" + - "# This demo session will send telemetry to Cockroach Labs in the background.\n" + - "# To disable this behavior, set the environment variable\n" + - "# COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING=true.") - } - if demoCtx.disableEnterpriseFeatures { cliCtx.PrintlnUnlessEmbedded("#\n# Enterprise features are disabled by configuration.\n") } diff --git a/pkg/cli/democluster/context.go b/pkg/cli/democluster/context.go index 3314c9cf89..de8eb5dfde 100644 --- a/pkg/cli/democluster/context.go +++ b/pkg/cli/democluster/context.go @@ -35,9 +35,6 @@ type Context struct { // CacheSize is the size of the storage cache for each KV server. CacheSize int64 - // DisableTelemetry requests that telemetry be disabled. - DisableTelemetry bool - // NoExampleDatabase prevents the auto-creation of a demo database // from a workload. NoExampleDatabase bool diff --git a/pkg/cli/democluster/demo_cluster.go b/pkg/cli/democluster/demo_cluster.go index f2dea23237..727b559327 100644 --- a/pkg/cli/democluster/demo_cluster.go +++ b/pkg/cli/democluster/demo_cluster.go @@ -489,14 +489,6 @@ func (c *transientCluster) Start( } } - // Start up the update check loop. - // We don't do this in (*server.Server).Start() because we don't want this - // overhead and possible interference in tests. - if !c.demoCtx.DisableTelemetry { - c.infoLog(ctx, "starting telemetry") - c.firstServer.StartDiagnostics(ctx) - } - return nil }(phaseCtx); err != nil { return err diff --git a/pkg/cli/mt_start_sql.go b/pkg/cli/mt_start_sql.go index 7160c4f4f7..dfa141152e 100644 --- a/pkg/cli/mt_start_sql.go +++ b/pkg/cli/mt_start_sql.go @@ -20,7 +20,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/cli/clierrorplus" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/server" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/sdnotify" "github.com/cockroachdb/cockroach/pkg/util/timeutil" @@ -152,13 +151,6 @@ func runStartSQL(cmd *cobra.Command, args []string) error { log.Ops.Errorf(ctx, "failed to signal readiness using systemd protocol: %s", err) } - // Start up the diagnostics reporting loop. - // We don't do this in (*server.SQLServer).preStart() because we don't - // want this overhead and possible interference in tests. - if !cluster.TelemetryOptOut() { - sqlServer.StartDiagnostics(ctx) - } - tenantClusterID := sqlServer.LogicalClusterID() // Report the server identifiers and other server details diff --git a/pkg/cli/start.go b/pkg/cli/start.go index 4b51bcb149..b87b89e27e 100644 --- a/pkg/cli/start.go +++ b/pkg/cli/start.go @@ -647,12 +647,6 @@ If problems persist, please see %s.` serverStatusMu.started = true serverStatusMu.Unlock() - // Start up the diagnostics reporting and update check loops. - // We don't do this in (*server.Server).Start() because we don't - // want this overhead and possible interference in tests. - if !cluster.TelemetryOptOut() { - s.StartDiagnostics(ctx) - } initialStart := s.InitialStart() // Run SQL for new clusters. diff --git a/pkg/server/admin.go b/pkg/server/admin.go index d0eb09ec7c..5ef35c189f 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -57,7 +57,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/iterutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" "github.com/cockroachdb/cockroach/pkg/util/mon" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/cockroach/pkg/util/quotapool" @@ -1885,7 +1884,7 @@ func (s *adminServer) Cluster( return &serverpb.ClusterResponse{ // TODO(knz): Respond with the logical cluster ID as well. ClusterID: storageClusterID.String(), - ReportingEnabled: logcrash.DiagnosticsReportingEnabled.Get(&s.server.st.SV), + ReportingEnabled: false, EnterpriseEnabled: enterpriseEnabled, }, nil } diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go index a90a45910e..218469245b 100644 --- a/pkg/server/admin_test.go +++ b/pkg/server/admin_test.go @@ -1027,10 +1027,10 @@ func TestAdminAPIEvents(t *testing.T) { {"create_database", false, 0, false, 3}, {"drop_table", false, 0, false, 2}, {"create_table", false, 0, false, 3}, - {"set_cluster_setting", false, 0, false, 4}, + {"set_cluster_setting", false, 0, false, 3}, // We use limit=true with no limit here because otherwise the // expCount will mess up the expected total count below. - {"set_cluster_setting", true, 0, true, 4}, + {"set_cluster_setting", true, 0, true, 3}, {"create_table", true, 0, false, 3}, {"create_table", true, -1, false, 3}, {"create_table", true, 2, false, 2}, @@ -1389,46 +1389,40 @@ func TestAdminAPIUISeparateData(t *testing.T) { func TestClusterAPI(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - s, db, _ := serverutils.StartServer(t, base.TestServerArgs{}) + s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) defer s.Stopper().Stop(context.Background()) - testutils.RunTrueAndFalse(t, "reportingOn", func(t *testing.T, reportingOn bool) { - testutils.RunTrueAndFalse(t, "enterpriseOn", func(t *testing.T, enterpriseOn bool) { - // Override server license check. - if enterpriseOn { - old := base.CheckEnterpriseEnabled - base.CheckEnterpriseEnabled = func(_ *cluster.Settings, _ uuid.UUID, _, _ string) error { - return nil - } - defer func() { base.CheckEnterpriseEnabled = old }() + testutils.RunTrueAndFalse(t, "enterpriseOn", func(t *testing.T, enterpriseOn bool) { + // Override server license check. + if enterpriseOn { + old := base.CheckEnterpriseEnabled + base.CheckEnterpriseEnabled = func(_ *cluster.Settings, _ uuid.UUID, _, _ string) error { + return nil } + defer func() { base.CheckEnterpriseEnabled = old }() + } - if _, err := db.Exec(`SET CLUSTER SETTING diagnostics.reporting.enabled = $1`, reportingOn); err != nil { - t.Fatal(err) + // We need to retry, because the cluster ID isn't set until after + // bootstrapping and because setting a cluster setting isn't necessarily + // instantaneous. + // + // Also note that there's a migration that affects `diagnostics.reporting.enabled`, + // so manipulating the cluster setting var directly is a bad idea. + testutils.SucceedsSoon(t, func() error { + var resp serverpb.ClusterResponse + if err := getAdminJSONProto(s, "cluster", &resp); err != nil { + return err } - - // We need to retry, because the cluster ID isn't set until after - // bootstrapping and because setting a cluster setting isn't necessarily - // instantaneous. - // - // Also note that there's a migration that affects `diagnostics.reporting.enabled`, - // so manipulating the cluster setting var directly is a bad idea. - testutils.SucceedsSoon(t, func() error { - var resp serverpb.ClusterResponse - if err := getAdminJSONProto(s, "cluster", &resp); err != nil { - return err - } - if a, e := resp.ClusterID, s.RPCContext().StorageClusterID.String(); a != e { - return errors.Errorf("cluster ID %s != expected %s", a, e) - } - if a, e := resp.ReportingEnabled, reportingOn; a != e { - return errors.Errorf("reportingEnabled = %t, wanted %t", a, e) - } - if a, e := resp.EnterpriseEnabled, enterpriseOn; a != e { - return errors.Errorf("enterpriseEnabled = %t, wanted %t", a, e) - } - return nil - }) + if a, e := resp.ClusterID, s.RPCContext().StorageClusterID.String(); a != e { + return errors.Errorf("cluster ID %s != expected %s", a, e) + } + if resp.ReportingEnabled { + return errors.Errorf("reportingEnabled = %t, wanted %t", resp.ReportingEnabled, false) + } + if a, e := resp.EnterpriseEnabled, enterpriseOn; a != e { + return errors.Errorf("enterpriseEnabled = %t, wanted %t", a, e) + } + return nil }) }) } diff --git a/pkg/server/diagnostics/diagnostics.go b/pkg/server/diagnostics/diagnostics.go index db839d9d58..fbd83b1240 100644 --- a/pkg/server/diagnostics/diagnostics.go +++ b/pkg/server/diagnostics/diagnostics.go @@ -12,117 +12,17 @@ package diagnostics import ( "context" - "math/rand" - "net/url" - "strconv" - "time" - "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server/diagnostics/diagnosticspb" "github.com/cockroachdb/cockroach/pkg/util/cloudinfo" - "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/system" - "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/shirou/gopsutil/v3/cpu" "github.com/shirou/gopsutil/v3/host" "github.com/shirou/gopsutil/v3/load" "github.com/shirou/gopsutil/v3/mem" ) -// updatesURL is the URL used to check for new versions. Can be nil if an empty -// URL is set. -var updatesURL *url.URL - -const defaultUpdatesURL = `https://register.cockroachdb.com/api/clusters/updates` - -// reportingURL is the URL used to report diagnostics/telemetry. Can be nil if -// an empty URL is set. -var reportingURL *url.URL - -const defaultReportingURL = `https://register.cockroachdb.com/api/clusters/report` - -func init() { - var err error - updatesURL, err = url.Parse( - envutil.EnvOrDefaultString("COCKROACH_UPDATE_CHECK_URL", defaultUpdatesURL), - ) - if err != nil { - panic(err) - } - reportingURL, err = url.Parse( - envutil.EnvOrDefaultString("COCKROACH_USAGE_REPORT_URL", defaultReportingURL), - ) - if err != nil { - panic(err) - } -} - -// TestingKnobs groups testing knobs for diagnostics. -type TestingKnobs struct { - // OverrideUpdatesURL if set, overrides the URL used to check for new - // versions. It is a pointer to pointer to allow overriding to the nil URL. - OverrideUpdatesURL **url.URL - - // OverrideReportingURL if set, overrides the URL used to report diagnostics. - // It is a pointer to pointer to allow overriding to the nil URL. - OverrideReportingURL **url.URL -} - -// ClusterInfo contains cluster information that will become part of URLs. -type ClusterInfo struct { - StorageClusterID uuid.UUID - LogicalClusterID uuid.UUID - TenantID roachpb.TenantID - IsInsecure bool - IsInternal bool -} - -// addInfoToURL sets query parameters on the URL used to report diagnostics. If -// this is a CRDB node, then nodeInfo is filled (and nodeInfo.NodeID is -// non-zero). Otherwise, this is a SQL-only tenant and sqlInfo is filled. -func addInfoToURL( - url *url.URL, - clusterInfo *ClusterInfo, - env *diagnosticspb.Environment, - nodeID roachpb.NodeID, - sqlInfo *diagnosticspb.SQLInstanceInfo, -) *url.URL { - if url == nil { - return nil - } - result := *url - q := result.Query() - - // Don't set nodeid if this is a SQL-only instance. - if nodeID != 0 { - q.Set("nodeid", strconv.Itoa(int(nodeID))) - } - - b := env.Build - q.Set("sqlid", strconv.Itoa(int(sqlInfo.SQLInstanceID))) - q.Set("uptime", strconv.Itoa(int(sqlInfo.Uptime))) - q.Set("licensetype", env.LicenseType) - q.Set("version", b.Tag) - q.Set("platform", b.Platform) - q.Set("uuid", clusterInfo.StorageClusterID.String()) - q.Set("logical_uuid", clusterInfo.LogicalClusterID.String()) - q.Set("tenantid", clusterInfo.TenantID.String()) - q.Set("insecure", strconv.FormatBool(clusterInfo.IsInsecure)) - q.Set("internal", strconv.FormatBool(clusterInfo.IsInternal)) - q.Set("buildchannel", b.Channel) - q.Set("envchannel", b.EnvChannel) - result.RawQuery = q.Encode() - return &result -} - -// randomly shift `d` to be up to `jitterSeconds` shorter or longer. -func addJitter(d time.Duration) time.Duration { - const jitterSeconds = 120 - j := time.Duration(rand.Intn(jitterSeconds*2)-jitterSeconds) * time.Second - return d + j -} - var populateMutex syncutil.Mutex // populateHardwareInfo populates OS, CPU, memory, etc. information about the diff --git a/pkg/server/diagnostics/reporter.go b/pkg/server/diagnostics/reporter.go index 59c6bdbe2a..06ee97774f 100644 --- a/pkg/server/diagnostics/reporter.go +++ b/pkg/server/diagnostics/reporter.go @@ -11,11 +11,7 @@ package diagnostics import ( - "bytes" "context" - "io/ioutil" - "net/http" - "net/url" "reflect" "time" @@ -35,13 +31,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" - "github.com/cockroachdb/cockroach/pkg/util/httputil" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" "github.com/cockroachdb/cockroach/pkg/util/protoutil" - "github.com/cockroachdb/cockroach/pkg/util/stop" "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" "github.com/mitchellh/reflectwalk" "google.golang.org/protobuf/proto" @@ -56,29 +48,12 @@ type NodeStatusGenerator interface { GenerateNodeStatus(ctx context.Context) *statuspb.NodeStatus } -var reportFrequency = settings.RegisterDurationSetting( - settings.TenantWritable, - "diagnostics.reporting.interval", - "interval at which diagnostics data should be reported", - time.Hour, - settings.NonNegativeDuration, -).WithPublic() - -// Reporter is a helper struct that phones home to report usage and diagnostics. +// Reporter is a helper struct for generating usage and diagnostics reports. type Reporter struct { - StartTime time.Time - AmbientCtx *log.AmbientContext - Config *base.Config - Settings *cluster.Settings + StartTime time.Time + Settings *cluster.Settings - // StorageClusterID is the cluster ID of the underlying storage - // cluster. It is not yet available at the time the reporter is - // created, so instead initialize with a function that gets it - // dynamically. - StorageClusterID func() uuid.UUID - TenantID roachpb.TenantID - // LogicalClusterID is the tenant-specific logical cluster ID. - LogicalClusterID func() uuid.UUID + TenantID roachpb.TenantID // SQLInstanceID is not yet available at the time the reporter is created, // so instead initialize with a function that gets it dynamically. @@ -90,81 +65,6 @@ type Reporter struct { // Locality is a description of the topography of the server. Locality roachpb.Locality - - // TestingKnobs is used for internal test controls only. - TestingKnobs *TestingKnobs -} - -// PeriodicallyReportDiagnostics starts a background worker that periodically -// phones home to report usage and diagnostics. -func (r *Reporter) PeriodicallyReportDiagnostics(ctx context.Context, stopper *stop.Stopper) { - _ = stopper.RunAsyncTaskEx(ctx, stop.TaskOpts{TaskName: "diagnostics", SpanOpt: stop.SterileRootSpan}, func(ctx context.Context) { - defer logcrash.RecoverAndReportNonfatalPanic(ctx, &r.Settings.SV) - nextReport := r.StartTime - - var timer timeutil.Timer - defer timer.Stop() - for { - // TODO(dt): we should allow tuning the reset and report intervals separately. - // Consider something like rand.Float() > resetFreq/reportFreq here to sample - // stat reset periods for reporting. - if logcrash.DiagnosticsReportingEnabled.Get(&r.Settings.SV) { - r.ReportDiagnostics(ctx) - } - - nextReport = nextReport.Add(reportFrequency.Get(&r.Settings.SV)) - - timer.Reset(addJitter(nextReport.Sub(timeutil.Now()))) - select { - case <-stopper.ShouldQuiesce(): - return - case <-timer.C: - timer.Read = true - } - } - }) -} - -// ReportDiagnostics phones home to report usage and diagnostics. -// -// NOTE: This can be slow because of cloud detection; use cloudinfo.Disable() in -// tests to avoid that. -func (r *Reporter) ReportDiagnostics(ctx context.Context) { - ctx, span := r.AmbientCtx.AnnotateCtxWithSpan(ctx, "usageReport") - defer span.Finish() - - report := r.CreateReport(ctx, telemetry.ResetCounts) - - url := r.buildReportingURL(report) - if url == nil { - return - } - - b, err := protoutil.Marshal(report) - if err != nil { - log.Warningf(ctx, "%v", err) - return - } - - res, err := httputil.Post( - ctx, url.String(), "application/x-protobuf", bytes.NewReader(b), - ) - if err != nil { - if log.V(2) { - // This is probably going to be relatively common in production - // environments where network access is usually curtailed. - log.Warningf(ctx, "failed to report node usage metrics: %v", err) - } - return - } - defer res.Body.Close() - b, err = ioutil.ReadAll(res.Body) - if err != nil || res.StatusCode != http.StatusOK { - log.Warningf(ctx, "failed to report node usage metrics: status: %s, body: %s, "+ - "error: %v", res.Status, b, err) - return - } - r.SQLServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx) } // CreateReport generates a new diagnostics report containing information about @@ -343,24 +243,6 @@ func (r *Reporter) collectSchemaInfo(ctx context.Context) ([]descpb.TableDescrip return tables, nil } -// buildReportingURL creates a URL to report diagnostics. -// If an empty updates URL is set (via empty environment variable), returns nil. -func (r *Reporter) buildReportingURL(report *diagnosticspb.DiagnosticReport) *url.URL { - clusterInfo := ClusterInfo{ - StorageClusterID: r.StorageClusterID(), - LogicalClusterID: r.LogicalClusterID(), - TenantID: r.TenantID, - IsInsecure: r.Config.Insecure, - IsInternal: sql.ClusterIsInternal(&r.Settings.SV), - } - - url := reportingURL - if r.TestingKnobs != nil && r.TestingKnobs.OverrideReportingURL != nil { - url = *r.TestingKnobs.OverrideReportingURL - } - return addInfoToURL(url, &clusterInfo, &report.Env, report.Node.NodeID, &report.SQL) -} - func getLicenseType(ctx context.Context, settings *cluster.Settings) string { licenseType, err := base.LicenseType(settings) if err != nil { diff --git a/pkg/server/diagnostics/update_checker.go b/pkg/server/diagnostics/update_checker.go deleted file mode 100644 index 1f58a6e0da..0000000000 --- a/pkg/server/diagnostics/update_checker.go +++ /dev/null @@ -1,213 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package diagnostics - -import ( - "context" - "encoding/json" - "io" - "io/ioutil" - "net/http" - "net/url" - "time" - - "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/build" - "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/server/diagnostics/diagnosticspb" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" - "github.com/cockroachdb/cockroach/pkg/sql" - "github.com/cockroachdb/cockroach/pkg/util/httputil" - "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" - "github.com/cockroachdb/cockroach/pkg/util/stop" - "github.com/cockroachdb/cockroach/pkg/util/timeutil" - "github.com/cockroachdb/cockroach/pkg/util/uuid" -) - -const ( - updateCheckFrequency = time.Hour * 24 - // TODO(dt): switch to settings. - updateCheckPostStartup = time.Minute * 5 - updateCheckRetryFrequency = time.Hour - updateMaxVersionsToReport = 3 -) - -type versionInfo struct { - Version string `json:"version"` - Details string `json:"details"` -} - -// UpdateChecker is a helper struct that phones home to check for updates. -type UpdateChecker struct { - StartTime time.Time - AmbientCtx *log.AmbientContext - Config *base.Config - Settings *cluster.Settings - - // StorageClusterID is the cluster ID of the underlying storage - // cluster. It is not yet available at the time the updater is - // created, so instead initialize with a function to get it. - StorageClusterID func() uuid.UUID - // LogicalClusterID is the tenant-specific logical cluster ID. - LogicalClusterID func() uuid.UUID - - // NodeID is not yet available at the time the updater is created, so - // instead initialize with a function to get it. - NodeID func() roachpb.NodeID - - // SQLInstanceID is not yet available at the time the reporter is created, - // so instead initialize with a function that gets it dynamically. - SQLInstanceID func() base.SQLInstanceID - - // TestingKnobs is used for internal test controls only. - TestingKnobs *TestingKnobs -} - -// PeriodicallyCheckForUpdates starts a background worker that periodically -// phones home to check for updates. -func (u *UpdateChecker) PeriodicallyCheckForUpdates(ctx context.Context, stopper *stop.Stopper) { - _ = stopper.RunAsyncTaskEx(ctx, stop.TaskOpts{ - TaskName: "update-checker", - SpanOpt: stop.SterileRootSpan, - }, func(ctx context.Context) { - defer logcrash.RecoverAndReportNonfatalPanic(ctx, &u.Settings.SV) - nextUpdateCheck := u.StartTime - - var timer timeutil.Timer - defer timer.Stop() - for { - now := timeutil.Now() - runningTime := now.Sub(u.StartTime) - - nextUpdateCheck = u.maybeCheckForUpdates(ctx, now, nextUpdateCheck, runningTime) - - timer.Reset(addJitter(nextUpdateCheck.Sub(timeutil.Now()))) - select { - case <-stopper.ShouldQuiesce(): - return - case <-timer.C: - timer.Read = true - } - } - }) -} - -// CheckForUpdates calls home to check for new versions for the current platform -// and logs messages if it finds them, as well as if it encounters any errors. -// The returned boolean indicates if the check succeeded (and thus does not need -// to be re-attempted by the scheduler after a retry-interval). -func (u *UpdateChecker) CheckForUpdates(ctx context.Context) bool { - ctx, span := u.AmbientCtx.AnnotateCtxWithSpan(ctx, "version update check") - defer span.Finish() - - url := u.buildUpdatesURL(ctx) - if url == nil { - return true // don't bother with asking for retry -- we'll never succeed. - } - - res, err := httputil.Get(ctx, url.String()) - if err != nil { - // This is probably going to be relatively common in production - // environments where network access is usually curtailed. - return false - } - defer res.Body.Close() - - if res.StatusCode != http.StatusOK { - b, err := ioutil.ReadAll(res.Body) - log.Infof(ctx, "failed to check for updates: status: %s, body: %s, error: %v", - res.Status, b, err) - return false - } - - decoder := json.NewDecoder(res.Body) - r := struct { - Details []versionInfo `json:"details"` - }{} - - err = decoder.Decode(&r) - if err != nil && err != io.EOF { - log.Warningf(ctx, "error decoding updates info: %v", err) - return false - } - - // Ideally the updates server only returns the most relevant updates for us, - // but if it replied with an excessive number of updates, limit log spam by - // only printing the last few. - if len(r.Details) > updateMaxVersionsToReport { - r.Details = r.Details[len(r.Details)-updateMaxVersionsToReport:] - } - for _, v := range r.Details { - log.Infof(ctx, "a new version is available: %s, details: %s", v.Version, v.Details) - } - return true -} - -// maybeCheckForUpdates determines if it is time to check for updates and does -// so if it is, before returning the time at which the next check be done. -func (u *UpdateChecker) maybeCheckForUpdates( - ctx context.Context, now, scheduled time.Time, runningTime time.Duration, -) time.Time { - if scheduled.After(now) { - return scheduled - } - - // If diagnostics reporting is disabled, we should assume that means that the - // user doesn't want us phoning home for new-version checks either. - if !logcrash.DiagnosticsReportingEnabled.Get(&u.Settings.SV) { - return now.Add(updateCheckFrequency) - } - - // checkForUpdates handles its own errors, but it returns a bool indicating if - // it succeeded, so we can schedule a re-attempt if it did not. - if succeeded := u.CheckForUpdates(ctx); !succeeded { - return now.Add(updateCheckRetryFrequency) - } - - // If we've just started up, we want to check again shortly after. - // During startup is when a message is most likely to be actually seen by a - // human operator so we check as early as possible, but this makes it hard to - // differentiate real deployments vs short-lived instances for tests. - if runningTime < updateCheckPostStartup { - return now.Add(time.Hour - runningTime) - } - - return now.Add(updateCheckFrequency) -} - -// BuildUpdatesURL creates a URL to check for version updates. -// If an empty updates URL is set (via empty environment variable), returns nil. -func (u *UpdateChecker) buildUpdatesURL(ctx context.Context) *url.URL { - clusterInfo := ClusterInfo{ - StorageClusterID: u.StorageClusterID(), - LogicalClusterID: u.LogicalClusterID(), - TenantID: roachpb.SystemTenantID, - IsInsecure: u.Config.Insecure, - IsInternal: sql.ClusterIsInternal(&u.Settings.SV), - } - - var env diagnosticspb.Environment - env.Build = build.GetInfo() - env.LicenseType = getLicenseType(ctx, u.Settings) - populateHardwareInfo(ctx, &env) - - sqlInfo := diagnosticspb.SQLInstanceInfo{ - SQLInstanceID: u.SQLInstanceID(), - Uptime: int64(timeutil.Since(u.StartTime).Seconds()), - } - - url := updatesURL - if u.TestingKnobs != nil && u.TestingKnobs.OverrideUpdatesURL != nil { - url = *u.TestingKnobs.OverrideUpdatesURL - } - return addInfoToURL(url, &clusterInfo, &env, u.NodeID(), &sqlInfo) -} diff --git a/pkg/server/diagnostics/update_checker_test.go b/pkg/server/diagnostics/update_checker_test.go deleted file mode 100644 index 7096002e87..0000000000 --- a/pkg/server/diagnostics/update_checker_test.go +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright 2021 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package diagnostics_test - -import ( - "context" - "net/url" - "testing" - - "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/build" - "github.com/cockroachdb/cockroach/pkg/server" - "github.com/cockroachdb/cockroach/pkg/server/diagnostics" - "github.com/cockroachdb/cockroach/pkg/testutils/diagutils" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/util/cloudinfo" - "github.com/cockroachdb/cockroach/pkg/util/leaktest" - "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/stretchr/testify/require" -) - -func TestCheckVersion(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - defer cloudinfo.Disable()() - - ctx := context.Background() - - t.Run("expected-reporting", func(t *testing.T) { - r := diagutils.NewServer() - defer r.Close() - - url := r.URL() - s, _, _ := serverutils.StartServer(t, base.TestServerArgs{ - Knobs: base.TestingKnobs{ - Server: &server.TestingKnobs{ - DiagnosticsTestingKnobs: diagnostics.TestingKnobs{ - OverrideUpdatesURL: &url, - }, - }, - }, - }) - defer s.Stopper().Stop(ctx) - s.UpdateChecker().(*diagnostics.UpdateChecker).CheckForUpdates(ctx) - r.Close() - - require.Equal(t, 1, r.NumRequests()) - - last := r.LastRequestData() - require.Equal(t, s.(*server.TestServer).StorageClusterID().String(), last.UUID) - require.Equal(t, "system", last.TenantID) - require.Equal(t, build.GetInfo().Tag, last.Version) - require.Equal(t, "OSS", last.LicenseType) - require.Equal(t, "false", last.Internal) - }) - - t.Run("npe", func(t *testing.T) { - // Ensure nil, which happens when an empty env override URL is used, does not - // cause a crash. - var nilURL *url.URL - s, _, _ := serverutils.StartServer(t, base.TestServerArgs{ - Knobs: base.TestingKnobs{ - Server: &server.TestingKnobs{ - DiagnosticsTestingKnobs: diagnostics.TestingKnobs{ - OverrideUpdatesURL: &nilURL, - OverrideReportingURL: &nilURL, - }, - }, - }, - }) - defer s.Stopper().Stop(ctx) - s.UpdateChecker().(*diagnostics.UpdateChecker).CheckForUpdates(ctx) - s.DiagnosticsReporter().(*diagnostics.Reporter).ReportDiagnostics(ctx) - }) -} diff --git a/pkg/server/server.go b/pkg/server/server.go index a981267f3a..381273ba04 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -12,12 +12,10 @@ package server import ( "context" - "fmt" "io/ioutil" "net/http" "path/filepath" "reflect" - "strconv" "sync" "time" @@ -45,7 +43,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/rpc/nodedialer" "github.com/cockroachdb/cockroach/pkg/server/debug" - "github.com/cockroachdb/cockroach/pkg/server/diagnostics" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/server/status" "github.com/cockroachdb/cockroach/pkg/server/systemconfigwatcher" @@ -86,7 +83,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/uuid" "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" - "github.com/getsentry/sentry-go" "google.golang.org/grpc/codes" ) @@ -115,7 +111,6 @@ type Server struct { runtime *status.RuntimeStatSampler ruleRegistry *metric.RuleRegistry promRuleExporter *metric.PrometheusRuleExporter - updates *diagnostics.UpdateChecker ctSender *sidetransport.Sender http *httpServer @@ -658,21 +653,6 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) { recorder := status.NewMetricsRecorder(clock, nodeLiveness, rpcContext, g, st) registry.AddMetricStruct(rpcContext.RemoteClocks.Metrics()) - updates := &diagnostics.UpdateChecker{ - StartTime: timeutil.Now(), - AmbientCtx: &cfg.AmbientCtx, - Config: cfg.BaseConfig.Config, - Settings: cfg.Settings, - StorageClusterID: rpcContext.StorageClusterID.Get, - LogicalClusterID: rpcContext.LogicalClusterID.Get, - NodeID: nodeIDContainer.Get, - SQLInstanceID: idContainer.SQLInstanceID, - } - - if cfg.TestingKnobs.Server != nil { - updates.TestingKnobs = &cfg.TestingKnobs.Server.(*TestingKnobs).DiagnosticsTestingKnobs - } - tenantUsage := NewTenantUsageServer(st, db, internalExecutor) registry.AddMetricStruct(tenantUsage.Metrics()) @@ -841,7 +821,6 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) { recorder: recorder, ruleRegistry: ruleRegistry, promRuleExporter: promRuleExporter, - updates: updates, ctSender: ctSender, runtime: runtimeSampler, http: sHTTP, @@ -1175,14 +1154,10 @@ func (s *Server) PreStart(ctx context.Context) error { listenHTTP: s.cfg.HTTPAdvertiseAddr, }.Iter() - encryptedStore := false for _, storeSpec := range s.cfg.Stores.Specs { if storeSpec.InMemory { continue } - if storeSpec.IsEncrypted() { - encryptedStore = true - } for name, val := range listenerFiles { file := filepath.Join(storeSpec.Path, name) @@ -1377,16 +1352,6 @@ func (s *Server) PreStart(ctx context.Context) error { } s.replicationReporter.Start(ctx, s.stopper) - sentry.ConfigureScope(func(scope *sentry.Scope) { - scope.SetTags(map[string]string{ - "cluster": s.StorageClusterID().String(), - "node": s.NodeID().String(), - "server_id": fmt.Sprintf("%s-%s", s.StorageClusterID().Short(), s.NodeID()), - "engine_type": s.cfg.StorageEngine.String(), - "encrypted_store": strconv.FormatBool(encryptedStore), - }) - }) - // We can now add the node registry. s.recorder.AddNode( s.registry, @@ -1631,14 +1596,6 @@ func (s *Server) PGServer() *pgwire.Server { return s.sqlServer.pgServer } -// StartDiagnostics starts periodic diagnostics reporting and update checking. -// NOTE: This is not called in PreStart so that it's disabled by default for -// testing. -func (s *Server) StartDiagnostics(ctx context.Context) { - s.updates.PeriodicallyCheckForUpdates(ctx, s.stopper) - s.sqlServer.StartDiagnostics(ctx) -} - func init() { tracing.RegisterTagRemapping("n", "node") } diff --git a/pkg/server/server_sql.go b/pkg/server/server_sql.go index 79ffa4d934..096c458d6c 100644 --- a/pkg/server/server_sql.go +++ b/pkg/server/server_sql.go @@ -1035,23 +1035,15 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) { ) reporter := &diagnostics.Reporter{ - StartTime: timeutil.Now(), - AmbientCtx: &cfg.AmbientCtx, - Config: cfg.BaseConfig.Config, - Settings: cfg.Settings, - StorageClusterID: cfg.rpcContext.StorageClusterID.Get, - LogicalClusterID: clusterIDForSQL.Get, - TenantID: cfg.rpcContext.TenantID, - SQLInstanceID: cfg.nodeIDContainer.SQLInstanceID, - SQLServer: pgServer.SQLServer, - InternalExec: cfg.circularInternalExecutor, - DB: cfg.db, - Recorder: cfg.recorder, - Locality: cfg.Locality, - } - - if cfg.TestingKnobs.Server != nil { - reporter.TestingKnobs = &cfg.TestingKnobs.Server.(*TestingKnobs).DiagnosticsTestingKnobs + StartTime: timeutil.Now(), + Settings: cfg.Settings, + TenantID: cfg.rpcContext.TenantID, + SQLInstanceID: cfg.nodeIDContainer.SQLInstanceID, + SQLServer: pgServer.SQLServer, + InternalExec: cfg.circularInternalExecutor, + DB: cfg.db, + Recorder: cfg.recorder, + Locality: cfg.Locality, } var settingsWatcher *settingswatcher.SettingsWatcher @@ -1341,13 +1333,6 @@ func (s *SQLServer) SQLInstanceID() base.SQLInstanceID { return s.sqlIDContainer.SQLInstanceID() } -// StartDiagnostics starts periodic diagnostics reporting. -// NOTE: This is not called in preStart so that it's disabled by default for -// testing. -func (s *SQLServer) StartDiagnostics(ctx context.Context) { - s.diagnosticsReporter.PeriodicallyReportDiagnostics(ctx, s.stopper) -} - // AnnotateCtx annotates the given context with the server tracer and tags. func (s *SQLServer) AnnotateCtx(ctx context.Context) context.Context { return s.ambientCtx.AnnotateCtx(ctx) diff --git a/pkg/server/stats_test.go b/pkg/server/stats_test.go index 03858e8ba5..d65c107fa6 100644 --- a/pkg/server/stats_test.go +++ b/pkg/server/stats_test.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/server/diagnostics" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" @@ -27,7 +26,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats" "github.com/cockroachdb/cockroach/pkg/sql/tests" "github.com/cockroachdb/cockroach/pkg/testutils" - "github.com/cockroachdb/cockroach/pkg/testutils/diagutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" @@ -48,16 +46,6 @@ func TestTelemetrySQLStatsIndependence(t *testing.T) { }, } - r := diagutils.NewServer() - defer r.Close() - - url := r.URL() - params.Knobs.Server = &TestingKnobs{ - DiagnosticsTestingKnobs: diagnostics.TestingKnobs{ - OverrideReportingURL: &url, - }, - } - s, sqlDB, _ := serverutils.StartServer(t, params) defer s.Stopper().Stop(ctx) @@ -79,11 +67,11 @@ CREATE TABLE t.test (x INT PRIMARY KEY); if _, err := sqlDB.Exec(`INSERT INTO t.test VALUES ($1)`, 1); err != nil { t.Fatal(err) } - s.DiagnosticsReporter().(*diagnostics.Reporter).ReportDiagnostics(ctx) + sqlServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx) if _, err := sqlDB.Exec(`INSERT INTO t.test VALUES ($1)`, 2); err != nil { t.Fatal(err) } - s.DiagnosticsReporter().(*diagnostics.Reporter).ReportDiagnostics(ctx) + sqlServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx) // Ensure that our SQL statement data was not affected by the telemetry report. stats, err := sqlServer.GetScrubbedStmtStats(ctx) diff --git a/pkg/server/testing_knobs.go b/pkg/server/testing_knobs.go index ba5f3601a7..022b48919a 100644 --- a/pkg/server/testing_knobs.go +++ b/pkg/server/testing_knobs.go @@ -19,7 +19,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" - "github.com/cockroachdb/cockroach/pkg/server/diagnostics" ) // TestingKnobs groups testing knobs for the Server. @@ -42,8 +41,6 @@ type TestingKnobs struct { PauseAfterGettingRPCAddress chan struct{} // ContextTestingKnobs allows customization of the RPC context testing knobs. ContextTestingKnobs rpc.ContextTestingKnobs - // DiagnosticsTestingKnobs allows customization of diagnostics testing knobs. - DiagnosticsTestingKnobs diagnostics.TestingKnobs // If set, use this listener for RPC (and possibly SQL, depending on // the SplitListenSQL setting), instead of binding a new listener. diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go index 4d1d3d9fc2..4f20e2ca1b 100644 --- a/pkg/server/testserver.go +++ b/pkg/server/testserver.go @@ -858,11 +858,6 @@ func (ts *TestServer) WriteSummaries() error { return ts.node.writeNodeStatus(context.TODO(), time.Hour, false) } -// UpdateChecker implements TestServerInterface. -func (ts *TestServer) UpdateChecker() interface{} { - return ts.Server.updates -} - // DiagnosticsReporter implements TestServerInterface. func (ts *TestServer) DiagnosticsReporter() interface{} { return ts.Server.sqlServer.diagnosticsReporter diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go index 088f3261fd..3ceb35790e 100644 --- a/pkg/server/version_cluster_test.go +++ b/pkg/server/version_cluster_test.go @@ -114,12 +114,6 @@ func setupMixedCluster( ServerArgsPerNode: twh.args(), }) - // We simulate crashes using this cluster, and having this enabled (which is - // a default migration) causes leaktest to complain. - if _, err := tc.ServerConn(0).Exec("SET CLUSTER SETTING diagnostics.reporting.enabled = 'false'"); err != nil { - t.Fatal(err) - } - twh.TestCluster = tc return twh } diff --git a/pkg/settings/cluster/cluster_settings.go b/pkg/settings/cluster/cluster_settings.go index b8f863e1fd..1582515db8 100644 --- a/pkg/settings/cluster/cluster_settings.go +++ b/pkg/settings/cluster/cluster_settings.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/settings" - "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" ) @@ -69,11 +68,6 @@ type OverridesInformer interface { IsOverridden(settingName string) bool } -// TelemetryOptOut is a place for controlling whether to opt out of telemetry or not. -func TelemetryOptOut() bool { - return envutil.EnvOrDefaultBool("COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING", false) -} - // NoSettings is used when a func requires a Settings but none is available // (for example, a CLI subcommand that does not connect to a cluster). var NoSettings *Settings // = nil diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index 33158e8d28..97682509cb 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -134,6 +134,11 @@ var retiredSettings = map[string]struct{}{ "tenant_cost_model.pod_cpu_second_cost": {}, "tenant_cost_model.pgwire_egress_cost_per_megabyte": {}, "sql.ttl.range_batch_size": {}, + + // [Oxide-specific] removed in our fork. + "diagnostics.reporting.enabled": {}, + "diagnostics.reporting.send_crash_reports": {}, + "diagnostics.reporting.interval": {}, } // register adds a setting to the registry. diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index 46649b8a85..0e76d79719 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -1054,12 +1054,7 @@ func (ex *connExecutor) closeWrapper(ctx context.Context, recovered interface{}) log.SqlExec.Shoutf(ctx, severity.ERROR, "a SQL panic has occurred while executing the following statement:\n%s", // For the log message, the statement is not anonymized. - truncateStatementStringForTelemetry(ex.curStmtAST.String())) - - // Embed the statement in the error object for the telemetry - // report below. The statement gets anonymized. - vt := ex.planner.extendedEvalCtx.VirtualSchemas - panicErr = WithAnonymizedStatement(panicErr, ex.curStmtAST, vt) + ex.curStmtAST.String()) } // Report the panic to telemetry in any case. @@ -3098,7 +3093,6 @@ func (ex *connExecutor) txnStateTransitionsApplyWrapper( errors.Safe(advInfo.txnEvent.eventType.String()), res.Err()) log.Errorf(ex.Ctx(), "%v", err) - errorutil.SendReport(ex.Ctx(), &ex.server.cfg.Settings.SV, err) return advanceInfo{}, err } @@ -3629,24 +3623,3 @@ type contextStatementKey struct{} func withStatement(ctx context.Context, stmt tree.Statement) context.Context { return context.WithValue(ctx, contextStatementKey{}, stmt) } - -// statementFromCtx returns the statement value from a context, or nil if unset. -func statementFromCtx(ctx context.Context) tree.Statement { - stmt := ctx.Value(contextStatementKey{}) - if stmt == nil { - return nil - } - return stmt.(tree.Statement) -} - -func init() { - // Register a function to include the anonymized statement in crash reports. - logcrash.RegisterTagFn("statement", func(ctx context.Context) string { - stmt := statementFromCtx(ctx) - if stmt == nil { - return "" - } - // Anonymize the statement for reporting. - return anonymizeStmtAndConstants(stmt, nil /* VirtualTabler */) - }) -} diff --git a/pkg/sql/conn_executor_test.go b/pkg/sql/conn_executor_test.go index 42829661da..d6f14c4e25 100644 --- a/pkg/sql/conn_executor_test.go +++ b/pkg/sql/conn_executor_test.go @@ -16,7 +16,6 @@ import ( "database/sql/driver" "fmt" "net/url" - "regexp" "strings" "sync/atomic" "testing" @@ -30,12 +29,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/execinfra" "github.com/cockroachdb/cockroach/pkg/sql/mutations" - "github.com/cockroachdb/cockroach/pkg/sql/parser" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/rowexec" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -56,76 +53,12 @@ import ( "github.com/cockroachdb/datadriven" "github.com/cockroachdb/errors" "github.com/cockroachdb/logtags" - "github.com/cockroachdb/redact" "github.com/jackc/pgconn" "github.com/jackc/pgx/v4" "github.com/lib/pq" - "github.com/pmezard/go-difflib/difflib" "github.com/stretchr/testify/require" ) -func TestAnonymizeStatementsForReporting(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - - s := cluster.MakeTestingClusterSettings() - vt, err := sql.NewVirtualSchemaHolder(context.Background(), s) - if err != nil { - t.Fatal(err) - } - - const stmt1s = ` -INSERT INTO sensitive(super, sensible) VALUES('that', 'nobody', 'must', 'see') -` - stmt1, err := parser.ParseOne(stmt1s) - if err != nil { - t.Fatal(err) - } - - rUnsafe := errors.New("some error") - safeErr := sql.WithAnonymizedStatement(rUnsafe, stmt1.AST, vt) - - const expMessage = "some error" - actMessage := safeErr.Error() - if actMessage != expMessage { - t.Errorf("wanted: %s\ngot: %s", expMessage, actMessage) - } - - const expSafeRedactedMessage = `some error -(1) while executing: INSERT INTO _(_, _) VALUES ('_', '_', __more1_10__) -Wraps: (2) attached stack trace - -- stack trace: - | github.com/cockroachdb/cockroach/pkg/sql_test.TestAnonymizeStatementsForReporting - | ...conn_executor_test.go:NN - | testing.tRunner - | ...testing.go:NN - | runtime.goexit - | ...asm_scrubbed.s:NN -Wraps: (3) some error -Error types: (1) *safedetails.withSafeDetails (2) *withstack.withStack (3) *errutil.leafError` - - // Edit non-determinstic stack trace filenames from the message. - actSafeRedactedMessage := strings.ReplaceAll(strings.ReplaceAll(fileref.ReplaceAllString( - redact.Sprintf("%+v", safeErr).Redact().StripMarkers(), "...$2:NN"), - "asm_arm64", "asm_scrubbed"), - "asm_amd64", "asm_scrubbed") - - if actSafeRedactedMessage != expSafeRedactedMessage { - diff, _ := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ - A: difflib.SplitLines(expSafeRedactedMessage), - B: difflib.SplitLines(actSafeRedactedMessage), - FromFile: "Expected", - FromDate: "", - ToFile: "Actual", - ToDate: "", - Context: 1, - }) - t.Errorf("Diff:\n%s", diff) - } -} - -var fileref = regexp.MustCompile(`((?:[a-zA-Z0-9\._@-]*/)*)([a-zA-Z0-9._@-]*\.(?:go|s)):\d+`) - // Test that a connection closed abruptly while a SQL txn is in progress results // in that txn being rolled back. // diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index a9e916fd74..a48050f79c 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -2098,20 +2098,6 @@ func (jc *jobsCollection) add(ids ...jobspb.JobID) { *jc = append(*jc, ids...) } -// truncateStatementStringForTelemetry truncates the string -// representation of a statement to a maximum length, so as to not -// create unduly large logging and error payloads. -func truncateStatementStringForTelemetry(stmt string) string { - // panicLogOutputCutoiffChars is the maximum length of the copy of the - // current statement embedded in telemetry reports and panic errors in - // logs. - const panicLogOutputCutoffChars = 10000 - if len(stmt) > panicLogOutputCutoffChars { - stmt = stmt[:len(stmt)-6] + " [...]" - } - return stmt -} - // hideNonVirtualTableNameFunc returns a function that can be used with // FmtCtx.SetReformatTableNames. It hides all table names that are not virtual // tables. @@ -2161,31 +2147,6 @@ func hideNonVirtualTableNameFunc(vt VirtualTabler) func(ctx *tree.FmtCtx, name * return reformatFn } -func anonymizeStmtAndConstants(stmt tree.Statement, vt VirtualTabler) string { - // Re-format to remove most names. - fmtFlags := tree.FmtAnonymize | tree.FmtHideConstants - var f *tree.FmtCtx - if vt != nil { - f = tree.NewFmtCtx( - fmtFlags, - tree.FmtReformatTableNames(hideNonVirtualTableNameFunc(vt)), - ) - } else { - f = tree.NewFmtCtx(fmtFlags) - } - f.FormatNode(stmt) - return f.CloseAndGetString() -} - -// WithAnonymizedStatement attaches the anonymized form of a statement -// to an error object. -func WithAnonymizedStatement(err error, stmt tree.Statement, vt VirtualTabler) error { - anonStmtStr := anonymizeStmtAndConstants(stmt, vt) - anonStmtStr = truncateStatementStringForTelemetry(anonStmtStr) - return errors.WithSafeDetails(err, - "while executing: %s", errors.Safe(anonStmtStr)) -} - // SessionTracing holds the state used by SET TRACING statements in the context // of one SQL session. // It holds the current trace being collected (or the last trace collected, if diff --git a/pkg/sql/logictest/testdata/logic_test/event_log b/pkg/sql/logictest/testdata/logic_test/event_log index ae8d3787a8..c7c6f6aeaf 100644 --- a/pkg/sql/logictest/testdata/logic_test/event_log +++ b/pkg/sql/logictest/testdata/logic_test/event_log @@ -478,7 +478,6 @@ AND info NOT LIKE '%sql.defaults.experimental_distsql_planning%' AND info NOT LIKE '%sql.defaults.use_declarative_schema_changer%' ORDER BY "timestamp", info ---- -0 1 {"ApplicationName": "$ internal-optInToDiagnosticsStatReporting", "EventType": "set_cluster_setting", "SettingName": "diagnostics.reporting.enabled", "Statement": "SET CLUSTER SETTING \"diagnostics.reporting.enabled\" = true", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "true"} 0 1 {"EventType": "set_cluster_setting", "SettingName": "kv.range_merge.queue_enabled", "Statement": "SET CLUSTER SETTING \"kv.range_merge.queue_enabled\" = false", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "false"} 0 1 {"EventType": "set_cluster_setting", "PlaceholderValues": ["5"], "SettingName": "sql.stats.automatic_collection.min_stale_rows", "Statement": "SET CLUSTER SETTING \"sql.stats.automatic_collection.min_stale_rows\" = $1::INT8", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "5"} 0 1 {"EventType": "set_cluster_setting", "SettingName": "sql.crdb_internal.table_row_statistics.as_of_time", "Statement": "SET CLUSTER SETTING \"sql.crdb_internal.table_row_statistics.as_of_time\" = e'-1\\u00B5s'", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "-00:00:00.000001"} @@ -499,7 +498,6 @@ AND info NOT LIKE '%sql.defaults.experimental_distsql_planning%' AND info NOT LIKE '%sql.defaults.use_declarative_schema_changer%' ORDER BY "timestamp", info ---- -0 1 {"ApplicationName": "$ internal-optInToDiagnosticsStatReporting", "EventType": "set_cluster_setting", "SettingName": "diagnostics.reporting.enabled", "Statement": "SET CLUSTER SETTING \"diagnostics.reporting.enabled\" = true", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "true"} 0 1 {"EventType": "set_cluster_setting", "PlaceholderValues": ["5"], "SettingName": "sql.stats.automatic_collection.min_stale_rows", "Statement": "SET CLUSTER SETTING \"sql.stats.automatic_collection.min_stale_rows\" = $1::INT8", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "5"} 0 1 {"EventType": "set_cluster_setting", "SettingName": "sql.crdb_internal.table_row_statistics.as_of_time", "Statement": "SET CLUSTER SETTING \"sql.crdb_internal.table_row_statistics.as_of_time\" = e'-1\\u00B5s'", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "-00:00:00.000001"} 0 1 {"EventType": "set_cluster_setting", "PlaceholderValues": ["'some string'"], "SettingName": "cluster.organization", "Statement": "SET CLUSTER SETTING \"cluster.organization\" = $1", "Tag": "SET CLUSTER SETTING", "User": "root", "Value": "'some string'"} diff --git a/pkg/sql/logictest/testdata/logic_test/system b/pkg/sql/logictest/testdata/logic_test/system index 2e6f92d439..47f7568896 100644 --- a/pkg/sql/logictest/testdata/logic_test/system +++ b/pkg/sql/logictest/testdata/logic_test/system @@ -1082,7 +1082,6 @@ AND name != 'sql.defaults.use_declarative_schema_changer' ORDER BY name ---- cluster.secret -diagnostics.reporting.enabled kv.range_merge.queue_enabled sql.crdb_internal.table_row_statistics.as_of_time sql.stats.automatic_collection.min_stale_rows @@ -1101,7 +1100,6 @@ AND name NOT LIKE '%sql.defaults.use_declarative_schema_changer%' ORDER BY name ---- cluster.secret -diagnostics.reporting.enabled sql.crdb_internal.table_row_statistics.as_of_time sql.stats.automatic_collection.min_stale_rows version @@ -1121,7 +1119,6 @@ WHERE name NOT IN ('version', 'sql.defaults.distsql', 'cluster.secret', 'sql.defaults.use_declarative_schema_changer') ORDER BY name ---- -diagnostics.reporting.enabled true kv.range_merge.queue_enabled false somesetting somevalue sql.crdb_internal.table_row_statistics.as_of_time -1µs @@ -1136,7 +1133,6 @@ WHERE name NOT IN ('version', 'sql.defaults.distsql', 'cluster.secret', 'sql.defaults.experimental_distsql_planning', 'sql.defaults.use_declarative_schema_changer') ORDER BY name ---- -diagnostics.reporting.enabled true somesetting somevalue sql.crdb_internal.table_row_statistics.as_of_time -1µs sql.stats.automatic_collection.min_stale_rows 5 diff --git a/pkg/sql/logictest/testdata/logic_test/txn b/pkg/sql/logictest/testdata/logic_test/txn index f1a21c3575..40d1d6ea16 100644 --- a/pkg/sql/logictest/testdata/logic_test/txn +++ b/pkg/sql/logictest/testdata/logic_test/txn @@ -1038,7 +1038,7 @@ statement error cannot execute DROP ROLE in a read-only transaction DROP ROLE testuser statement error cannot execute SET CLUSTER SETTING in a read-only transaction -SET CLUSTER SETTING diagnostics.reporting.enabled = true +SET CLUSTER SETTING feature.stats.enabled = false statement error cannot execute GRANT in a read-only transaction GRANT admin TO testuser diff --git a/pkg/sql/opt/norm/factory.go b/pkg/sql/opt/norm/factory.go index 6df3aaa172..6e8c12442a 100644 --- a/pkg/sql/opt/norm/factory.go +++ b/pkg/sql/opt/norm/factory.go @@ -332,7 +332,6 @@ func (f *Factory) onMaxConstructorStackDepthExceeded() { if buildutil.CrdbTestBuild { panic(err) } - errorutil.SendReport(f.evalCtx.Ctx(), &f.evalCtx.Settings.SV, err) } // onConstructRelational is called as a final step by each factory method that diff --git a/pkg/sql/show_test.go b/pkg/sql/show_test.go index f4f03cb10d..fa74b3d74b 100644 --- a/pkg/sql/show_test.go +++ b/pkg/sql/show_test.go @@ -1018,7 +1018,6 @@ func TestLintClusterSettingNames(t *testing.T) { "server.web_session_timeout": `server.web_session_timeout: use ".timeout" instead of "_timeout"`, "sql.distsql.flow_stream_timeout": `sql.distsql.flow_stream_timeout: use ".timeout" instead of "_timeout"`, "debug.panic_on_failed_assertions": `debug.panic_on_failed_assertions: use .enabled for booleans`, - "diagnostics.reporting.send_crash_reports": `diagnostics.reporting.send_crash_reports: use .enabled for booleans`, "kv.closed_timestamp.follower_reads_enabled": `kv.closed_timestamp.follower_reads_enabled: use ".enabled" instead of "_enabled"`, "kv.raft_log.disable_synchronization_unsafe": `kv.raft_log.disable_synchronization_unsafe: use .enabled for booleans`, "kv.range_merge.queue_enabled": `kv.range_merge.queue_enabled: use ".enabled" instead of "_enabled"`, diff --git a/pkg/sql/sqltelemetry/report.go b/pkg/sql/sqltelemetry/report.go index 75dcd8654d..72af7ed1f2 100644 --- a/pkg/sql/sqltelemetry/report.go +++ b/pkg/sql/sqltelemetry/report.go @@ -18,7 +18,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/util/log" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" "github.com/cockroachdb/errors" ) @@ -55,10 +54,5 @@ func RecordError(ctx context.Context, err error, sv *settings.Values) { // We want to log the internal error regardless of whether a // report is sent to sentry below. log.Errorf(ctx, "encountered internal error:\n%+v", err) - - if logcrash.ShouldSendReport(sv) { - event, extraDetails := errors.BuildSentryReport(err) - logcrash.SendReport(ctx, logcrash.ReportTypeError, event, extraDetails) - } } } diff --git a/pkg/sql/sqltestutils/telemetry.go b/pkg/sql/sqltestutils/telemetry.go index 85bae4e47e..49e598fd41 100644 --- a/pkg/sql/sqltestutils/telemetry.go +++ b/pkg/sql/sqltestutils/telemetry.go @@ -23,13 +23,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/server/diagnostics" + "github.com/cockroachdb/cockroach/pkg/server/diagnostics/diagnosticspb" + "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/testutils" - "github.com/cockroachdb/cockroach/pkg/testutils/diagutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/cloudinfo" @@ -95,7 +95,12 @@ func TelemetryTest(t *testing.T, serverArgs []base.TestServerArgs) { datadriven.RunTest(t, path, func(t *testing.T, td *datadriven.TestData) string { sqlServer := test.server.SQLServer().(*sql.Server) reporter := test.server.DiagnosticsReporter().(*diagnostics.Reporter) - return test.RunTest(td, test.serverDB, reporter.ReportDiagnostics, sqlServer) + createReport := func(ctx context.Context) *diagnosticspb.DiagnosticReport { + report := reporter.CreateReport(ctx, telemetry.ResetCounts) + sqlServer.GetReportedSQLStatsController().ResetLocalSQLStats(ctx) + return report + } + return test.RunTest(td, test.serverDB, createReport, sqlServer) }) }) }) @@ -103,7 +108,6 @@ func TelemetryTest(t *testing.T, serverArgs []base.TestServerArgs) { type telemetryTest struct { t *testing.T - diagSrv *diagutils.Server cluster serverutils.TestClusterInterface server serverutils.TestServerInterface serverDB *gosql.DB @@ -121,19 +125,12 @@ type rewrite struct { func (tt *telemetryTest) Start(t *testing.T, serverArgs []base.TestServerArgs) { tt.t = t - tt.diagSrv = diagutils.NewServer() var tempExternalIODir string tempExternalIODir, tt.tempDirCleanup = testutils.TempDir(tt.t) - diagSrvURL := tt.diagSrv.URL() mapServerArgs := make(map[int]base.TestServerArgs, len(serverArgs)) for i, v := range serverArgs { - v.Knobs.Server = &server.TestingKnobs{ - DiagnosticsTestingKnobs: diagnostics.TestingKnobs{ - OverrideReportingURL: &diagSrvURL, - }, - } v.ExternalIODir = tempExternalIODir mapServerArgs[i] = v } @@ -149,14 +146,13 @@ func (tt *telemetryTest) Start(t *testing.T, serverArgs []base.TestServerArgs) { func (tt *telemetryTest) Close() { tt.cluster.Stopper().Stop(context.Background()) - tt.diagSrv.Close() tt.tempDirCleanup() } func (tt *telemetryTest) RunTest( td *datadriven.TestData, db *gosql.DB, - reportDiags func(ctx context.Context), + createReport func(ctx context.Context) *diagnosticspb.DiagnosticReport, sqlServer *sql.Server, ) (out string) { defer func() { @@ -182,8 +178,7 @@ func (tt *telemetryTest) RunTest( return "" case "schema": - reportDiags(ctx) - last := tt.diagSrv.LastRequestData() + last := createReport(ctx) var buf bytes.Buffer for i := range last.Schema { buf.WriteString(formatTableDescriptor(&last.Schema[i])) @@ -200,14 +195,13 @@ func (tt *telemetryTest) RunTest( case "feature-usage", "feature-counters": // Report diagnostics once to reset the counters. - reportDiags(ctx) + createReport(ctx) _, err := db.Exec(td.Input) var buf bytes.Buffer if err != nil { fmt.Fprintf(&buf, "error: %v\n", err) } - reportDiags(ctx) - last := tt.diagSrv.LastRequestData() + last := createReport(ctx) usage := last.FeatureUsage keys := make([]string, 0, len(usage)) for k, v := range usage { @@ -237,7 +231,7 @@ func (tt *telemetryTest) RunTest( case "sql-stats": // Report diagnostics once to reset the stats. sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx) - reportDiags(ctx) + createReport(ctx) _, err := db.Exec(td.Input) var buf bytes.Buffer @@ -245,8 +239,7 @@ func (tt *telemetryTest) RunTest( fmt.Fprintf(&buf, "error: %v\n", err) } sqlServer.GetSQLStatsController().ResetLocalSQLStats(ctx) - reportDiags(ctx) - last := tt.diagSrv.LastRequestData() + last := createReport(ctx) buf.WriteString(formatSQLStats(last.SqlStats)) return buf.String() @@ -273,9 +266,6 @@ func (tt *telemetryTest) RunTest( func (tt *telemetryTest) prepareCluster(db *gosql.DB) { runner := sqlutils.MakeSQLRunner(db) - // Disable automatic reporting so it doesn't interfere with the test. - runner.Exec(tt.t, "SET CLUSTER SETTING diagnostics.reporting.enabled = false") - runner.Exec(tt.t, "SET CLUSTER SETTING diagnostics.reporting.send_crash_reports = false") // Disable plan caching to get accurate counts if the same statement is // issued multiple times. runner.Exec(tt.t, "SET CLUSTER SETTING sql.query_cache.enabled = false") diff --git a/pkg/startupmigrations/migrations.go b/pkg/startupmigrations/migrations.go index b676d19f19..b6c257f79f 100644 --- a/pkg/startupmigrations/migrations.go +++ b/pkg/startupmigrations/migrations.go @@ -719,17 +719,12 @@ func extendCreateRoleWithCreateLogin(ctx context.Context, r runner) error { // an explicit value for a setting, effectively changing the "default value" // from what was defined in code. var SettingsDefaultOverrides = map[string]string{ - "diagnostics.reporting.enabled": "true", - "cluster.secret": "", + "cluster.secret": "", } func optInToDiagnosticsStatReporting(ctx context.Context, r runner) error { - // We're opting-out of the automatic opt-in. See discussion in updates.go. - if cluster.TelemetryOptOut() { - return nil - } - return r.execAsRootWithRetry(ctx, "optInToDiagnosticsStatReporting", - `SET CLUSTER SETTING diagnostics.reporting.enabled = true`) + // Diagnostics reporting has been removed. + return nil } func initializeClusterSecret(ctx context.Context, r runner) error { diff --git a/pkg/testutils/diagutils/diag_test_server.go b/pkg/testutils/diagutils/diag_test_server.go deleted file mode 100644 index d15888e254..0000000000 --- a/pkg/testutils/diagutils/diag_test_server.go +++ /dev/null @@ -1,119 +0,0 @@ -// Copyright 2020 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package diagutils - -import ( - "io/ioutil" - "net/http" - "net/http/httptest" - "net/url" - - "github.com/cockroachdb/cockroach/pkg/server/diagnostics/diagnosticspb" - "github.com/cockroachdb/cockroach/pkg/util/protoutil" - "github.com/cockroachdb/cockroach/pkg/util/syncutil" -) - -// Server is a http server that implements a diagnostics endpoint. Its URL can -// be used as the updates or reporting URL (see diagonsticspb.TestingKnobs). -type Server struct { - httpSrv *httptest.Server - url *url.URL - - mu struct { - syncutil.Mutex - - numRequests int - last *RequestData - } -} - -// RequestData stores the data provided by a diagnostics request. -type RequestData struct { - UUID string - TenantID string - NodeID string - SQLInstanceID string - Version string - LicenseType string - Internal string - RawReportBody string - - diagnosticspb.DiagnosticReport -} - -// NewServer creates and starts a new server. The server must be closed. -func NewServer() *Server { - srv := &Server{} - - srv.httpSrv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - defer r.Body.Close() - body, err := ioutil.ReadAll(r.Body) - if err != nil { - panic(err) - } - - srv.mu.Lock() - defer srv.mu.Unlock() - - srv.mu.numRequests++ - - data := &RequestData{ - UUID: r.URL.Query().Get("uuid"), - TenantID: r.URL.Query().Get("tenantid"), - NodeID: r.URL.Query().Get("nodeid"), - SQLInstanceID: r.URL.Query().Get("sqlid"), - Version: r.URL.Query().Get("version"), - LicenseType: r.URL.Query().Get("licensetype"), - Internal: r.URL.Query().Get("internal"), - RawReportBody: string(body), - } - - // TODO(dt): switch on the request path to handle different request types. - if err := protoutil.Unmarshal(body, &data.DiagnosticReport); err != nil { - panic(err) - } - srv.mu.last = data - })) - - var err error - srv.url, err = url.Parse(srv.httpSrv.URL) - if err != nil { - panic(err) - } - - return srv -} - -// URL returns the URL that can be used to send requests to the server. -func (s *Server) URL() *url.URL { - return s.url -} - -// Close shuts down the server and blocks until all outstanding -// requests on this server have completed. -func (s *Server) Close() { - s.httpSrv.Close() -} - -// NumRequests returns the total number of requests received by this server. -func (s *Server) NumRequests() int { - s.mu.Lock() - defer s.mu.Unlock() - return s.mu.numRequests -} - -// LastRequestData returns the data from last request received by the server. -// Returns nil if there were no requests. -func (s *Server) LastRequestData() *RequestData { - s.mu.Lock() - defer s.mu.Unlock() - return s.mu.last -} diff --git a/pkg/testutils/serverutils/test_server_shim.go b/pkg/testutils/serverutils/test_server_shim.go index e8573775d4..e34bf91ab0 100644 --- a/pkg/testutils/serverutils/test_server_shim.go +++ b/pkg/testutils/serverutils/test_server_shim.go @@ -173,11 +173,6 @@ type TestServerInterface interface { // inside the specified database. ForceTableGC(ctx context.Context, database, table string, timestamp hlc.Timestamp) error - // UpdateChecker returns the server's *diagnostics.UpdateChecker as an - // interface{}. The UpdateChecker periodically phones home to check for new - // updates that are available. - UpdateChecker() interface{} - // StartTenant spawns off tenant process connecting to this TestServer. StartTenant(ctx context.Context, params base.TestTenantArgs) (TestTenantInterface, error) diff --git a/pkg/testutils/serverutils/test_tenant_shim.go b/pkg/testutils/serverutils/test_tenant_shim.go index 169cfe2bd9..63255825c1 100644 --- a/pkg/testutils/serverutils/test_tenant_shim.go +++ b/pkg/testutils/serverutils/test_tenant_shim.go @@ -49,8 +49,8 @@ type TestTenantInterface interface { PGServer() interface{} // DiagnosticsReporter returns the tenant's *diagnostics.Reporter as an - // interface{}. The DiagnosticsReporter periodically phones home to report - // diagnostics and usage. + // interface{}. The DiagnosticsReporter can generate diagnostics and usage + // reports. DiagnosticsReporter() interface{} // StatusServer returns the tenant's *server.SQLStatusServer as an diff --git a/pkg/util/errorutil/error.go b/pkg/util/errorutil/error.go index 48c8618bcb..11f68f38c1 100644 --- a/pkg/util/errorutil/error.go +++ b/pkg/util/errorutil/error.go @@ -11,11 +11,8 @@ package errorutil import ( - "context" "fmt" - "github.com/cockroachdb/cockroach/pkg/settings" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" "github.com/cockroachdb/errors" ) @@ -37,14 +34,3 @@ func UnexpectedWithIssueErrorf(issue int, format string, args ...interface{}) er issue)) return err } - -// SendReport creates a Sentry report about the error, if the settings allow. -// The format string will be reproduced ad litteram in the report; the arguments -// will be sanitized. -func SendReport(ctx context.Context, sv *settings.Values, err error) { - if !logcrash.ShouldSendReport(sv) { - return - } - event, extraDetails := errors.BuildSentryReport(err) - logcrash.SendReport(ctx, logcrash.ReportTypeError, event, extraDetails) -} diff --git a/pkg/util/log/channels.go b/pkg/util/log/channels.go index a084bcc88a..598300446e 100644 --- a/pkg/util/log/channels.go +++ b/pkg/util/log/channels.go @@ -20,7 +20,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log/channel" "github.com/cockroachdb/cockroach/pkg/util/log/logpb" "github.com/cockroachdb/cockroach/pkg/util/log/severity" - "github.com/cockroachdb/errors" ) //go:generate go run gen/main.go logpb/log.proto logging.md ../../../docs/generated/logging.md @@ -73,10 +72,6 @@ func logfDepthInternal( }) defer t.Stop() - if MaybeSendCrashReport != nil { - err := errors.NewWithDepthf(depth+1, "log.Fatal: "+format, args...) - MaybeSendCrashReport(ctx, err) - } if ch != channel.OPS { // Tell the OPS channel about this termination. logfDepth(ctx, depth+1, severity.INFO, channel.OPS, @@ -138,6 +133,3 @@ func (l *loggingT) getLogger(ch Channel) *loggerT { func LoggingToStderr(s Severity) bool { return s >= logging.stderrSinkInfoTemplate.threshold.get(channel.DEV) } - -// MaybeSendCrashReport is injected by package logcrash -var MaybeSendCrashReport func(ctx context.Context, err error) diff --git a/pkg/util/log/channels_test.go b/pkg/util/log/channels_test.go index 420ad746fa..ad99f1501a 100644 --- a/pkg/util/log/channels_test.go +++ b/pkg/util/log/channels_test.go @@ -80,7 +80,6 @@ func TestRepro81025(t *testing.T) { defer Scope(t).Close(t) // Don't bother sending crash events. - MaybeSendCrashReport = func(ctx context.Context, err error) {} ExitTimeoutOnFatalLog = time.Second // Create a new file-backed sink. diff --git a/pkg/util/log/logcrash/crash_reporting.go b/pkg/util/log/logcrash/crash_reporting.go index 46736bcd8c..5991ae8ef2 100644 --- a/pkg/util/log/logcrash/crash_reporting.go +++ b/pkg/util/log/logcrash/crash_reporting.go @@ -12,18 +12,14 @@ package logcrash import ( "context" - "fmt" "sync/atomic" - "time" "github.com/cockroachdb/cockroach/pkg/build" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/util/envutil" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/log/severity" - "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" - sentry "github.com/getsentry/sentry-go" ) // The call stack here is usually: @@ -34,38 +30,6 @@ import ( const depthForRecoverAndReportPanic = 3 var ( - // crashReportEnv controls the version reported in crash reports - crashReportEnv = "development" - - // DiagnosticsReportingEnabled wraps "diagnostics.reporting.enabled". - // - // "diagnostics.reporting.enabled" enables reporting of metrics related to a - // node's storage (number, size and health of ranges) back to CockroachDB. - // Collecting this data from production clusters helps us understand and improve - // how our storage systems behave in real-world use cases. - // - // Note: while the setting itself is actually defined with a default value of - // `false`, it is usually automatically set to `true` when a cluster is created - // (or is migrated from a earlier beta version). This can be prevented with the - // env var COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING. - // - // Doing this, rather than just using a default of `true`, means that a node - // will not errantly send a report using a default before loading settings. - DiagnosticsReportingEnabled = settings.RegisterBoolSetting( - settings.TenantWritable, - "diagnostics.reporting.enabled", - "enable reporting diagnostic metrics to cockroach labs", - false, - ).WithPublic() - - // CrashReports wraps "diagnostics.reporting.send_crash_reports". - CrashReports = settings.RegisterBoolSetting( - settings.TenantWritable, - "diagnostics.reporting.send_crash_reports", - "send crash and panic reports", - true, - ) - // PanicOnAssertions wraps "debug.panic_on_failed_assertions" PanicOnAssertions = settings.RegisterBoolSetting( settings.TenantWritable, @@ -74,10 +38,6 @@ var ( false, ) - // startTime records when the process started so that crash reports can - // include the server's uptime as an extra tag. - startTime = timeutil.Now() - // ReportSensitiveDetails enables reporting of unanonymized data. // // This should not be used by anyone unwilling to share the whole cluster @@ -173,9 +133,6 @@ func ReportPanic(ctx context.Context, sv *settings.Values, r interface{}, depth panicErr := PanicAsError(depth+1, r) log.Ops.Shoutf(ctx, severity.ERROR, "a panic has occurred!\n%+v", panicErr) - // In addition to informing the user, also report the details to telemetry. - sendCrashReport(ctx, sv, panicErr, ReportTypePanic) - // Ensure that the logs are flushed before letting a panic // terminate the server. log.Flush() @@ -189,180 +146,6 @@ func PanicAsError(depth int, r interface{}) error { return errors.NewWithDepthf(depth+1, "panic: %v", r) } -// Crash reporting URL. -// -// This uses a Sentry proxy run by Cockroach Labs. The proxy -// abstracts the actual Sentry submission project ID and -// submission key. -// -// Non-release builds wishing to use Sentry reports -// are invited to use the following URL instead: -// -// https://ignored@errors.cockroachdb.com/api/sentrydev/v2/1111 -// -// This can be set via e.g. the env var COCKROACH_CRASH_REPORTS. -// Note that the special number "1111" is important as it -// needs to be matched exactly by the CRL proxy. -// -// TODO(knz): We could envision auto-selecting this alternate URL -// when detecting a non-release build. -var crashReportURL = func() string { - var defaultURL string - if build.SeemsOfficial() { - defaultURL = "https://ignored@errors.cockroachdb.com/api/sentry/v2/1111" - } - return envutil.EnvOrDefaultString("COCKROACH_CRASH_REPORTS", defaultURL) -}() - -// crashReportingActive is set to true if raven has been initialized. -var crashReportingActive bool - -// SetupCrashReporter sets the crash reporter info. -func SetupCrashReporter(ctx context.Context, cmd string) { - if crashReportURL == "" { - return - } - - if cmd == "start" { - cmd = "server" - } - info := build.GetInfo() - - if err := sentry.Init(sentry.ClientOptions{ - Dsn: crashReportURL, - Environment: crashReportEnv, - Release: info.Tag, - Dist: info.Distribution, - }); err != nil { - panic(errors.Wrap(err, "failed to setup crash reporting")) - } - - crashReportingActive = true - - sentry.ConfigureScope(func(scope *sentry.Scope) { - scope.SetTags(map[string]string{ - "cmd": cmd, - "platform": info.Platform, - "distribution": info.Distribution, - "rev": info.Revision, - "goversion": info.GoVersion, - "buildchannel": info.Channel, - "envchannel": info.EnvChannel, - }) - }) -} - -func uptimeTag(now time.Time) string { - uptime := now.Sub(startTime) - switch { - case uptime < 1*time.Second: - return "<1s" - case uptime < 10*time.Second: - return "<10s" - case uptime < 1*time.Minute: - return "<1m" - case uptime < 10*time.Minute: - return "<10m" - case uptime < 1*time.Hour: - return "<1h" - case uptime < 10*time.Hour: - return "<10h" - default: - daysUp := int(uptime / (24 * time.Hour)) - return fmt.Sprintf("<%dd", daysUp+1) - } -} - -// ReportType is used to differentiate between an actual crash/panic and just -// reporting an error. This data is useful for stability purposes. -type ReportType int - -const ( - // ReportTypePanic signifies that this is an actual panic. - ReportTypePanic ReportType = iota - // ReportTypeError signifies that this is just a report of an error but it - // still may include an exception and stack trace. - ReportTypeError - // ReportTypeLogFatal signifies that this is an error report that - // was generated via a log.Fatal call. - ReportTypeLogFatal -) - -// sendCrashReport posts to sentry. -// -// The crashReportType parameter adds a tag to the event that shows if the -// cluster did indeed crash or not. -func sendCrashReport( - ctx context.Context, sv *settings.Values, err error, crashReportType ReportType, -) { - if !ShouldSendReport(sv) { - return - } - - errEvent, extraDetails := errors.BuildSentryReport(err) - SendReport(ctx, crashReportType, errEvent, extraDetails) -} - -// ShouldSendReport returns true iff SendReport() should be called. -func ShouldSendReport(sv *settings.Values) bool { - if sv == nil || !DiagnosticsReportingEnabled.Get(sv) || !CrashReports.Get(sv) { - return false // disabled via settings. - } - if !crashReportingActive { - return false // disabled via empty URL env var. - } - return true -} - -// SendReport uploads a detailed error report to sentry. -// Note that there can be at most one reportable object of each type in the report. -// For more messages, use extraDetails. -// The crashReportType parameter adds a tag to the event that shows if the -// cluster did indeed crash or not. -func SendReport( - ctx context.Context, - crashReportType ReportType, - event *sentry.Event, - extraDetails map[string]interface{}, -) { - for extraKey, extraValue := range extraDetails { - event.Extra[extraKey] = extraValue - } - - if !ReportSensitiveDetails { - // Avoid leaking the machine's hostname by injecting the literal "". - // Otherwise, raven.Client.Capture will see an empty ServerName field and - // automatically fill in the machine's hostname. - event.ServerName = "" - } - - event.Tags["uptime"] = uptimeTag(timeutil.Now()) - - switch crashReportType { - case ReportTypePanic: - event.Tags["report_type"] = "panic" - case ReportTypeError: - event.Tags["report_type"] = "error" - case ReportTypeLogFatal: - event.Tags["report_type"] = "log_fatal" - } - - for _, f := range tagFns { - v := f.value(ctx) - if v != "" { - event.Tags[f.key] = maybeTruncate(v) - } - } - - res := sentry.CaptureEvent(event) - if res != nil { - log.Shoutf(ctx, severity.ERROR, "Queued as error %v", string(*res)) - } - if !sentry.Flush(10 * time.Second) { - log.Shout(ctx, severity.ERROR, "Timeout trying to submit crash report") - } -} - // ReportOrPanic either reports an error to sentry, if run from a release // binary, or panics, if triggered in tests. This is intended to be used for // failing assertions which are recoverable but serious enough to report and to @@ -378,41 +161,4 @@ func ReportOrPanic( panic(err) } log.Warningf(ctx, "%v", err) - sendCrashReport(ctx, sv, err, ReportTypeError) -} - -// Sentry max tag value length. -// From: https://github.com/getsentry/sentry-docs/pull/1304/files -const maxTagLen = 200 - -func maybeTruncate(tagValue string) string { - if len(tagValue) > maxTagLen { - const truncatedSuffix = " [...]" - return tagValue[:maxTagLen-len(truncatedSuffix)] + truncatedSuffix - } - return tagValue -} - -type tagFn struct { - key string - value func(context.Context) string -} - -var tagFns []tagFn - -// RegisterTagFn adds a function for tagging crash reports based on the context. -// This is intended to be called by other packages at init time. -func RegisterTagFn(key string, value func(context.Context) string) { - tagFns = append(tagFns, tagFn{key, value}) -} - -func maybeSendCrashReport(ctx context.Context, err error) { - // We load the ReportingSettings from global singleton in this call path. - if sv := getGlobalSettings(); sv != nil { - sendCrashReport(ctx, sv, err, ReportTypeLogFatal) - } -} - -func init() { - log.MaybeSendCrashReport = maybeSendCrashReport } diff --git a/pkg/util/log/logcrash/crash_reporting_packet_test.go b/pkg/util/log/logcrash/crash_reporting_packet_test.go deleted file mode 100644 index 361a486ba3..0000000000 --- a/pkg/util/log/logcrash/crash_reporting_packet_test.go +++ /dev/null @@ -1,286 +0,0 @@ -// Copyright 2017 The Cockroach Authors. -// -// Use of this software is governed by the Business Source License -// included in the file licenses/BSL.txt. -// -// As of the Change Date specified in that file, in accordance with -// the Business Source License, use of this software will be governed -// by the Apache License, Version 2.0, included in the file -// licenses/APL.txt. - -package logcrash_test - -import ( - "context" - "regexp" - "runtime" - "strings" - "testing" - "time" - - "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" - "github.com/cockroachdb/cockroach/pkg/testutils" - "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/util/leaktest" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" - "github.com/cockroachdb/redact" - sentry "github.com/getsentry/sentry-go" - "github.com/kr/pretty" - "github.com/stretchr/testify/assert" -) - -// Renumber lines so they're stable no matter what changes above. (We -// could make the regexes accept any string of digits, but we also -// want to make sure that the correct line numbers get captured). -// -//line crash_reporting_packet_test.go:1000 - -// interceptingTransport is an implementation of raven.Transport that delegates -// calls to the Send method to the send function contained within. -type interceptingTransport struct { - send func(packet *sentry.Event) -} - -// SendEvent implements the sentry.Transport interface. -func (it interceptingTransport) SendEvent(packet *sentry.Event) { - it.send(packet) -} - -// Flush implements the sentry.Transport interface. -func (it interceptingTransport) Flush(time.Duration) bool { return true } - -// Configure implements the sentry.Transport interface. -func (it interceptingTransport) Configure(sentry.ClientOptions) {} - -func TestCrashReportingPacket(t *testing.T) { - defer leaktest.AfterTest(t)() - - ctx := context.Background() - var packets []*sentry.Event - - st := cluster.MakeTestingClusterSettings() - // Enable all crash-reporting settings. - logcrash.DiagnosticsReportingEnabled.Override(ctx, &st.SV, true) - - defer logcrash.TestingSetCrashReportingURL("https://ignored:ignored@ignored/1234")() - - logcrash.SetupCrashReporter(ctx, "test") - - // Install a Transport that locally records events rather than sending them - // to Sentry over HTTP. - defer func(transport sentry.Transport) { - sentry.CurrentHub().Client().Transport = transport - }(sentry.CurrentHub().Client().Transport) - sentry.CurrentHub().Client().Transport = interceptingTransport{ - send: func(packet *sentry.Event) { - packets = append(packets, packet) - }, - } - - expectPanic := func(name string) { - if r := recover(); r == nil { - t.Fatalf("'%s' failed to panic", name) - } - } - - const ( - panicPre = "boom" - panicPost = "baam" - ) - - func() { - defer expectPanic("before server start") - defer logcrash.RecoverAndReportPanic(ctx, &st.SV) - panic(redact.Safe(panicPre)) - }() - - func() { - defer expectPanic("after server start") - defer logcrash.RecoverAndReportPanic(ctx, &st.SV) - s, _, _ := serverutils.StartServer(t, base.TestServerArgs{}) - s.Stopper().Stop(ctx) - panic(redact.Safe(panicPost)) - }() - - const prefix = "crash_reporting_packet_test.go:" - - expectations := []struct { - serverID *regexp.Regexp - tagCount int - title string - message *regexp.Regexp - }{ - {regexp.MustCompile(`^$`), 9, func() string { - message := prefix - // gccgo stack traces are different in the presence of function literals. - if runtime.Compiler == "gccgo" { - message += "[0-9]+" // TODO(bdarnell): verify on gccgo - } else { - message += "1058" - } - message += " (TestCrashReportingPacket)" - return message - }(), - regexp.MustCompile(`crash_reporting_packet_test.go:\d+: panic: boom`), - }, - {regexp.MustCompile(`^[a-z0-9]{8}-1$`), 14, func() string { - message := prefix - // gccgo stack traces are different in the presence of function literals. - if runtime.Compiler == "gccgo" { - message += "[0-9]+" // TODO(bdarnell): verify on gccgo - } else { - message += "1066" - } - message += " (TestCrashReportingPacket)" - return message - }(), - regexp.MustCompile(`crash_reporting_packet_test.go:\d+: panic: baam`), - }, - } - - if e, a := len(expectations), len(packets); e != a { - t.Fatalf("expected %d packets, but got %d", e, a) - } - - for _, exp := range expectations { - p := packets[0] - packets = packets[1:] - t.Run("", func(t *testing.T) { - t.Logf("message: %q", p.Message) - - if !logcrash.ReportSensitiveDetails { - e, a := "", p.ServerName - if e != a { - t.Errorf("expected ServerName to be '', but got '%s'", a) - } - } - - tags := p.Tags - if e, a := exp.tagCount, len(tags); e != a { - t.Errorf("expected %d tags, but got %d", e, a) - } - - if serverID := tags["server_id"]; !exp.serverID.MatchString(serverID) { - t.Errorf("expected server_id '%s' to match %s", serverID, exp.serverID) - } - - assert.Regexp(t, exp.message, p.Message) - - if len(p.Exception) < 1 { - t.Error("expected some exception in packet, got none") - } else { - if p.Exception[0].Type != exp.title { - t.Errorf("expected %q in exception type, got %q", - exp.title, p.Exception[0].Type) - } - - lastFrame := p.Exception[0].Stacktrace.Frames[len(p.Exception[0].Stacktrace.Frames)-1] - if lastFrame.Function != "TestCrashReportingPacket" { - t.Errorf("last frame function: expected TestCrashReportingPacket, got %q", lastFrame.Function) - } - if !strings.HasSuffix(lastFrame.Filename, "crash_reporting_packet_test.go") { - t.Errorf("last frame filename: expected crash_reporting_packet_test.go, got %q", lastFrame.Filename) - } - } - }) - } -} - -func TestInternalErrorReporting(t *testing.T) { - defer leaktest.AfterTest(t)() - - ctx := context.Background() - var packets []*sentry.Event - - st := cluster.MakeTestingClusterSettings() - // Enable all crash-reporting settings. - logcrash.DiagnosticsReportingEnabled.Override(ctx, &st.SV, true) - - defer logcrash.TestingSetCrashReportingURL("https://ignored:ignored@ignored/1234")() - - logcrash.SetupCrashReporter(ctx, "test") - - // Install a Transport that locally records packets rather than sending them - // to Sentry over HTTP. - defer func(transport sentry.Transport) { - sentry.CurrentHub().Client().Transport = transport - }(sentry.CurrentHub().Client().Transport) - sentry.CurrentHub().Client().Transport = interceptingTransport{ - send: func(packet *sentry.Event) { - packets = append(packets, packet) - }, - } - - s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{}) - defer s.Stopper().Stop(ctx) - _, err := sqlDB.Exec(`SET CLUSTER SETTING sql.metrics.statement_details.plan_collection.enabled = true;`) - if err != nil { - t.Errorf("failed to enable plan collection due to %s", err.Error()) - } - - if _, err := sqlDB.Exec("SELECT crdb_internal.force_assertion_error('woo')"); !testutils.IsError(err, "internal error") { - t.Fatalf("expected internal error, got %v", err) - } - - // TODO(knz): With Ben's changes to report errors in pgwire as well - // as the executor, we get twice the sentry reports. This needs to - // be fixed, then the test below can check for len(packets) == 1. - if len(packets) < 1 { - t.Fatalf("expected at least 1 reporting packet") - } - - p := packets[0] - - t.Logf("%# v", pretty.Formatter(p)) - - // rm is the redaction mark, what remains after redaction when - // the redaction markers are removed. - rm := string(redact.RedactableBytes(redact.RedactedMarker()).StripMarkers()) - - assert.Regexp(t, `builtins\.go:\d+: crdb_internal.force_assertion_error\(\): `+rm+`\n`, p.Message) - idx := strings.Index(p.Message, "-- report composition:\n") - assert.GreaterOrEqual(t, idx, 1) - if idx > 0 { - assert.Regexp(t, - `-- report composition:\n`+ - `\*errutil.leafError: `+rm+`\n`+ - `builtins.go:\d+: \*withstack.withStack \(top exception\)\n`+ - `\*assert.withAssertionFailure\n`+ - `\*errutil.withPrefix: crdb_internal.force_assertion_error\(\)\n`+ - `eval.go:\d+: \*withstack.withStack \(1\)\n`+ - `\*telemetrykeys.withTelemetry: crdb_internal.force_assertion_error\(\)\n`+ - `\*colexecerror.notInternalError\n`+ - `\(check the extra data payloads\)`, p.Message[idx:]) - } - - if len(p.Exception) < 2 { - t.Fatalf("expected 2 stacktraces, got %d", len(p.Exception)) - } - - extra, ok := p.Extra["error types"] - assert.True(t, ok) - if ok { - assert.Equal(t, "github.com/cockroachdb/errors/errutil/*errutil.leafError (*::)\n"+ - "github.com/cockroachdb/errors/withstack/*withstack.withStack (*::)\n"+ - "github.com/cockroachdb/errors/assert/*assert.withAssertionFailure (*::)\n"+ - "github.com/cockroachdb/errors/errutil/*errutil.withPrefix (*::)\n"+ - "github.com/cockroachdb/errors/withstack/*withstack.withStack (*::)\n"+ - "github.com/cockroachdb/errors/telemetrykeys/*telemetrykeys.withTelemetry (*::)\n"+ - "github.com/cockroachdb/cockroach/pkg/sql/colexecerror/*colexecerror.notInternalError (*::)\n", - extra) - } - - // The innermost stack trace (and main exception object) is the last - // one in the Sentry event. - assert.Regexp(t, `^builtins.go:\d+ \(.*\)$`, p.Exception[1].Type) - assert.Regexp(t, `^\*errutil\.leafError: crdb_internal\.force_assertion_error\(\): `+rm, p.Exception[1].Value) - fr := p.Exception[1].Stacktrace.Frames - assert.Regexp(t, `.*/builtins.go`, fr[len(fr)-1].Filename) - assert.Regexp(t, `.*/eval.go`, fr[len(fr)-2].Filename) - - assert.Regexp(t, `^\(1\) eval.go:\d+ \(MaybeWrapError\)$`, p.Exception[0].Type) - assert.Regexp(t, `^\*withstack\.withStack$`, p.Exception[0].Value) - fr = p.Exception[0].Stacktrace.Frames - assert.Regexp(t, `.*/eval.go`, fr[len(fr)-1].Filename) -} diff --git a/pkg/util/log/logcrash/crash_reporting_test.go b/pkg/util/log/logcrash/crash_reporting_test.go index 65a0991b43..81c70d65ce 100644 --- a/pkg/util/log/logcrash/crash_reporting_test.go +++ b/pkg/util/log/logcrash/crash_reporting_test.go @@ -17,10 +17,8 @@ import ( "regexp" "runtime" "testing" - "time" "github.com/cockroachdb/cockroach/pkg/util" - "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/errors" "github.com/cockroachdb/redact" "github.com/pmezard/go-difflib/difflib" @@ -80,7 +78,7 @@ Error types: (1) *runtime.TypeAssertionError`, expErr: `some visible detail: interface conversion: interface {} is nil, not int (1) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -102,7 +100,7 @@ Error types: (1) *withstack.withStack (2) *errutil.withPrefix (3) *runtime.TypeA expErr: `interface conversion: interface {} is nil, not int (1) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -134,7 +132,7 @@ Error types: (1) *safedetails.withSafeDetails (2) *runtime.TypeAssertionError`, expErr: `I like A and my pin code is ` + rm + ` or 9999 (1) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -154,7 +152,7 @@ Error types: (1) *withstack.withStack (2) *errutil.leafError`, expErr: `this is preserved: 6: context canceled (1) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -190,7 +188,7 @@ Error types: (1) *os.LinkError (2) *safedetails.withSafeDetails (3) logcrash.lea expErr: `this is reportable as well: this is reportable too: this is reportable: ` + rm + ` (1) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -203,7 +201,7 @@ Error types: (1) *os.LinkError (2) *safedetails.withSafeDetails (3) logcrash.lea Wraps: (2) this is reportable as well Wraps: (3) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -216,13 +214,13 @@ Wraps: (3) attached stack trace Wraps: (4) this is reportable too Wraps: (5) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | [...repeated from below...] Wraps: (6) this is reportable Wraps: (7) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -255,7 +253,7 @@ Error types: (1) *net.OpError (2) logcrash.leafErr`, expErr: `this embed an error: this is reportable too: this is reportable: ` + rm + ` (1) attached stack trace -- stack trace: - | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | ...crash_reporting_test.go:NN | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | ...crash_reporting_test.go:NN @@ -271,7 +269,7 @@ Wraps: (2) secondary error attachment | this is reportable too: this is reportable: ` + rm + ` | (1) attached stack trace | -- stack trace: - | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | | ...crash_reporting_test.go:NN | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | | ...crash_reporting_test.go:NN @@ -284,13 +282,13 @@ Wraps: (2) secondary error attachment | Wraps: (2) this is reportable too | Wraps: (3) attached stack trace | -- stack trace: - | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | | ...crash_reporting_test.go:NN | | [...repeated from below...] | Wraps: (4) this is reportable | Wraps: (5) attached stack trace | -- stack trace: - | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func2 + | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init.func1 | | ...crash_reporting_test.go:NN | | github.com/cockroachdb/cockroach/pkg/util/log/logcrash.init | | ...crash_reporting_test.go:NN @@ -337,45 +335,6 @@ type leafErr struct{} func (leafErr) Error() string { return "error" } -func TestingSetCrashReportingURL(url string) func() { - oldCrashReportURL := crashReportURL - crashReportURL = url - return func() { crashReportURL = oldCrashReportURL } -} - -func TestUptimeTag(t *testing.T) { - startTime = timeutil.Unix(0, 0) - testCases := []struct { - crashTime time.Time - expected string - }{ - {timeutil.Unix(0, 0), "<1s"}, - {timeutil.Unix(0, 0), "<1s"}, - {timeutil.Unix(1, 0), "<10s"}, - {timeutil.Unix(9, 0), "<10s"}, - {timeutil.Unix(10, 0), "<1m"}, - {timeutil.Unix(59, 0), "<1m"}, - {timeutil.Unix(60, 0), "<10m"}, - {timeutil.Unix(9*60, 0), "<10m"}, - {timeutil.Unix(10*60, 0), "<1h"}, - {timeutil.Unix(59*60, 0), "<1h"}, - {timeutil.Unix(60*60, 0), "<10h"}, - {timeutil.Unix(9*60*60, 0), "<10h"}, - {timeutil.Unix(10*60*60, 0), "<1d"}, - {timeutil.Unix(23*60*60, 0), "<1d"}, - {timeutil.Unix(24*60*60, 0), "<2d"}, - {timeutil.Unix(47*60*60, 0), "<2d"}, - {timeutil.Unix(119*60*60, 0), "<5d"}, - {timeutil.Unix(10*24*60*60, 0), "<11d"}, - {timeutil.Unix(365*24*60*60, 0), "<366d"}, - } - for _, tc := range testCases { - if a, e := uptimeTag(tc.crashTime), tc.expected; a != e { - t.Errorf("uptimeTag(%v) got %v, want %v)", tc.crashTime, a, e) - } - } -} - // makeTypeAssertionErr returns a runtime.Error with the message: // interface conversion: interface {} is nil, not int func makeTypeAssertionErr() (result runtime.Error) { diff --git a/pkg/util/log/logcrash/main_test.go b/pkg/util/log/logcrash/main_test.go index 08c7c0102d..817e0d16e5 100644 --- a/pkg/util/log/logcrash/main_test.go +++ b/pkg/util/log/logcrash/main_test.go @@ -11,16 +11,13 @@ package logcrash_test import ( - "context" "os" "testing" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/security/securitytest" "github.com/cockroachdb/cockroach/pkg/server" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" - "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" "github.com/cockroachdb/cockroach/pkg/util/randutil" ) @@ -28,14 +25,6 @@ func TestMain(m *testing.M) { randutil.SeedForTests() security.SetAssetLoader(securitytest.EmbeddedAssets) serverutils.InitTestServerFactory(server.TestServerFactory) - ctx := context.Background() - - // MakeTestingClusterSettings initializes log.ReportingSettings to this - // instance of setting values. - // TODO(knz): This comment appears to be untrue. - st := cluster.MakeTestingClusterSettings() - logcrash.DiagnosticsReportingEnabled.Override(ctx, &st.SV, false) - logcrash.CrashReports.Override(ctx, &st.SV, false) os.Exit(m.Run()) }