Movatterモバイル変換

Copy link

MemberAuthor

deansheather commentedJul 30, 2025•
edited
Loading

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

deansheather requested review fromjohnstcn andspikecurtis

July 30, 2025 06:04

deansheather marked this pull request as ready for review

July 30, 2025 06:05

deansheather mentioned this pull request

chore: wire up usage tracking for managed agents#19096

Merged

johnstcn reviewed

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

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

coderd/database/dbauthz/dbauthz.go OutdatedShow resolvedHide resolved

coderd/database/queries/usageevents.sql OutdatedShow resolvedHide resolved

enterprise/coderd/usage/publisher.go Outdated

		const (
		CoderLicenseJWTHeader="Coder-License-JWT"

		tallymanURL="https://tallyman-ingress.coder.com"

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

enterprise/coderd/usage/publisher.go OutdatedShow resolvedHide resolved

enterprise/coderd/usage/publisher.goShow resolvedHide resolved

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Obligatory reminder to check migration number before merging!

spikecurtis reviewed

CODEOWNERS OutdatedShow resolvedHide resolved

coderd/database/migrations/000353_create_usage_events_table.up.sql OutdatedShow resolvedHide resolved

coderd/database/queries/usageevents.sql OutdatedShow resolvedHide resolved

coderd/database/migrations/000353_create_usage_events_table.up.sql OutdatedShow resolvedHide resolved

coderd/usage/events.goShow resolvedHide resolved

enterprise/coderd/usage/publisher_test.go OutdatedShow resolvedHide resolved

enterprise/coderd/usage/publisher.goShow resolvedHide resolved

enterprise/coderd/usage/publisher.go OutdatedShow resolvedHide resolved

deansheather mentioned this pull request

chore: move usage types to new package#19103

Merged

github-actionsbot added the staleThis issue is like stale bread. label

Aug 7, 2025

github-actionsbot closed this

Aug 10, 2025

deansheather reopened this

Aug 12, 2025

deansheather removed the staleThis issue is like stale bread. label

Aug 12, 2025

deansheather force-pushed the07-30-chore_add_usage_tracking_package branch frome376311 to82ea8e1Compare

August 15, 2025 09:28

deansheather requested a review fromEmyrk as acode owner

August 15, 2025 09:28

deansheather requested review fromspikecurtis andjohnstcn

August 15, 2025 09:29

johnstcn approved these changes

Copy link

Member

johnstcn left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't have any further blocking comments and the dbauthz stuff looks OK to me. I'd like a +1 from Spike before merging though. You probably also need to runmake gen again.

coderd/usage/events.goShow resolvedHide resolved

enterprise/coderd/usage/inserter.goShow resolvedHide resolved

enterprise/coderd/usage/publisher_test.goShow resolvedHide resolved

spikecurtis approved these changes

Copy link

Contributor

spikecurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM modulogen failures

Emyrk reviewed

coderd/database/dbauthz/dbauthz_test.go

Copy link

Member

Emyrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No blockers from me! 👍

		}

		func (s*MethodTestSuite)TestUsageEvents() {
		s.Run("InsertUsageEvent",s.Mocked(func(dbdbmock.MockStore,fakergofakeit.Faker,check*expects) {

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Mocked! Nice! Been slowly converting them all 👍

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

Comment on lines +3 to +6

		-- We use a TEXT column with a CHECK constraint rather than an enum because of
		-- the limitations with adding new values to an enum and using them in the
		-- same transaction.
		event_typeTEXTNOT NULLCONSTRAINT usage_event_type_checkCHECK (event_typeIN ('dc_managed_agents_v1')),

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Adding enum values is actually easy now:

coder/coderd/database/migrations/000300_notifications_method_inbox.up.sql

Line 1 in82ea8e1

ALTERTYPE notification_method ADD VALUE IF NOT EXISTS'inbox';

Removing them though is a problem:

coder/coderd/database/migrations/000300_notifications_method_inbox.down.sql

Lines 1 to 3 in82ea8e1

	-- The migration is about an enum value change
	-- As we can not remove a value from an enum, we can let the down migration empty
	-- In order to avoid any failure, we use ADD VALUE IF NOT EXISTS to add the value

(Although we never run down migrations so....)

Making it an enum generates the proper Golang enum

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Spike asked me to not do this in a prior comment because you can't add a value and then reference it in the same transaction unless you're on pg 17. We run all migrations in a single transaction (for good reason, to avoid partial upgrades), which means we can never reference enum types in future transactions.

coderd/database/queries/usageevents.sql

Comment on lines +59 to +61

		-- Note that this selects from the CTE, not the original table. The CTE is named
		-- the same as the original table to trick sqlc into reusing the existing struct
		-- for the table.

Copy link

Member

EmyrkAug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍 Good trick

coderd/database/queries/usageevents.sql

Comment on lines +74 to +86

		SELECT
		UNNEST(@ids::text[])AS id,
		UNNEST(@failure_messages::text[])AS failure_message,
		UNNEST(@set_published_ats::boolean[])AS set_published_at
		) input
		WHERE
		input.id=usage_events.id
		-- If the number of ids, failure messages, and set published ats are not the
		-- same, do not do anything. Unfortunately you can't really throw from a
		-- query without writing a function or doing some jank like dividing by
		-- zero, so this is the best we can do.
		AND cardinality(@ids::text[])= cardinality(@failure_messages::text[])
		AND cardinality(@ids::text[])= cardinality(@set_published_ats::boolean[]);

Copy link

Member

EmyrkAug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yea, if we ever switch to PGX, it has batch operations:https://docs.sqlc.dev/en/latest/reference/changelog.html#batch-operation-improvements

Then we get type safety and the inputs are[]struct{ /* ... Fields ... */}

coderd/rbac/roles_test.go OutdatedShow resolvedHide resolved

enterprise/coderd/license/license.go

		Versionuint64`json:"version"`
		FeaturesFeatures`json:"features"`
		RequireTelemetrybool`json:"require_telemetry,omitempty"`
		PublishUsageDatabool`json:"publish_usage_data,omitempty"`

Copy link

Member

EmyrkAug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Interesting this is not an entitlement?

Copy link

MemberAuthor

deansheatherAug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's not really a feature, rather a flag to send usage data (which will always get collected in enterprise builds) to a server. So it makes sense that it goes whereRequireTelemetry goes since they're very similar.

enterprise/coderd/usage/publisher.go OutdatedShow resolvedHide resolved

deansheather added5 commits

August 15, 2025 15:09

chore: add usage tracking package

b978c4c

add fixture

1a38205

PR comments

3afa02e

more PR comments

5ab8c27

gen

2529c80

deansheather force-pushed the07-30-chore_add_usage_tracking_package branch fromdca0890 to2529c80Compare

August 15, 2025 15:19

deansheather merged commita25d856 intomain

50 of 53 checks passed

Copy link

MemberAuthor

deansheather commentedAug 15, 2025

Merge activity

Aug 15, 3:31 PM UTC:@deansheather merged this pull request withGraphite.

deansheather deleted the 07-30-chore_add_usage_tracking_package branch

August 15, 2025 15:31

github-actionsbot locked and limited conversation to collaborators