chore: add usage tracking package #19095

deansheather · 2025-07-30T06:02:37Z

Not used in coderd yet, see stack.

Adds two new packages:

coderd/usage: provides an interface for the "Collector" as well as a stub implementation for AGPL
enterprise/coderd/usage: provides an interface for the "Publisher" as well as a Tallyman implementation

Relates to coder/internal#814

deansheather · 2025-07-30T06:02:57Z

chore: move usage types to new package #19103
chore: wire up usage tracking for managed agents #19096
chore: add usage tracking package #19095 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

johnstcn

I have some non-blocking comments below but might need to take another pass.

coderd/database/dbauthz/dbauthz.go

coderd/database/queries/usageevents.sql

johnstcn · 2025-07-30T08:21:26Z

enterprise/coderd/usage/publisher.go

+const (
+	CoderLicenseJWTHeader = "Coder-License-JWT"
+
+	tallymanURL         = "https://tallyman-ingress.coder.com"


I can see an argument for making this a var so it's configurable at build time.

If we do need that we can add it very easily in the future.

johnstcn · 2025-07-30T08:27:55Z

enterprise/coderd/usage/publisher.go

+			if !allFailed {
+				// These are all going to have the same message, so don't log
+				// them. We already logged the overall error above.
+				p.log.Warn(ctx, "tallyman rejected usage event", slog.F("id", event.ID), slog.F("message", rejectedEvent.Message), slog.F("permanent", rejectedEvent.Permanent))


Should we instead collect the failed IDs and warn once? Depending on the number of events, this could spam.

johnstcn · 2025-07-30T08:28:22Z

enterprise/coderd/usage/publisher.go

+		} else {
+			// It's not good if this path gets hit, but we'll handle it as if it
+			// was a temporary rejection.
+			p.log.Warn(ctx, "tallyman did not include a usage event in the response, considering it temporarily rejected", slog.F("id", event.ID))


As above, I'd prefer if we didn't log in a tight loop.

johnstcn · 2025-07-30T08:29:03Z

enterprise/coderd/usage/publisher.go

+	err = p.db.UpdateUsageEventsPostPublish(ctx, dbUpdate)
+	if err != nil {


nit: suggest

if err := p.b.UpdateUsageEventsPostPublish(ctx, dbUpdate); err != nil { ... }

It's not enforced by the formatter and I prefer it to be two lines, so I'm going to keep it like this

johnstcn · 2025-07-30T08:34:40Z

coderd/database/migrations/000353_create_usage_events_table.up.sql

Obligatory reminder to check migration number before merging!

spikecurtis · 2025-07-30T07:58:53Z

CODEOWNERS

+
+# Usage tracking code requires intimate knowledge of Tallyman and Metronome, as
+# well as guidance from revenue.
+coderd/usage/ @deansheather


in general, I'd like to ensure we have 2 code owners for each thing, so that we have some ability to make progress if someone is out

I don't think codeowners are required to approve PRs for merge, they just get auto-requested for review. I just want to be requested on every PR so I have visibility into changes. If I'm out, anyone else can approve.

Most other codeowners in this file have only one person listed and function the same way.

coderd/database/migrations/000353_create_usage_events_table.up.sql

coderd/database/queries/usageevents.sql

coderd/database/migrations/000353_create_usage_events_table.up.sql

spikecurtis · 2025-07-30T08:53:10Z

coderd/usage/events.go

+// Note that the following event types should not be updated once they are
+// merged into the product. Please consult Dean before making any changes.
+type Event interface {
+	usageEvent() // to prevent external types from implementing this interface


what's the motivation for preventing external types from implementing?

It'd be very bad if somehow event types were added to an external package by mistake and started getting used, since Tallyman would run into issues using them. If we ever do want to add implementations of this interface outside of this package, we can do so after assessing the impacts and removing this dummy method

That makes sense --- tallyman imports this package and this package only to determine what events to care about. Can you add a comment?

spikecurtis · 2025-07-30T09:00:06Z

coderd/usage/events.go

+//     the count of all existing managed agents (count=N)
+//   - A new managed agent is created (count=1)
+type DCManagedAgentsV1 struct {
+	Count uint64 `json:"count"`


Seems like we'll want a lot more than just the count so that customers can understand their usage, e.g. by template, organization, user.

spikecurtis · 2025-07-30T09:21:04Z

enterprise/coderd/usage/publisher_test.go

+	startErr := make(chan error)
+	go func() {
+		err := publisher.Start()
+		testutil.RequireSend(ctx, t, startErr, err)


can't call Require methods in goroutines, only the main test goroutine.

spikecurtis · 2025-07-30T09:28:38Z

enterprise/coderd/usage/publisher_test.go

+		handler func(req usage.TallymanIngestRequestV1) any
+	)
+	ingestURL := fakeServer(t, tallymanHandler(t, licenseJWT, func(req usage.TallymanIngestRequestV1) any {
+		callCount := atomic.AddInt64(&calls, 1)


In order to prevent test races, we need to ensure that the publish calls are completed before we attempt to read this value. If the test prevents such races, atomic is superfluous because we will not concurrently access it. Making it atomic doesn't do anything to prevent racy tests and is confusing because it signals to people that there might be concurrent accesses, when we need to ensure there are none.

spikecurtis · 2025-07-30T09:35:20Z

enterprise/coderd/usage/publisher.go

+// publishOnce publishes up to tallymanPublishBatchSize usage events to
+// tallyman. It returns the number of successfully published events.
+func (p *tallymanPublisher) publishOnce(ctx context.Context, deploymentID uuid.UUID) (int, error) {
+	licenseJwt, err := p.getBestLicenseJWT(ctx)


We shouldn't be doing this for every publish. Can we connect it up to the rest of the license code so we don't add more queries? Or at least use the same query strategy of querying every 10 minutes and listening for published changes?

We shouldn't be doing this for every publish.

This is very light so I don't see the issue with querying it every time. The reason why the entitlements check isn't performed on demand and is cached is that it's used on almost every single request to the API. In general, this is being done a few times every 17 minutes per replica. The licenses table is also very small.

Can we connect it up to the rest of the license code so we don't add more queries?

The license code doesn't actually return much information about licenses themselves, or perform any license selection that matches our requirements. Quite frankly it's in desperate need of a rewrite, but it's out of scope for this PR.

enterprise/coderd/usage/publisher.go

chore: add usage tracking package

f2963cd

github-actions bot assigned deansheather Jul 30, 2025

deansheather requested review from johnstcn and spikecurtis July 30, 2025 06:04

deansheather marked this pull request as ready for review July 30, 2025 06:05

deansheather mentioned this pull request Jul 30, 2025

chore: wire up usage tracking for managed agents #19096

Draft

deansheather added 2 commits July 30, 2025 07:04

add fixture

939aba4

fix manifest.json

e376311

johnstcn reviewed Jul 30, 2025

View reviewed changes

spikecurtis reviewed Jul 30, 2025

View reviewed changes

deansheather mentioned this pull request Jul 30, 2025

chore: move usage types to new package #19103

Draft

github-actions bot added the stale This issue is like stale bread. label Aug 7, 2025

github-actions bot closed this Aug 10, 2025

deansheather reopened this Aug 12, 2025

deansheather removed the stale This issue is like stale bread. label Aug 12, 2025

		err = p.db.UpdateUsageEventsPostPublish(ctx, dbUpdate)
		if err != nil {

chore: add usage tracking package #19095

Are you sure you want to change the base?

chore: add usage tracking package #19095

Conversation

deansheather commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deansheather commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deansheather commented Jul 30, 2025 •

edited

Loading

deansheather commented Jul 30, 2025 •

edited

Loading