refactor: switch to hub object storage s3 client #28140
Conversation
| @@ -495,17 +495,31 @@ export async function startPluginsServer( | |||
| if (capabilities.sessionRecordingBlobIngestionV2) { | |||
| const hub = await setupHub() | |||
| const postgres = hub?.postgres ?? new PostgresRouter(serverConfig) | |||
| const objectStorage = hub?.objectStorage ?? getObjectStorage(serverConfig) | |||
There was a problem hiding this comment.
Hmmmmm does this mean that we are using the same config as the other recorder? We definitely don't want to use the same bucket etc 🤔
There was a problem hiding this comment.
We use that bucket for a few other things too, so I wanted to get something running quickly. I can take a look at creating a new bucket – creating the bucket should be easy, I'm not sure how access keys are handled in this case though.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors the session recording ingestion to use a centralized S3 client from the hub's object storage instead of creating its own instance, improving configuration management and dependency injection.
- Modified
SessionRecordingIngesterV2constructor to accept an external S3Client in/plugin-server/src/main/ingestion-queues/session-recording-v2/consumer.ts - Simplified
S3SessionBatchWriterto use injected S3Client in/plugin-server/src/main/ingestion-queues/session-recording-v2/sessions/s3-session-batch-writer.ts - Updated
pluginsServer.tsto pass hub's object storage S3 client to session recording ingestion - Removed
SESSION_RECORDING_V2_S3_REGIONconfig as region is now handled through hub's object storage - Updated test suite to use mocked S3Client through dependency injection instead of internal creation
5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| beforeEach(() => { | ||
| uploadedData = Buffer.alloc(0) | ||
| jest.mocked(S3Client).mockImplementation(() => ({} as any)) | ||
| mockS3Client = {} as jest.Mocked<S3Client> |
There was a problem hiding this comment.
style: Empty mock object might not be sufficient - consider adding minimal required S3Client interface methods
Problem
Session recording v2 instantiated a new S3 client, which wasn't properly set up.
Changes
Switches to the S3 client provided by the hub or by the
getObjectStoragefactory.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Will verify on dev.