internal/frontend: remove dependency on cloud error reporting client

This removes the dependency of internal/frontend on the cloud error reporting client, both directly, and through the derrors package by introducing a new interface Reporter that is used both to set the reporting client for internal/derrors, and on the Server. For golang/go#61399 Change-Id: Id4d4def522cda9b4e49f53cff6708019dec2693c Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/514676 Reviewed-by: Jamal Carvalho <[email protected]> kokoro-CI: kokoro <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Michael Matloob <[email protected]>
kd-collective · Aug 4, 2023 · 52eb228 · 52eb228
1 parent 1e93998
commit 52eb228
Show file tree

Hide file tree

Showing 9 changed files with 89 additions and 92 deletions.
diff --git a/cmd/frontend/main.go b/cmd/frontend/main.go
@@ -107,7 +107,7 @@ func main() {
 		}
 	}
 
-	rc := cmdconfig.ReportingClient(ctx, cfg)
+	reporter := cmdconfig.Reporter(ctx, cfg)
 	vc, err := vuln.NewClient(cfg.VulnDB)
 	if err != nil {
 		log.Fatalf(ctx, "vuln.NewClient: %v", err)
@@ -124,7 +124,7 @@ func main() {
 		ThirdPartyFS:         os.DirFS(*thirdPartyPath),
 		DevMode:              *devMode,
 		LocalMode:            *localMode,
-		ReportingClient:      rc,
+		Reporter:             reporter,
 		VulndbClient:         vc,
 	})
 	if err != nil {
@@ -174,12 +174,12 @@ func main() {
 		log.Fatal(ctx, err)
 	}
 	log.Infof(ctx, "cmd/frontend: initializing cmdconfig.Experimenter")
-	experimenter := cmdconfig.Experimenter(ctx, cfg, expg, rc)
+	experimenter := cmdconfig.Experimenter(ctx, cfg, expg, reporter)
 	log.Infof(ctx, "cmd/frontend: initialized cmdconfig.Experimenter")
 
 	ermw := middleware.Identity()
-	if rc != nil {
-		ermw = middleware.ErrorReporting(rc.Report)
+	if reporter != nil {
+		ermw = middleware.ErrorReporting(reporter)
 	}
 	mw := middleware.Chain(
 		middleware.RequestLog(cmdconfig.Logger(ctx, cfg, "frontend-log")),

diff --git a/cmd/internal/cmdconfig/cmdconfig.go b/cmd/internal/cmdconfig/cmdconfig.go
@@ -8,6 +8,7 @@ package cmdconfig
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"strings"
 	"time"
 
@@ -44,12 +45,12 @@ func Logger(ctx context.Context, cfg *config.Config, logName string) middleware.
 	return middleware.LocalLogger{}
 }
 
-// ReportingClient configures an Error Reporting client.
-func ReportingClient(ctx context.Context, cfg *config.Config) *errorreporting.Client {
+// Reporter configures an Error Reporting client.
+func Reporter(ctx context.Context, cfg *config.Config) derrors.Reporter {
 	if !cfg.OnGCP() || cfg.DisableErrorReporting {
 		return nil
 	}
-	reporter, err := errorreporting.NewClient(ctx, cfg.ProjectID, errorreporting.Config{
+	reportingClient, err := errorreporting.NewClient(ctx, cfg.ProjectID, errorreporting.Config{
 		ServiceName: cfg.ServiceID,
 		OnError: func(err error) {
 			log.Errorf(ctx, "Error reporting failed: %v", err)
@@ -58,13 +59,22 @@ func ReportingClient(ctx context.Context, cfg *config.Config) *errorreporting.Cl
 	if err != nil {
 		log.Fatal(ctx, err)
 	}
-	derrors.SetReportingClient(reporter)
+	reporter := &reporter{reportingClient}
+	derrors.SetReporter(reporter)
 	return reporter
 }
 
+type reporter struct {
+	c *errorreporting.Client
+}
+
+func (r *reporter) Report(err error, req *http.Request, stack []byte) {
+	r.c.Report(errorreporting.Entry{Error: err, Req: req, Stack: stack})
+}
+
 // Experimenter configures a middleware.Experimenter.
-func Experimenter(ctx context.Context, cfg *config.Config, getter middleware.ExperimentGetter, reportingClient *errorreporting.Client) *middleware.Experimenter {
-	e, err := middleware.NewExperimenter(ctx, 1*time.Minute, getter, reportingClient)
+func Experimenter(ctx context.Context, cfg *config.Config, getter middleware.ExperimentGetter, reporter derrors.Reporter) *middleware.Experimenter {
+	e, err := middleware.NewExperimenter(ctx, 1*time.Minute, getter, reporter)
 	if err != nil {
 		log.Fatal(ctx, err)
 	}

diff --git a/cmd/worker/main.go b/cmd/worker/main.go
@@ -90,10 +90,10 @@ func main() {
 		log.Fatalf(ctx, "gcpqueue.New: %v", err)
 	}
 
-	reportingClient := cmdconfig.ReportingClient(ctx, cfg)
+	reporter := cmdconfig.Reporter(ctx, cfg)
 	redisCacheClient := getCacheRedis(ctx, cfg)
 	redisBetaCacheClient := getBetaCacheRedis(ctx, cfg)
-	experimenter := cmdconfig.Experimenter(ctx, cfg, expg, reportingClient)
+	experimenter := cmdconfig.Experimenter(ctx, cfg, expg, reporter)
 	server, err := worker.NewServer(cfg, worker.ServerConfig{
 		DB:                   db,
 		IndexClient:          indexClient,
@@ -102,7 +102,7 @@ func main() {
 		RedisCacheClient:     redisCacheClient,
 		RedisBetaCacheClient: redisBetaCacheClient,
 		Queue:                fetchQueue,
-		ReportingClient:      reportingClient,
+		Reporter:             reporter,
 		StaticPath:           template.TrustedSourceFromFlag(flag.Lookup("static").Value),
 		GetExperiments:       experimenter.Experiments,
 	})

diff --git a/internal/derrors/derrors.go b/internal/derrors/derrors.go
@@ -11,8 +11,6 @@ import (
 	"fmt"
 	"net/http"
 	"runtime"
-
-	"cloud.google.com/go/errorreporting"
 )
 
 //lint:file-ignore ST1012 prefixing error values with Err would stutter
@@ -284,16 +282,21 @@ func WrapAndReport(errp *error, format string, args ...any) {
 	}
 }
 
-var repClient *errorreporting.Client
+var reporter Reporter
+
+// SetReporter the Reporter to use, for use by Report.
+func SetReporter(r Reporter) {
+	reporter = r
+}
 
-// SetReportingClient sets an errorreporting client, for use by Report.
-func SetReportingClient(c *errorreporting.Client) {
-	repClient = c
+// Reporter is an interface used for reporting errors.
+type Reporter interface {
+	Report(err error, req *http.Request, stack []byte)
 }
 
-// Report uses the errorreporting API to report an error.
+// Report uses the Reporter to report an error.
 func Report(err error) {
-	if repClient != nil {
-		repClient.Report(errorreporting.Entry{Error: err})
+	if reporter != nil {
+		reporter.Report(err, nil, nil)
 	}
 }
diff --git a/internal/frontend/server.go b/internal/frontend/server.go
@@ -21,7 +21,6 @@ import (
 	"sync"
 	"time"
 
-	"cloud.google.com/go/errorreporting"
 	"github.com/google/safehtml"
 	"github.com/google/safehtml/template"
 	"golang.org/x/pkgsite/internal"
@@ -58,7 +57,7 @@ type Server struct {
 	appVersionLabel      string
 	googleTagManagerID   string
 	serveStats           bool
-	reportingClient      *errorreporting.Client
+	reporter             derrors.Reporter
 	fileMux              *http.ServeMux
 	vulnClient           *vuln.Client
 	versionID            string
@@ -83,7 +82,7 @@ type ServerConfig struct {
 	LocalMode            bool
 	LocalModules         []LocalModule
 	StaticPath           string // used only for dynamic loading in dev mode
-	ReportingClient      *errorreporting.Client
+	Reporter             derrors.Reporter
 	VulndbClient         *vuln.Client
 }
 
@@ -107,7 +106,7 @@ func NewServer(scfg ServerConfig) (_ *Server, err error) {
 		staticPath:           scfg.StaticPath,
 		templates:            ts,
 		taskIDChangeInterval: scfg.TaskIDChangeInterval,
-		reportingClient:      scfg.ReportingClient,
+		reporter:             scfg.Reporter,
 		fileMux:              http.NewServeMux(),
 		vulnClient:           scfg.VulndbClient,
 	}
@@ -588,19 +587,15 @@ func (s *Server) serveError(w http.ResponseWriter, r *http.Request, err error) {
 
 // reportError sends the error to the GCP Error Reporting service.
 func (s *Server) reportError(ctx context.Context, err error, w http.ResponseWriter, r *http.Request) {
-	if s.reportingClient == nil {
+	if s.reporter == nil {
 		return
 	}
 	// Extract the stack trace from the error if there is one.
 	var stack []byte
 	if serr := (*derrors.StackError)(nil); errors.As(err, &serr) {
 		stack = serr.Stack
 	}
-	s.reportingClient.Report(errorreporting.Entry{
-		Error: err,
-		Req:   r,
-		Stack: stack,
-	})
+	s.reporter.Report(err, r, stack)
 	log.Debugf(ctx, "reported error %v with stack size %d", err, len(stack))
 	// Bypass the error-reporting middleware.
 	w.Header().Set(config.BypassErrorReportingHeader, "true")

diff --git a/internal/middleware/errorreporting.go b/internal/middleware/errorreporting.go
@@ -8,14 +8,13 @@ import (
 	"fmt"
 	"net/http"
 
-	"cloud.google.com/go/errorreporting"
 	"golang.org/x/pkgsite/internal/config"
 	"golang.org/x/pkgsite/internal/derrors"
 )
 
 // ErrorReporting returns a middleware that reports any server errors using the
 // report func.
-func ErrorReporting(report func(errorreporting.Entry)) Middleware {
+func ErrorReporting(reporter derrors.Reporter) Middleware {
 	return func(h http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			w2 := &erResponseWriter{ResponseWriter: w}
@@ -44,10 +43,8 @@ func ErrorReporting(report func(errorreporting.Entry)) Middleware {
 			if w2.status == derrors.ToStatus(derrors.VulnDBError) {
 				return
 			}
-			report(errorreporting.Entry{
-				Error: fmt.Errorf("handler for %q returned status code %d", r.URL.Path, w2.status),
-				Req:   r,
-			})
+			reporter.Report(
+				fmt.Errorf("handler for %q returned status code %d", r.URL.Path, w2.status), r, nil)
 		})
 	}
 }

diff --git a/internal/middleware/errorreporting_test.go b/internal/middleware/errorreporting_test.go
@@ -10,7 +10,6 @@ import (
 	"net/http/httptest"
 	"testing"
 
-	"cloud.google.com/go/errorreporting"
 	"golang.org/x/pkgsite/internal/config"
 )
 
@@ -37,19 +36,25 @@ func TestErrorReporting(t *testing.T) {
 				}
 				w.WriteHeader(test.code)
 			})
-			reports := 0
-			mw := ErrorReporting(func(errorreporting.Entry) {
-				reports++
-			})
+			fr := &fakeReporter{}
+			mw := ErrorReporting(fr)
 			ts := httptest.NewServer(mw(handler))
 			resp, err := http.Get(ts.URL)
 			if err != nil {
 				t.Fatal(err)
 			}
 			resp.Body.Close()
-			if got := reports; got != test.wantReports {
+			if got := fr.reports; got != test.wantReports {
 				t.Errorf("Got %d reports, want %d", got, test.wantReports)
 			}
 		})
 	}
 }
+
+type fakeReporter struct {
+	reports int
+}
+
+func (f *fakeReporter) Report(error, *http.Request, []byte) {
+	f.reports++
+}
diff --git a/internal/middleware/experiment.go b/internal/middleware/experiment.go
@@ -11,7 +11,6 @@ import (
 	"net/http"
 	"time"
 
-	"cloud.google.com/go/errorreporting"
 	"golang.org/x/pkgsite/internal"
 	"golang.org/x/pkgsite/internal/derrors"
 	"golang.org/x/pkgsite/internal/experiment"
@@ -21,11 +20,6 @@ import (
 
 const experimentQueryParamKey = "experiment"
 
-// A Reporter sends errors to the Error-Reporting service.
-type Reporter interface {
-	Report(errorreporting.Entry)
-}
-
 // ExperimentGetter is the signature of a function that gets experiments.
 type ExperimentGetter func(context.Context) ([]*internal.Experiment, error)
 
@@ -37,7 +31,7 @@ type Experimenter struct {
 
 // NewExperimenter returns an Experimenter for use in the middleware. The
 // experimenter regularly polls for updates to the snapshot in the background.
-func NewExperimenter(ctx context.Context, pollEvery time.Duration, getter ExperimentGetter, rep Reporter) (_ *Experimenter, err error) {
+func NewExperimenter(ctx context.Context, pollEvery time.Duration, getter ExperimentGetter, rep derrors.Reporter) (_ *Experimenter, err error) {
 	defer derrors.Wrap(&err, "middleware.NewExperimenter")
 
 	initial, err := getter(ctx)
@@ -55,9 +49,7 @@ func NewExperimenter(ctx context.Context, pollEvery time.Duration, getter Experi
 				// Log and report // the error.
 				log.Error(ctx, err)
 				if rep != nil {
-					rep.Report(errorreporting.Entry{
-						Error: fmt.Errorf("loading experiments: %v", err),
-					})
+					rep.Report(fmt.Errorf("loading experiments: %v", err), nil, nil)
 				}
 			}),
 	}