-
Notifications
You must be signed in to change notification settings - Fork 962
chore: add usage tracking package #19095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this 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.
enterprise/coderd/usage/publisher.go
Outdated
const ( | ||
CoderLicenseJWTHeader = "Coder-License-JWT" | ||
|
||
tallymanURL = "https://tallyman-ingress.coder.com" |
There was a problem hiding this comment.
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 a var
so it's configurable at build time.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
coderd/database/migrations/000353_create_usage_events_table.up.sql
Outdated
Show resolved
Hide resolved
coderd/database/migrations/000353_create_usage_events_table.up.sql
Outdated
Show resolved
Hide resolved
e376311
to
82ea8e1
Compare
There was a problem hiding this comment.
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 run make gen
again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo gen
failures
There was a problem hiding this 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! 👍
@@ -5666,3 +5666,34 @@ func (s *MethodTestSuite) TestUserSecrets() { | |||
Asserts(userSecret, policy.ActionRead, userSecret, policy.ActionDelete) | |||
})) | |||
} | |||
|
|||
func (s *MethodTestSuite) TestUsageEvents() { | |||
s.Run("InsertUsageEvent", s.Mocked(func(db *dbmock.MockStore, faker *gofakeit.Faker, check *expects) { |
There was a problem hiding this comment.
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 👍
-- 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_type TEXT NOT NULL CONSTRAINT usage_event_type_check CHECK (event_type IN ('dc_managed_agents_v1')), |
There was a problem hiding this comment.
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:
ALTER TYPE notification_method ADD VALUE IF NOT EXISTS 'inbox'; |
Removing them though is a problem:
-- 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
There was a problem hiding this comment.
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.
-- 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Good trick
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[]); |
There was a problem hiding this comment.
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 ... */}
@@ -584,6 +584,7 @@ type Claims struct { | |||
Version uint64 `json:"version"` | |||
Features Features `json:"features"` | |||
RequireTelemetry bool `json:"require_telemetry,omitempty"` | |||
PublishUsageData bool `json:"publish_usage_data,omitempty"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 where RequireTelemetry
goes since they're very similar.
dca0890
to
2529c80
Compare
Merge activity
|
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 AGPLenterprise/coderd/usage
: provides an interface for the "Publisher" as well as a Tallyman implementationRelates to coder/internal#814