chore: populate connectionlog count using a separate query #18629
+688
−10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ed8e139 to
8cec6d1
Compare
a031154 to
d03fe73
Compare
8cec6d1 to
482a5d3
Compare
d03fe73 to
8cbd44a
Compare
482a5d3 to
ccc7539
Compare
8cbd44a to
09cbe49
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a separate count query for connection logs so that the ConnectionLogResponse includes a total count, mitigating performance issues described in #17689. Key changes include:
- HTTP handler now runs
CountConnectionLogsand populatesCountin its response. searchquery.ConnectionLogsreturns two filter structs (offsetFilterandcountFilter) and the handler invokes the newCountConnectionLogsmethod.- Added SQL and Go plumbing (queries, querier interfaces, mocks, metrics, in-memory impl, authorization, and tests) to support counting connection logs.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| enterprise/coderd/connectionlog.go | Calls CountConnectionLogs before fetching paged results and writes Count in the response |
| enterprise/coderd/connectionlog_test.go | Adds assertions for the new Count field in tests |
| coderd/searchquery/search.go | Updated ConnectionLogs to return a CountConnectionLogsParams filter |
| coderd/searchquery/search_test.go | Adjusted tests to unpack the new return value |
| coderd/database/queries/connectionlogs.sql | Introduced CountConnectionLogs SQL query |
| coderd/database/queries.sql.go | Added countConnectionLogs constant and CountConnectionLogs implementation |
| coderd/database/querier.go | Extended querier interface with CountConnectionLogs |
| coderd/database/modelqueries_internal_test.go | Added consistency test to ensure both queries share the same WHERE clause |
| coderd/database/modelqueries.go | Implemented CountAuthorizedConnectionLogs |
| coderd/database/dbmock/dbmock.go | Generated mocks for the new count methods |
| coderd/database/dbmetrics/querymetrics.go | Wrapped new count methods with latency metrics |
| coderd/database/dbmem/dbmem.go | Added in-memory implementation of CountConnectionLogs |
| coderd/database/dbauthz/setup_test.go | Unified empty-response checks for count tests |
| coderd/database/dbauthz/dbauthz_test.go | Added RBAC tests for count methods |
| coderd/database/dbauthz/dbauthz.go | Authorized count methods in the dbauthz layer |
deansheather
approved these changes
Jul 2, 2025
ccc7539 to
406e3f4
Compare
09cbe49 to
0f13056
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the third PR for moving connection events out of the audit log.
This PR populates
countonConnectionLogResponseusing a separate query, to preemptively mitigate the issue described in #17689. It's structurally identical to a portion of #18600, but for the connection log instead of the audit log.Future PRs: