Skip to content

feat: add dedicated slow query logging to ClickHouse adapter#112

Closed
lohanidamodar wants to merge 3 commits intomainfrom
CLO-4204-slow-query-table
Closed

feat: add dedicated slow query logging to ClickHouse adapter#112
lohanidamodar wants to merge 3 commits intomainfrom
CLO-4204-slow-query-table

Conversation

@lohanidamodar
Copy link
Copy Markdown
Contributor

@lohanidamodar lohanidamodar commented Apr 12, 2026

Summary

  • Adds purpose-built slow query logging methods to the ClickHouse adapter with a dedicated slow_queries table schema
  • Schema uses native ClickHouse types (UInt32 for duration, UInt16 for status code, LowCardinality(String) for method/action/plan) instead of stuffing everything into a generic audit data JSON blob
  • Includes a minmax skip-index on durationMs for efficient threshold-based filtering

Changes

  • src/Audit/Adapter/ClickHouse.php:
    • Added setupSlowQueries() — creates {namespace}_slow_queries table with the dedicated schema
    • Added createSlowQuery(array $data) — type-safe insert for a single slow query row
    • Added findSlowQueries(?DateTime, ?DateTime, limit, offset) — time-range query with tenant isolation
    • Added cleanupSlowQueries(DateTime $before) — deletes rows older than a given date
    • Removed the $table constructor parameter (replaced by dedicated methods instead of generic table-swapping)

Slow queries table schema

id          String
time        DateTime64(3)
durationMs  UInt32
method      LowCardinality(String)
action      LowCardinality(String)
path        String
uri         String
hostname    String
projectId   String
userId      String
statusCode  UInt16
plan        LowCardinality(String)
ip          String
userAgent   String
data        String
-- + tenant Nullable(UInt64) when sharedTables is enabled
-- INDEX idx_duration durationMs TYPE minmax GRANULARITY 4
-- ORDER BY (time, id), PARTITION BY toYYYYMM(time)

Verification

  • PHPStan (composer check): No errors
  • Lint (composer lint): Pass

Test plan

  • Verify setupSlowQueries() creates console_slow_queries and projects_slow_queries tables in ClickHouse
  • Verify createSlowQuery() inserts a row with correct types (durationMs as UInt32, statusCode as UInt16)
  • Verify findSlowQueries() returns results filtered by time range and tenant
  • Verify cleanupSlowQueries() deletes rows older than the given datetime
  • Verify existing audit table methods (setup(), create(), find()) are unaffected

Allow configuring the table name used by the ClickHouse adapter,
enabling use cases like a dedicated slow query log table alongside
the default audits table. Follows the same setter/getter pattern
as setDatabase, setNamespace, and setTenant.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 12, 2026

Greptile Summary

This PR's title and description state it adds setTable()/getTable() methods, but the actual diff instead introduces a full slow-query logging subsystem: setupSlowQueries(), createSlowQuery(), findSlowQueries(), cleanupSlowQueries(), and the getSlowQueryTableName() helper. The test-plan checkboxes in the description (which reference setTable/getTable) are therefore testing functionality that was not added. No setTable() or getTable() methods appear anywhere in the changeset or in the existing file.

The new methods follow established patterns in the adapter (identifier escaping, tenant filtering, namespace prefixing) and the implementation looks functionally correct. The one minor inconsistency is that cleanupSlowQueries binds its datetime as {datetime:String} while the new findSlowQueries correctly uses {after:DateTime64(3)}/{before:DateTime64(3)} for the same column type.

Confidence Score: 4/5

Safe to merge functionally, but the PR description should be corrected to accurately reflect the slow-query logging feature that was actually implemented.

The implementation follows the adapter's existing patterns and is functionally correct. The score is 4 rather than 5 because the PR title and description completely misrepresent the changes (claiming setTable/getTable when slow-query logging was actually added), which makes the PR difficult to review or reason about historically.

src/Audit/Adapter/ClickHouse.php — verify the slow-query feature is what was intended, and update the PR title/description accordingly.

Important Files Changed

Filename Overview
src/Audit/Adapter/ClickHouse.php Adds a complete slow-query logging subsystem (setupSlowQueries, createSlowQuery, findSlowQueries, cleanupSlowQueries) — PR title/description incorrectly describes this as adding setTable/getTable methods; minor DateTime parameter-type inconsistency between findSlowQueries and cleanupSlowQueries.

Reviews (3): Last reviewed commit: "feat: add dedicated slow query logging m..." | Re-trigger Greptile

Comment thread src/Audit/Adapter/ClickHouse.php Outdated
lohanidamodar and others added 2 commits April 12, 2026 06:10
Replace mutable setTable()/getTable() with an immutable constructor
parameter to prevent accidental table name changes on a shared adapter
instance. The table name defaults to 'audits' for backward compatibility.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the generic setTable/getTable approach with purpose-built
slow query methods: setupSlowQueries(), createSlowQuery(),
findSlowQueries(), and cleanupSlowQueries(). The slow_queries table
has its own schema with indexed durationMs (UInt32), LowCardinality
method/action/plan, and UInt16 statusCode — optimized for analytical
queries without JSON extraction.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@lohanidamodar lohanidamodar changed the title feat: add setTable/getTable methods to ClickHouse adapter feat: add dedicated slow query logging to ClickHouse adapter Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant