Skip to content

Commit 4b285fa

Browse files
ammarioammario
committed
Address review comments
1 parent bca9928 commit 4b285fa

File tree

11 files changed

+43
-32
lines changed

11 files changed

+43
-32
lines changed

‎agent/agent.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,7 @@ func (a *agent) runTailnet(ctx context.Context, derpMap *tailcfg.DERPMap) {
201201
}
202202
a.network.SetForwardTCPCallback(func(conn net.Conn, listenerExists bool) net.Conn {
203203
if listenerExists {
204+
// If a listener already exists, we would double-wrap the conn.
204205
return conn
205206
}
206207
return a.stats.wrapConn(conn)

‎cli/server.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
121121
spooky bool
122122
verbose bool
123123
metricsCacheRefreshInterval time.Duration
124-
agentStatReportInterval time.Duration
124+
agentStatRefreshInterval time.Duration
125125
)
126126

127127
root := &cobra.Command{
@@ -363,7 +363,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
363363
Telemetry: telemetry.NewNoop(),
364364
AutoImportTemplates: validatedAutoImportTemplates,
365365
MetricsCacheRefreshInterval: metricsCacheRefreshInterval,
366-
AgentStatsReportInterval: agentStatReportInterval,
366+
AgentStatsRefreshInterval: agentStatRefreshInterval,
367367
}
368368

369369
if oauth2GithubClientSecret != "" {
@@ -843,7 +843,7 @@ func Server(newAPI func(*coderd.Options) *coderd.API) *cobra.Command {
843843

844844
cliflag.DurationVarP(root.Flags(), &metricsCacheRefreshInterval, "metrics-cache-refresh-interval", "", "CODER_METRICS_CACHE_REFRESH_INTERVAL", time.Hour, "How frequently metrics are refreshed")
845845
_ = root.Flags().MarkHidden("metrics-cache-refresh-interval")
846-
cliflag.DurationVarP(root.Flags(), &agentStatReportInterval, "agent-stats-report-interval", "", "CODER_AGENT_STATS_REPORT_INTERVAL", time.Minute*10, "How frequently agent stats are recorded")
846+
cliflag.DurationVarP(root.Flags(), &agentStatRefreshInterval, "agent-stats-refresh-interval", "", "CODER_AGENT_STATS_REFRESH_INTERVAL", time.Minute*10, "How frequently agent stats are recorded")
847847
_ = root.Flags().MarkHidden("agent-stats-report-interval")
848848

849849
return root

‎coderd/coderd.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ type Options struct {
8080

8181
// Metrics related intervals.
8282
MetricsCacheRefreshInterval time.Duration
83-
AgentStatsReportInterval time.Duration
83+
AgentStatsRefreshInterval time.Duration
8484
}
8585

8686
// New constructs a Coder API handler.
@@ -372,7 +372,7 @@ func New(options *Options) *API {
372372
r.Use(
373373
apiKeyMiddleware,
374374
)
375-
r.Get("/daus", api.daus)
375+
r.Get("/daus", api.metricsDAUs)
376376
})
377377
})
378378
r.Route("/workspaceagents", func(r chi.Router) {

‎coderd/coderdtest/coderdtest.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ func newWithAPI(t *testing.T, options *Options) (*codersdk.Client, io.Closer, *c
236236
},
237237
AutoImportTemplates: options.AutoImportTemplates,
238238
MetricsCacheRefreshInterval: time.Millisecond * 100,
239-
AgentStatsReportInterval: time.Millisecond * 100,
239+
AgentStatsRefreshInterval: time.Millisecond * 100,
240240
})
241241
t.Cleanup(func() {
242242
_ = coderAPI.Close()

‎coderd/database/databasefake/databasefake.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ func (q *fakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu
137137
}
138138
return database.ProvisionerJob{}, sql.ErrNoRows
139139
}
140-
func (q *fakeQuerier) DeleteOldAgentStats(_ context.Context) error {
140+
func (_ *fakeQuerier) DeleteOldAgentStats(_ context.Context) error {
141141
// no-op
142142
return nil
143143
}

‎coderd/metrics.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818
"github.com/coder/coder/codersdk"
1919
)
2020

21-
func (api *API) daus(rw http.ResponseWriter, r *http.Request) {
21+
func (api *API) metricsDAUs(rw http.ResponseWriter, r *http.Request) {
2222
if !api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) {
2323
httpapi.Forbidden(rw)
2424
return
@@ -78,7 +78,7 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
7878

7979
// Allow overriding the stat interval for debugging and testing purposes.
8080
ctx := r.Context()
81-
timer := time.NewTicker(api.AgentStatsReportInterval)
81+
timer := time.NewTicker(api.AgentStatsRefreshInterval)
8282
var lastReport codersdk.AgentStatsReportResponse
8383
for {
8484
err := wsjson.Write(ctx, conn, codersdk.AgentStatsReportRequest{})
@@ -113,7 +113,7 @@ func (api *API) workspaceAgentReportStats(rw http.ResponseWriter, r *http.Reques
113113
var insert = !reflect.DeepEqual(lastReport, rep)
114114

115115
api.Logger.Debug(ctx, "read stats report",
116-
slog.F("interval", api.AgentStatsReportInterval),
116+
slog.F("interval", api.AgentStatsRefreshInterval),
117117
slog.F("agent", workspaceAgent.ID),
118118
slog.F("resource", resource.ID),
119119
slog.F("workspace", workspace.ID),

‎coderd/metrics_test.go

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"time"
77

88
"github.com/google/uuid"
9+
"github.com/stretchr/testify/assert"
910
"github.com/stretchr/testify/require"
1011

1112
"cdr.dev/slog/sloggers/slogtest"
@@ -95,20 +96,22 @@ func TestMetrics(t *testing.T) {
9596
_, err = session.Output("echo hello")
9697
require.NoError(t, err)
9798

98-
// Give enough time for stats to hit DB
99-
// and metrics cache to refresh.
100-
time.Sleep(time.Second * 5)
101-
102-
daus, err = client.GetDAUsFromAgentStats(context.Background())
103-
require.NoError(t, err)
104-
105-
require.Equal(t, &codersdk.DAUsResponse{
99+
want := &codersdk.DAUsResponse{
106100
Entries: []codersdk.DAUEntry{
107101
{
108102

109103
Date: time.Now().UTC().Truncate(time.Hour * 24),
110104
DAUs: 1,
111105
},
112106
},
113-
}, daus)
107+
}
108+
require.Eventuallyf(t, func() bool {
109+
daus, err = client.GetDAUsFromAgentStats(context.Background())
110+
require.NoError(t, err)
111+
112+
return assert.ObjectsAreEqual(want, daus)
113+
},
114+
testutil.WaitShort, testutil.IntervalFast,
115+
"got %+v != %+v", daus, want,
116+
)
114117
}

‎coderd/metricscache/metricscache.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ import (
1313
"github.com/coder/retry"
1414
)
1515

16-
// Cache holds the DAU cache and, later, the
17-
// user activity cache. The aggregation queries responsible for these values
16+
// Cache holds the DAU cache.
17+
// The aggregation queries responsible for these values
1818
// can take up to a minute on large deployments, but the cache has near zero
1919
// effect on most deployments.
2020
type Cache struct {
@@ -110,6 +110,9 @@ func (c *Cache) run(ctx context.Context) {
110110
start := time.Now()
111111
err := c.refresh(ctx)
112112
if err != nil {
113+
if ctx.Err() != nil {
114+
return
115+
}
113116
c.log.Error(ctx, "refresh", slog.Error(err))
114117
continue
115118
}

‎coderd/metricscache/metricscache_test.go

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,14 @@ import (
77
"time"
88

99
"github.com/google/uuid"
10+
"github.com/stretchr/testify/require"
1011

1112
"cdr.dev/slog/sloggers/slogtest"
1213
"github.com/coder/coder/coderd/database"
1314
"github.com/coder/coder/coderd/database/databasefake"
1415
"github.com/coder/coder/coderd/metricscache"
1516
"github.com/coder/coder/codersdk"
17+
"github.com/coder/coder/testutil"
1618
)
1719

1820
func date(year, month, day int) time.Time {
@@ -164,12 +166,14 @@ func TestCache(t *testing.T) {
164166
db.InsertAgentStat(context.Background(), row)
165167
}
166168

167-
// Wait for value to refresh..
168-
time.Sleep(time.Second)
169+
var got codersdk.DAUsResponse
169170

170-
if got := cache.DAUs(); !reflect.DeepEqual(got.Entries, tt.want) {
171-
t.Errorf("GetDAUs() = %v, want %v", got, tt.want)
172-
}
171+
require.Eventuallyf(t, func() bool {
172+
got = cache.DAUs()
173+
return reflect.DeepEqual(got.Entries, tt.want)
174+
}, testutil.WaitShort, testutil.IntervalFast,
175+
"GetDAUs() = %v, want %v", got, tt.want,
176+
)
173177
})
174178
}
175179
}

‎codersdk/client.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,3 +218,9 @@ func (e *Error) Error() string {
218218
}
219219
return builder.String()
220220
}
221+
222+
type CloseFunc func() error
223+
224+
func (c CloseFunc) Close() error {
225+
return c()
226+
}

‎codersdk/metrics.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,6 @@ import (
1717
"github.com/coder/retry"
1818
)
1919

20-
type CloseFunc func() error
21-
22-
func (c CloseFunc) Close() error {
23-
return c()
24-
}
25-
2620
// AgentReportStats begins a stat streaming connection with the Coder server.
2721
// It is resilient to network failures and intermittent coderd issues.
2822
func (c *Client) AgentReportStats(

0 commit comments

Comments
 (0)