Address review comments

coder · ammario · Sep 1, 2022 · Aug 29, 2022 · Aug 29, 2022 · Sep 1, 2022
commit 52c9d103f4e419bf85aec77207fdcda7b7dec344
diff --git a/agent/agent.go b/agent/agent.go
@@ -201,6 +201,7 @@ func (a *agent) runTailnet(ctx context.Context, derpMap *tailcfg.DERPMap) {
 	}
 	a.network.SetForwardTCPCallback(func(conn net.Conn, listenerExists bool) net.Conn {
 		if listenerExists {
+			// If a listener already exists, we would double-wrap the conn.
 			return conn
 		}
 		return a.stats.wrapConn(conn)

diff --git a/cli/server.go b/cli/server.go
@@ -121,7 +121,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
 		spooky                           bool
 		verbose                          bool
 		metricsCacheRefreshInterval      time.Duration
-		agentStatReportInterval          time.Duration
+		agentStatRefreshInterval         time.Duration
 	)
 
 	root := &cobra.Command{
@@ -363,7 +363,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
 				Telemetry:                   telemetry.NewNoop(),
 				AutoImportTemplates:         validatedAutoImportTemplates,
 				MetricsCacheRefreshInterval: metricsCacheRefreshInterval,
-				AgentStatsReportInterval:    agentStatReportInterval,
+				AgentStatsRefreshInterval:   agentStatRefreshInterval,
 			}
 
 			if oauth2GithubClientSecret != "" {
@@ -843,7 +843,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
 
 	cliflag.DurationVarP(root.Flags(), &metricsCacheRefreshInterval, "metrics-cache-refresh-interval", "", "CODER_METRICS_CACHE_REFRESH_INTERVAL", time.Hour, "How frequently metrics are refreshed")
 	_ = root.Flags().MarkHidden("metrics-cache-refresh-interval")
-	cliflag.DurationVarP(root.Flags(), &agentStatReportInterval, "agent-stats-report-interval", "", "CODER_AGENT_STATS_REPORT_INTERVAL", time.Minute*10, "How frequently agent stats are recorded")
+	cliflag.DurationVarP(root.Flags(), &agentStatRefreshInterval, "agent-stats-refresh-interval", "", "CODER_AGENT_STATS_REFRESH_INTERVAL", time.Minute*10, "How frequently agent stats are recorded")
 	_ = root.Flags().MarkHidden("agent-stats-report-interval")
 
 	return root

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -80,7 +80,7 @@ type Options struct {
 
 	// Metrics related intervals.
 	MetricsCacheRefreshInterval time.Duration
-	AgentStatsReportInterval    time.Duration
+	AgentStatsRefreshInterval   time.Duration
 }
 
 // New constructs a Coder API handler.
@@ -378,7 +378,7 @@ func New(options *Options) *API {
 				r.Use(
 					apiKeyMiddleware,
 				)
-				r.Get("/daus", api.daus)
+				r.Get("/daus", api.metricsDAUs)
 			})
 		})
 		r.Route("/workspaceagents", func(r chi.Router) {

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -236,7 +236,7 @@ func newWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c
 		},
 		AutoImportTemplates:         options.AutoImportTemplates,
 		MetricsCacheRefreshInterval: time.Millisecond * 100,
-		AgentStatsReportInterval:    time.Millisecond * 100,
+		AgentStatsRefreshInterval:   time.Millisecond * 100,
 	})
 	t.Cleanup(func() {
 		_ = coderAPI.Close()

diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -137,7 +137,7 @@ func (q *fakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu
 	}
 	return database.ProvisionerJob{}, sql.ErrNoRows
 }
-func (q *fakeQuerier) DeleteOldAgentStats(_ context.Context) error {
+func (_ *fakeQuerier) DeleteOldAgentStats(_ context.Context) error {
 	// no-op
 	return nil
 }

diff --git a/coderd/metrics.go b/coderd/metrics.go
@@ -18,7 +18,7 @@ import (
 	"github.com/coder/coder/codersdk"
 )
 
-func (api *API) daus(rw http.ResponseWriter, r *http.Request) {
+func (api *API) metricsDAUs(rw http.ResponseWriter, r *http.Request) {
 	if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) {
 		httpapi.Forbidden(rw)
 		return
@@ -78,7 +78,7 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
 
 	// Allow overriding the stat interval for debugging and testing purposes.
 	ctx := r.Context()
-	timer := time.NewTicker(api.AgentStatsReportInterval)
+	timer := time.NewTicker(api.AgentStatsRefreshInterval)
 	var lastReport codersdk.AgentStatsReportResponse
 	for {
 		err := wsjson.Write(ctx, conn, codersdk.AgentStatsReportRequest{})
@@ -113,7 +113,7 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
 		var insert = !reflect.DeepEqual(lastReport, rep)
 
 		api.Logger.Debug(ctx, "read stats report",
-			slog.F("interval", api.AgentStatsReportInterval),
+			slog.F("interval", api.AgentStatsRefreshInterval),
 			slog.F("agent", workspaceAgent.ID),
 			slog.F("resource", resource.ID),
 			slog.F("workspace", workspace.ID),

diff --git a/coderd/metrics_test.go b/coderd/metrics_test.go
@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"cdr.dev/slog/sloggers/slogtest"
@@ -95,20 +96,22 @@ func TestMetrics(t *testing.T) {
 	_, err = session.Output("echo hello")
 	require.NoError(t, err)
 
-	// Give enough time for stats to hit DB
-	// and metrics cache to refresh.
-	time.Sleep(time.Second * 5)
-
-	daus, err = client.GetDAUsFromAgentStats(context.Background())
-	require.NoError(t, err)
-
-	require.Equal(t, &codersdk.DAUsResponse{
+	want := &codersdk.DAUsResponse{
 		Entries: []codersdk.DAUEntry{
 			{
 
 				Date: time.Now().UTC().Truncate(time.Hour * 24),
 				DAUs: 1,
 			},
 		},
-	}, daus)
+	}
+	require.Eventuallyf(t, func() bool {
+		daus, err = client.GetDAUsFromAgentStats(context.Background())
+		require.NoError(t, err)
+
+		return assert.ObjectsAreEqual(want, daus)
+	},
+		testutil.WaitShort, testutil.IntervalFast,
+		"got %+v != %+v", daus, want,
+	)
 }
diff --git a/coderd/metricscache/metricscache.go b/coderd/metricscache/metricscache.go
@@ -13,8 +13,8 @@ import (
 	"github.com/coder/retry"
 )
 
-// Cache holds the DAU cache and, later, the
-// user activity cache. The aggregation queries responsible for these values
+// Cache holds the DAU cache.
+// The aggregation queries responsible for these values
 // can take up to a minute on large deployments, but the cache has near zero
 // effect on most deployments.
 type Cache struct {
@@ -110,6 +110,9 @@ func (c *Cache) run(ctx context.Context) {
 			start := time.Now()
 			err := c.refresh(ctx)
 			if err != nil {
+				if ctx.Err() != nil {
+					return
+				}
 				c.log.Error(ctx, "refresh", slog.Error(err))
 				continue
 			}

diff --git a/coderd/metricscache/metricscache_test.go b/coderd/metricscache/metricscache_test.go
@@ -7,12 +7,14 @@ import (
 	"time"
 
 	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
 
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/coderd/database"
 	"github.com/coder/coder/coderd/database/databasefake"
 	"github.com/coder/coder/coderd/metricscache"
 	"github.com/coder/coder/codersdk"
+	"github.com/coder/coder/testutil"
 )
 
 func date(year, month, day int) time.Time {
@@ -164,12 +166,14 @@ func TestCache(t *testing.T) {
 				db.InsertAgentStat(context.Background(), row)
 			}
 
-			// Wait for value to refresh..
-			time.Sleep(time.Second)
+			var got codersdk.DAUsResponse
 
-			if got := cache.DAUs(); !reflect.DeepEqual(got.Entries, tt.want) {
-				t.Errorf("GetDAUs() = %v, want %v", got, tt.want)
-			}
+			require.Eventuallyf(t, func() bool {
+				got = cache.DAUs()
+				return reflect.DeepEqual(got.Entries, tt.want)
+			}, testutil.WaitShort, testutil.IntervalFast,
+				"GetDAUs() = %v, want %v", got, tt.want,
+			)
 		})
 	}
 }
diff --git a/codersdk/client.go b/codersdk/client.go
@@ -218,3 +218,9 @@ func (e *Error) Error() string {
 	}
 	return builder.String()
 }
+
+type CloseFunc func() error
+
+func (c CloseFunc) Close() error {
+	return c()
+}
diff --git a/codersdk/metrics.go b/codersdk/metrics.go
@@ -17,12 +17,6 @@ import (
 	"github.com/coder/retry"
 )
 
-type CloseFunc func() error
-
-func (c CloseFunc) Close() error {
-	return c()
-}
-
 // AgentReportStats begins a stat streaming connection with the Coder server.
 // It is resilient to network failures and intermittent coderd issues.
 func (c *Client) AgentReportStats(