-
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
base: main
Are you sure you want to change the base?
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.
jsonData, err := json.Marshal(event.Fields()) | ||
if err != nil { | ||
return xerrors.Errorf("marshal event as JSON: %w", err) | ||
} |
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.
Might be worth adding a test case for an un-marshalable event
func jsoninate(t *testing.T, v any) string { | ||
t.Helper() | ||
buf, err := json.Marshal(v) | ||
require.NoError(t, err) | ||
return string(buf) | ||
} |
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.
suggestion, non-blocking: could be useful as testutil.MustJSON
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
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