- Notifications
You must be signed in to change notification settings - Fork960
chore!: route connection logs to new table#18340
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ethanndickson commentedJun 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Requesting a draft review just to confirm we're happy with the DB schema, the RBAC setup, and the overall direction. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Nice work! I think this looks great in general, but the upsert part of the logic here doesn't fully make sense to me (see related comment). Could you elaborate on the intent?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
947ccd1
to5231bef
Comparee4e1a04
to1fcd8d7
CompareUh oh!
There was an error while loading.Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
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.
Some minor nits and suggestions but largely looks alright to me, nice work! 👍🏻
Also, obligatory:./coderd/database/migrations/fix_migration_numbers.sh
.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
f47a3f7
to0346609
CompareThere 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.
Pull Request Overview
This PR introduces a dedicatedconnection_logs
table and replaces audit-based connection events with a newConnectionLogger
abstraction, ensuring SSH and Workspace App connections are recorded in their own table and UI. Key changes include:
- Schema and SQL migrations for
connection_logs
, plus generated Go query methods and RBAC filters. - New
ConnectionLogger
abstraction in both enterprise and open core, replacing audit calls for connection events. - Updates across CLI, API server, agent API, and tests to route connection events through
ConnectionLogger
.
Reviewed Changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
enterprise/coderd/connectionlog/connectionlog.go | ImplementsBackend andConnectionLogger types |
coderd/database/migrations/000349_connection_logs.up.sql | Createsconnection_logs table and types |
coderd/database/queries/connectionlogs.sql | SQL for reading and upserting connection logs |
coderd/workspaceapps/db.go | Replaces audit logic with connection logging |
coderd/agentapi/connectionlog.go | New agent API endpoint for reporting connections |
coderd/database/types.go | AddsParseIP helper forinet type |
coderd/database/queries.sql.go | Adds generatedGetConnectionLogsOffset andUpsertConnectionLog methods |
Comments suppressed due to low confidence (2)
coderd/workspaceapps/db.go:97
- [nitpick] The variable name
commitAudit
is misleading since this sets up connection logging, not audit logging. Consider renaming it tocommitConnLog
orcommitConnectionLog
for clarity.
aReq, commitAudit := p.connLogInitRequest(ctx, rw, r)
coderd/workspaceapps/db.go:394
- [nitpick] The comment uses “connect log” where “connection log” would match the terminology used elsewhere. Consider updating for consistency.
// connLogInitRequest creates a new connection log session and connect log for the
Uh oh!
There was an error while loading.Please reload this page.
7d13236
tobea35a3
Comparebea35a3
to3164ea2
Compareethanndickson commentedJul 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Merge activity
|
08e17a0
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Breaking Change (changelog note):
Context
This is the first PR of a few for moving connection events out of the audit log, and into a new database table and web UI page called the 'Connection Log'.
This PR:
ConnectionLogger
abstraction to replace theAuditor
abstraction for these logs. (No-op'd in AGPL, like theAuditor
)ConnectionLogger
ConnectionLogger
instead of theAuditor
.Future PRs:
Note
The PRs in this stack obviously won't be (completely) atomic. Whilst they'll each pass CI, the stack is designed to be merged all at once. I'm splitting them up for the sake of those reviewing, and so changes can be reviewed as early as possible. Despite this, it's really hard to make this PR any smaller than it already is. I'll be keeping it in draft until it's actually ready to merge.