feat(storage): support any S3-compatible provider for session replays#1021
Open
DanielvdSpoel wants to merge 1 commit into
Open
feat(storage): support any S3-compatible provider for session replays#1021DanielvdSpoel wants to merge 1 commit into
DanielvdSpoel wants to merge 1 commit into
Conversation
Session replay batches were stored only in Cloudflare R2, with the endpoint, region and credentials hardcoded to R2. The service already used the AWS S3 SDK, so generalize it to any S3-compatible provider (AWS S3, R2, MinIO, ...): - Rename r2StorageService -> s3StorageService (export objectStorage) and update the session-replay ingest/query consumers. - Configure via generic S3_* env vars (S3_ENDPOINT, S3_REGION, S3_ACCESS_KEY_ID, S3_SECRET_ACCESS_KEY, S3_BUCKET_NAME, S3_FORCE_PATH_STYLE). Enabled whenever credentials are present, so self-hosted instances can offload replays without IS_CLOUD. - Keep legacy R2_* vars as a fallback and derive the R2 endpoint from R2_ACCOUNT_ID, so existing cloud/R2 deployments work unchanged. - Document the new variables and wire them into docker-compose.yml. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@DanielvdSpoel is attempting to deploy a commit to the goldflag's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThis PR migrates session replay storage from Cloudflare R2 to a generalized S3-compatible object storage service. The storage layer is refactored to support multiple S3 providers with credential/endpoint configuration, while maintaining backward compatibility with existing R2 environment variables. Session replay ingest and query services are updated to use the new storage, with error-tolerant parallel batch retrieval and deletion patterns. ChangesR2 to S3-Compatible Storage Migration
Sequence DiagramssequenceDiagram
participant Client
participant IngestService as SessionReplayIngestService
participant ObjectStore as objectStorage
participant ClickHouse
Client->>IngestService: replay events
IngestService->>IngestService: prepare event batch
alt Storage Enabled
IngestService->>ObjectStore: storeBatch(batchKey, events)
alt Success
ObjectStore-->>IngestService: ✓
IngestService->>ClickHouse: insert metadata rows<br/>(event_data_key=batchKey)
else Failure
ObjectStore-->>IngestService: error
IngestService->>IngestService: fallback batchKey=null
IngestService->>ClickHouse: insert full event data<br/>(event_data=payload)
end
else Storage Disabled
IngestService->>ClickHouse: insert full event data
end
sequenceDiagram
participant Client
participant QueryService as SessionReplayQueryService
participant ClickHouse
participant ObjectStore as objectStorage
Note over QueryService: getSessionReplayEvents
Client->>QueryService: fetch session replays
QueryService->>ClickHouse: query event metadata
alt Object Storage Enabled
ClickHouse-->>QueryService: split into<br/>clickhouseBatches +<br/>objectStorageBatches
par Parallel Batch Fetch (chunk 50)
QueryService->>ObjectStore: getBatch(key1)
QueryService->>ObjectStore: getBatch(key2)
QueryService->>ObjectStore: getBatch(keyN)
and
ObjectStore-->>QueryService: batch data or null on error
end
QueryService->>QueryService: reconstruct events<br/>by batch_index
else Direct ClickHouse
ClickHouse-->>QueryService: full event data
end
QueryService->>QueryService: sort by timestamp
QueryService-->>Client: combined events
Note over QueryService: deleteSessionReplay
Client->>QueryService: delete session
QueryService->>ClickHouse: fetch distinct event_data_keys
alt Object Storage Enabled
par Parallel Deletion
QueryService->>ObjectStore: deleteBatch(key1)
QueryService->>ObjectStore: deleteBatch(key2)
and
ObjectStore-->>QueryService: ✓ or logged error
end
end
QueryService->>ClickHouse: delete session_replay_events
QueryService->>ClickHouse: delete session_replay_metadata
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/src/services/storage/s3StorageService.ts (1)
148-151: 💤 Low valueUse the structured logger for error paths.
The service initializes
this.loggerbut error handling usesconsole.error. This loses structured context (bucket, endpoint) that would aid debugging in production.Suggested fix
} catch (error) { - console.error("[S3Storage] Failed to store batch:", error); + this.logger.error({ error, key }, "Failed to store batch"); throw error; }} catch (error) { - console.error("[S3Storage] Failed to retrieve batch:", error); + this.logger.error({ error, key }, "Failed to retrieve batch"); throw error; }} catch (error) { - console.error("[S3Storage] Failed to delete batch:", error); + this.logger.error({ error, key }, "Failed to delete batch"); // Non-critical error, log but don't throw }Also applies to: 224-227, 245-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/storage/s3StorageService.ts` around lines 148 - 151, Replace console.error usages in S3StorageService catch blocks with this.logger.error and pass structured context (e.g., bucket and endpoint) and the caught error; specifically update the catch in the method handling batch storage (where console.error("[S3Storage] Failed to store batch:", error) currently appears) and the other two catch blocks flagged (around the same pattern) to call this.logger.error with a descriptive message and an object like { bucket: this.bucket, endpoint: this.endpoint, error } so the logger emits structured fields rather than plain console output.server/src/services/replay/sessionReplayQueryService.ts (1)
229-242: 💤 Low valueConsider explicit bounds check instead of truthy check.
Line 233 uses
data[event.batch_index]as a truthy guard, which would skip events with falsy data (e.g.,0,"",false). While rrweb event data is always an object so this is safe in practice, an explicit bounds check is more defensive.Suggested fix
if (data) { for (const event of batchEvents) { - if (event.batch_index !== null && data[event.batch_index]) { + if (event.batch_index !== null && event.batch_index < data.length) { events.push({ timestamp: event.timestamp, type: event.type, data: data[event.batch_index], }); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/replay/sessionReplayQueryService.ts` around lines 229 - 242, Replace the truthy guard on data[event.batch_index] with an explicit bounds/exists check: when iterating objectStorageResults and inner batchEvents in sessionReplayQueryService (variables: objectStorageResults, batchEvents, data, event.batch_index, events), ensure you verify that event.batch_index is not null/undefined and that the index/key actually exists on data (e.g., check typeof event.batch_index === 'number' and data[event.batch_index] !== undefined or use Object.prototype.hasOwnProperty.call(data, event.batch_index)) before pushing to events so falsy values like 0, "", or false are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/src/services/replay/sessionReplayQueryService.ts`:
- Around line 229-242: Replace the truthy guard on data[event.batch_index] with
an explicit bounds/exists check: when iterating objectStorageResults and inner
batchEvents in sessionReplayQueryService (variables: objectStorageResults,
batchEvents, data, event.batch_index, events), ensure you verify that
event.batch_index is not null/undefined and that the index/key actually exists
on data (e.g., check typeof event.batch_index === 'number' and
data[event.batch_index] !== undefined or use
Object.prototype.hasOwnProperty.call(data, event.batch_index)) before pushing to
events so falsy values like 0, "", or false are preserved.
In `@server/src/services/storage/s3StorageService.ts`:
- Around line 148-151: Replace console.error usages in S3StorageService catch
blocks with this.logger.error and pass structured context (e.g., bucket and
endpoint) and the caught error; specifically update the catch in the method
handling batch storage (where console.error("[S3Storage] Failed to store
batch:", error) currently appears) and the other two catch blocks flagged
(around the same pattern) to call this.logger.error with a descriptive message
and an object like { bucket: this.bucket, endpoint: this.endpoint, error } so
the logger emits structured fields rather than plain console output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83a3db78-c6a4-4b3f-9ce9-4a66020f7c5b
📒 Files selected for processing (5)
docker-compose.ymldocs/content/docs/managing-your-installation.mdxserver/src/services/replay/sessionReplayIngestService.tsserver/src/services/replay/sessionReplayQueryService.tsserver/src/services/storage/s3StorageService.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
feat(storage): support any S3-compatible provider for session replays
Summary
Session replay batches could previously only be offloaded to Cloudflare R2, and only on cloud (
IS_CLOUD) deployments — the endpoint, region, and credentials were hardcoded to R2. Since the service already uses the AWS S3 SDK, this PR generalizes it to any S3-compatible provider (AWS S3, Cloudflare R2, MinIO, …) so self-hosted instances can offload replays too.Storage is now enabled automatically whenever credentials are present. When no credentials are configured, replays continue to be stored in ClickHouse exactly as before.
Changes
r2StorageService→s3StorageService(singleton exported asobjectStorage) and update the session-replay ingest/query consumers. Configuration is resolved from the environment into a typed config object, with credentials being the only requirement to enable storage.S3_*configuration —S3_ENDPOINT,S3_REGION,S3_ACCESS_KEY_ID,S3_SECRET_ACCESS_KEY,S3_BUCKET_NAME,S3_FORCE_PATH_STYLE. No longer gated behindIS_CLOUD.R2_*variables (R2_ACCESS_KEY_ID,R2_SECRET_ACCESS_KEY,R2_BUCKET_NAME,R2_ACCOUNT_ID) are still honored as a fallback, and the R2 endpoint is derived fromR2_ACCOUNT_IDwhenS3_ENDPOINTis unset. Existing cloud/R2 deployments need no changes.managing-your-installation.mdxand wire them intodocker-compose.yml.Configuration
S3_ACCESS_KEY_IDS3_SECRET_ACCESS_KEYS3_BUCKET_NAMErybbitS3_ENDPOINThttps://<account>.r2.cloudflarestorage.comorhttp://minio:9000. Omit for AWS S3.S3_REGIONautoS3_FORCE_PATH_STYLEtruefalsefor virtual-hosted AWS S3.Compatibility / migration
R2_*vars keep working with the endpoint derived fromR2_ACCOUNT_ID.Testing
cd server && tsc(type check)Please let me know if you'd prefer the legacy
R2_*fallback dropped or kept — it's included here to avoid disrupting current cloud deployments.Summary by CodeRabbit
New Features
Documentation
Chores