-
Notifications
You must be signed in to change notification settings - Fork 25
refactor: logstore & log api #580
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
Open
alexluong
wants to merge
82
commits into
main
Choose a base branch
from
logstore
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Include last count, status code, and any errors in the timeout message to help debug flaky test failures. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Prevents tests from hanging indefinitely if the server doesn't respond. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove Event ID column from deliveries table - Remove Event Time from delivery details - Flatten delivery details into single section (no separate Event section) - Rename data sections: 'Data', 'Metadata', 'Response' - Reorder fields: Status, Response Code, Attempt, Topic, Delivered at, IDs
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Convert buildEventCursorCondition and buildCursorCondition to use parameterized queries with ? placeholders instead of string interpolation to prevent SQL injection vulnerabilities. Add parseTimestampMs helper to validate timestamp strings before use. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The SortBy option (event_time vs delivery_time) added complexity to cursor pagination and query logic without significant benefit. This change simplifies the API by always sorting by delivery_time, which is the natural ordering for delivery-focused queries. Changes: - Remove SortBy field from ListDeliveryEventRequest in driver.go - Simplify memlogstore, pglogstore, chlogstore to always use delivery_time sort - Remove sort_by query param validation from API handlers - Remove testCursorMismatchedSortBy and related tests from drivertest - Update log_handlers_test.go to remove sort_by validation tests Co-Authored-By: Claude Opus 4.5 <[email protected]>
…yEnd to Start/End Co-Authored-By: Claude Opus 4.5 <[email protected]>
…params Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ttempt fields Co-Authored-By: Claude Opus 4.5 <[email protected]>
…t fields Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Change partition strategy from daily (toYYYYMMDD) to monthly (toYYYYMM) to reduce part count and merge pressure - Reduce bloom filter granularity from 4 to 1 for better index selectivity on low-cardinality columns like tenant_id and destination_id Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
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.
LogStore Refactor: Stateless Schema Design
Summary
This PR refactors the log storage layer with a stateless schema design - queries return rows directly without aggregation, GROUP BY, or complex joins. The schema is optimized for O(limit) query performance.
Changes:
/deliveries,/events) with redesigned request/response schemaAI Disclosure
This PR was heavily AI-assisted for implementation. However, the following were done manually (some discussed with the team across multiple sessions):
Note on test changes: There are significant test code changes in this PR. These are intentional refactoring for:
Tests were passing before refactoring. The pattern was: feature tests pass → refactor tests → verify tests still pass.
Review Areas
1. LogStore Interface & Data Layer
Interface
Request structs support:
Next,Prev)EventStart,EventEndfor events;Start,Endfor deliveries)SortOrder: asc|desc - sort dimension is fixed per endpoint)TenantID,DestinationIDs,Topics,Status,EventID)ClickHouse Schema
Two tables with stateless query design - no GROUP BY, no aggregation:
Design rationale:
deliveries: Denormalized delivery+event rows for O(limit) delivery queriesevents: Separate table for O(limit) event listing without GROUP BYReplacingMergeTree: Handles duplicate inserts gracefullyRelevant Packages
internal/logstore/driver- interfaceinternal/logstore/cursor- cursor encodinginternal/logstore/chlogstore- ClickHouseinternal/logstore/pglogstore- PostgreSQLinternal/logstore/memlogstore- in-memory (reference impl)internal/logstore/drivertest- conformance tests2. API Layer
Routes
/:tenantID/deliveriesListDeliveries/:tenantID/deliveries/:deliveryIDRetrieveDelivery/:tenantID/deliveries/:deliveryID/retryRetryDelivery/:tenantID/eventsListEvents/:tenantID/events/:eventIDRetrieveEventQuery Parameters (ListDeliveries)
Include Pattern
The
?includeparameter controls nested object hydration using dot notation:"event": "evt_123"(ID only)event"event": {id, topic, time, eligible_for_retry, metadata}event.data"event": {..., data}(includes payload)By default, related entities return as IDs.
include=eventhydrates the event object without payload.include=event.dataadditionally includes the (potentially large) event data. This keeps responses lean while allowing single-request hydration when needed.Legacy Routes (Deprecated)
Preserved with
Deprecation: trueheader:GET /:tenantID/destinations/:destID/eventsGET /:tenantID/destinations/:destID/events/:eventIDPOST /:tenantID/destinations/:destID/events/:eventID/retryRelevant Packages
internal/apirouter3. Portal UI
Relevant Packages
internal/portal/src4. Test Infrastructure
Performance
Test suite reduced from 1-2 min → 30-40s through polling helpers and timeout reductions.
cmd/e2e19dadb4cmd/e2e0651251cmd/e2e706db81internal/logstore381fe6einternal/logstore072c0adinternal/destregistry/destwebhook1ed441cinternal/destregistry/destawskinesis9fe3bd6internal/idempotenceb8be306internal/mqinfrab8be306internal/mqsb8be306internal/models7fdb59fConformance Tests
Shared test suite (
drivertest) that all LogStore drivers must pass.Relevant Packages
cmd/e2einternal/logstore/drivertestMigration Notes