Daily Active User Metrics #3735

ammario · 2022-08-29T18:56:27Z

User facing:

Add API for retrieving stats
Add Metrics view to Users page heading Active user metrics 📊 #3633
Add zero state for DAU chart.
Fill in empty dates in backend (0 DAUs)
Replace interval environment variables with some other approach to debug configuration that is less likely to be abused by a customer. Perhaps only parse these debug flags when CODER_EXPERIMENTAL is true.
Move time series into per-template presentation

Plumbing:

ammario · 2022-08-30T05:55:59Z

@kylecarbs most of the backend is complete. I plan on adding the frontend tomorrow.

Emyrk

Putting this in the agent solves a lot of the issues we had with v1 👍.

This does only catch long lived connections. Like I would assume most ProtocolDials will be short http requests, right? Those would never likely be picked up.

coderd/database/migrations/000039_agent_stats.up.sql

codersdk/metrics.go

Emyrk · 2022-08-30T20:23:04Z

codersdk/metrics.go

+						return err
+					}
+
+					s := stats()


I am guessing this returns a threadsafe copy for the later json marshal. Would be nice if this copy didn't have a mutex in it. Can be misleading to see a mutex, and might assume to use it.

coderd/metrics.go

socket-security · 2022-09-01T01:58:39Z

Socket Security Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 New install scripts detected

A dependency change in this PR is introducing new install scripts to your install step.

Package	Script field	Location
canvas@2.9.3 (added)	`binding.gyp`	`site/package.json`
canvas@2.9.3 (added)	`install`	`site/package.json`
core-js@3.25.0 (upgraded)	`postinstall`	`site/package.json` via @storybook/addon-actions@6.5.9
core-js-pure@3.25.0 (upgraded)	`postinstall`	`site/package.json` via @pmmmwh/react-refresh-webpack-plugin@0.5.7

🫣 Native code

Contains native code which could be a vector to obscure malicious code, and generally decrease the likelihood of reproducible or reliable installs.

Package	Location
canvas@2.9.3 (added)	`site/package.json`

Socket.dev scan summary

Issue	Status
Did you mean?	✅ no new possible package typos
Install scripts	⚠️ 4 new install scripts detected
Telemetry	✅ no new telemetry
Troll package	✅ no new troll packages
Malware	✅ no new malware
Native code	⚠️ 1 new native module detected

Powered by socket.dev

agent/agent.go

agent/stats.go

agent/agent.go

coderd/coderd.go

coderd/metrics_test.go

coderd/metricscache/metricscache.go

coderd/metricscache/metricscache_test.go

codersdk/metrics.go

coderd/database/migrations/000042_agent_stats.up.sql

jsjoeio

FE: Didn't review the whole thing yet but can later today. So far looking good, just some housekeeping stuff mostly that we should address

site/package.json

jsjoeio · 2022-09-01T16:22:43Z

site/package.json

@@ -113,6 +117,7 @@
    "prettier": "2.7.1",
    "prettier-plugin-organize-imports": "3.0.0",
    "react-hot-loader": "4.13.0",
+    "resize-observer": "^1.0.4",


Could we update the PR description explaining the new libraries added and the reason behind them? That'll help the rest of the team aware 👍🏼

I shall explain in the comments.

Ugh... can't comment package.json...

PR description works well too

site/package.json

site/src/api/typesGenerated.ts

jsjoeio · 2022-09-01T16:24:56Z

site/src/api/typesGenerated.ts

+// From codersdk/metrics.go
+export interface DAUEntry {
+  readonly date: string
+  readonly daus: number


nit: DAUEntry.daus feels a bit weird. Would also prefer for us to use consistent casing if possible: DAU vs. dau

Maybe total?

The casing is an unfortunate side effect of the code generator, which is outside the scope of this PR. I can rename to total if you feel strongly, but I think daus is clear enough.

jsjoeio · 2022-09-01T16:26:00Z

site/src/pages/TemplatePage/DAUCharts.test.tsx

+Object.defineProperty(window, "ResizeObserver", {
+  value: ResizeObserver,
+})


Why are we adding this to the window? Feels like a codesmell but I blame my lack of context and understanding here.

If this is needed, could we add it in a beforeEach or beforeAll block inside your describe block?

We also need to remove it from the window or it could affect other tests so it should be removed in a teardown function like afterAll.

Unfortunately, there's no way to do this w/o modifying the global state. Since jest runs parallel, even if we use beforeAll I believe we may have state issues.

jsjoeio · 2022-09-01T16:52:10Z

site/src/pages/TemplatePage/DAUCharts.tsx

+export const Language = {
+  loadingText: "DAU stats are loading. Check back later.",
+  chartTitle: "Daily Active Users",
+}


I think now that we have internationalization, we're not using this pattern anymore and instead using the t() and then referencing strings from a json file. There should be some examples in the codebase but if you want specifics, let me know!

Slightly off-topic, but why are we pursuing internationalization? That seems like a feature that is 3+ years out.

Hey @ammario I was on vacation when you asked this, but I wanted to follow up. There are a couple reasons we're using this pattern, and some are outlined below:

It's helpful to have all strings colocated. We can be better about using consistent language, and we can re-use strings more easily.

Translation libraries have features like string interpolation or pluralization built into them, which helps us avoid a bunch of boilerplate JSX, keeping our component files lightweight.

We've actually had an internationalization solution in our app since I started, but prior to using i18n, we were using Language objects which is a problematic pattern for several reasons -you can read more in our FE Variety notes if you're interested

Even if translation is a long way off, it's best to establish a pattern before the app grows large.

Anyway, let me know if you want to chat about it further!

kylecarbs

Take a look at my comments before merge, but overall LGTM!

agent/agent_test.go

agent/stats.go

kylecarbs · 2022-09-01T18:58:34Z

coderd/database/migrations/000042_agent_stats.up.sql

+    agent_id uuid NOT NULL,
+    workspace_id uuid NOT NULL,
+    template_id uuid NOT NULL,
+    payload jsonb NOT NULL


Payload is weird since this is a specific stat type. Why not structure the data?

I anticipate significant churn in this structure, and there can be many agent versions simultaneously reporting to the same coderd. I'm thinking about long-lived workspaces that never stop.

codersdk/templates.go

site/src/testHelpers/coder.code-workspace

This primarily resolves delete template conflicts.

Mostly addressed

jsjoeio · 2022-09-01T21:24:18Z

site/package.json

@@ -38,6 +38,7 @@
    "@xstate/react": "3.0.1",
    "axios": "0.26.1",
    "can-ndjson-stream": "1.0.2",
+    "chart.js": "^3.5.0",


Looks like we still have a ^ here

jsjoeio · 2022-09-01T21:24:49Z

site/package.json

    "react": "18.2.0",
+    "react-chartjs-2": "^4.3.1",


One here as well

jsjoeio · 2022-09-01T21:24:56Z

site/package.json

    "react-dom": "18.2.0",
    "react-helmet-async": "1.3.0",
    "react-i18next": "11.18.4",
    "react-markdown": "8.0.3",
-    "react-router-dom": "6.3.0",
+    "react-router-dom": "^6.3.0",


and here as well

jsjoeio · 2022-09-01T21:25:01Z

site/package.json

@@ -87,6 +89,7 @@
    "@typescript-eslint/eslint-plugin": "5.31.0",
    "@typescript-eslint/parser": "5.31.0",
    "@xstate/cli": "0.3.0",
+    "canvas": "^2.9.3",


jsjoeio · 2022-09-01T21:25:24Z

site/package.json

@@ -113,6 +117,7 @@
    "prettier": "2.7.1",
    "prettier-plugin-organize-imports": "3.0.0",
    "react-hot-loader": "4.13.0",
+    "resize-observer": "^1.0.4",


PR description works well too

jsjoeio · 2022-09-01T21:26:38Z

site/src/pages/TemplatePage/DAUChart.test.tsx

+// The Chart performs dynamic resizes which fail in tests without this.
+Object.defineProperty(window, "ResizeObserver", {
+  value: ResizeObserver,
+})


If this is needed for these tests, it's usually best to put it in a setup function like beforeEach or beforeAll. Otherwise it can have unintended side effects.

jsjoeio · 2022-09-01T21:28:42Z

site/src/pages/TemplatePage/DAUChart.tsx

+export const Language = {
+  loadingText: "DAU stats are loading. Check back later.",
+  chartTitle: "Daily Active Users",
+}


This is the old pattern. As mentioned before, the new pattern is to create a json file in here and then use the t function like so.

Not a huge deal but in the future we should shoot for consistency 👍🏼

jsjoeio · 2022-09-01T21:29:43Z

site/src/pages/TemplatePage/TemplatePage.test.tsx

+Object.defineProperty(window, "ResizeObserver", {
+  value: ResizeObserver,
+})
+


Similar comment as above, would be best to keep this in a setup and teardown function

ammario linked an issue Aug 29, 2022 that may be closed by this pull request

Active user metrics 📊 #3633

Closed

ammario requested a review from kylecarbs August 30, 2022 05:54

ammario force-pushed the metrics branch from be745d9 to 3b55d4b Compare August 30, 2022 05:55

Emyrk reviewed Aug 30, 2022

View reviewed changes

This was referenced Aug 30, 2022

Add hours used to each User row #3768

Closed

Show when workspace was last used #3669

Closed

ammario force-pushed the metrics branch 4 times, most recently from 8dbf511 to df5d596 Compare August 31, 2022 04:12

ammario mentioned this pull request Aug 31, 2022

Add an experimental mode that we can flag features on #3780

Closed

ammario force-pushed the metrics branch 4 times, most recently from 618cd92 to 7c1a63f Compare September 1, 2022 01:56

ammario changed the title ~~Metrics~~ Daily Active User Metrics Sep 1, 2022

kylecarbs requested changes Sep 1, 2022

View reviewed changes

ammario marked this pull request as ready for review September 1, 2022 02:30

ammario requested a review from a team as a code owner September 1, 2022 02:30

ammario requested review from jsjoeio and removed request for a team September 1, 2022 02:30

ammario force-pushed the metrics branch 3 times, most recently from d593346 to 4b285fa Compare September 1, 2022 03:51

coadler reviewed Sep 1, 2022

View reviewed changes

coderd/database/migrations/000042_agent_stats.up.sql Outdated Show resolved Hide resolved

jsjoeio previously requested changes Sep 1, 2022

View reviewed changes

ammario added 2 commits September 1, 2022 16:56

agent: add ConnStats

502aa52

agent: add StatsReporter

3893045

ammario added 10 commits September 1, 2022 16:59

Fix compilation error

e0295e0

Rename ConnStats to StatsConn

2f1a423

continues instead of returns

0a50cc9

Fix some test failures

7feab5e

Redo tests

a4d2cf7

Address review comments

52c9d10

REVAMP — backend tests work

3f9901e

Black triangle

7840509

Consolidate template state machines

39170cf

Move workspaceagent endpoint

eb373d6

ammario force-pushed the metrics branch from 96a7b46 to eb373d6 Compare September 1, 2022 17:00

ammario added 6 commits September 1, 2022 17:14

Address most review comments

a3d87b8

Improve contrast in chart

31ba0c6

Add more agent tests

5b906c1

Fix JS ci

49d9386

A bunch of minor touch ups

b14a077

Stabilize protoc

8da24c4

kylecarbs approved these changes Sep 1, 2022

View reviewed changes

ammario added 6 commits September 1, 2022 19:05

Merge remote-tracking branch 'origin/main' into metrics

00ec953

This primarily resolves delete template conflicts.

Update lockfile

4940319

Address comments + attempt to fix protoc

22b2028

fixup! Address comments + attempt to fix protoc

0157365

Try to fix protoc...

b166cdd

PROTO!

4201998

ammario enabled auto-merge (squash) September 1, 2022 19:53

ammario merged commit 30f8fd9 into main Sep 1, 2022

ammario deleted the metrics branch September 1, 2022 19:58

ammario mentioned this pull request Sep 1, 2022

Use Active Users instead of Total Users in Templates page #3815

Closed

jsjoeio reviewed Sep 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Daily Active User Metrics #3735

Daily Active User Metrics #3735

ammario commented Aug 29, 2022 •

edited

Loading

ammario commented Aug 30, 2022

Emyrk left a comment

Emyrk Aug 30, 2022

socket-security bot commented Sep 1, 2022 •

edited

Loading

jsjoeio left a comment

jsjoeio Sep 1, 2022

ammario Sep 1, 2022

ammario Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

ammario Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

ammario Sep 1, 2022

jsjoeio Sep 1, 2022

ammario Sep 1, 2022

Kira-Pilot Sep 16, 2022

kylecarbs left a comment

kylecarbs Sep 1, 2022

ammario Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

jsjoeio Sep 1, 2022

Daily Active User Metrics #3735

Daily Active User Metrics #3735

Conversation

ammario commented Aug 29, 2022 • edited Loading

ammario commented Aug 30, 2022

Emyrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socket-security bot commented Sep 1, 2022 • edited Loading

jsjoeio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylecarbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammario commented Aug 29, 2022 •

edited

Loading

socket-security bot commented Sep 1, 2022 •

edited

Loading