ref(buffering): Add BufferService with SQLite backend #1920

olksdr · 2023-03-13T08:00:28Z

In PR:

add config option to enable persistent buffer
add migrations to create DB and required table for storing the envelopes
- migrations are built-in
- migrations run on start up and if the failure happens relay won't start at all
when persist the envelopes to the disk, the outcome won't be sent
keep a memory buffer and spool it when it hits defined memory limit
update BufferService to handle the sqlite pooling and insert/select/delete operations
when Relay stops we try to spool in-memory buffer to disk as well

fix: https://github.com/getsentry/team-ingest/issues/78

This is the initial naive implementation of the prsistent envelope buffer backed by SQLite. In this initial iterration: * add config option to enable persistent buffer * add migrations to create DB and required table for storing the envelopes - migrations run on start up and if the failure happens relay won't start at all * update envelope context to avoid sending the outcome if it's dropped while persisting it into the buffer - we might want to use `done` here instead * add SqliteBufferService which handles the sqlite pooling and insert/select/delete operations Still some TODOs: * metrics won't make any sense if we dropp and re-create the envelope context, still have to handle this properly - persist the data from the initil context? - don't care about it at all? * add high-/lowwaterrmark and use the memory for the inital buffer before storing envelopes in the db * read up the envelopes in batches instead of all at once

jjbayer

Preliminary review, mostly questions and bikeshedding.

jjbayer · 2023-03-20T16:10:20Z

migrations/20230311081917_envelopes_table.sql

+  own_key         TEXT,
+  sampling_key    TEXT,


Can we make these fixed width 32 byte arrays? That should make the lookup faster (at least in theory).

I will have to look into this, how the indicies on the binary fields work.

That should make the lookup faster

I'm basing this on an assumption that TEXT fields store a reference to a string that is saved somewhere else, while fixed-width types are stored "inline". But this assumption might be wrong as I have no idea how sqlite does it.

relay-server/src/actors/project_buffer.rs

.gitignore

relay-config/src/config.rs

relay-server/src/actors/project_buffer.rs

relay-server/src/actors/project_cache.rs

relay-server/src/utils/buffer.rs

iker-barriocanal

This is good stuff, looks good!

Some comments on top of what Joris suggests.

relay-config/src/config.rs

relay-server/src/lib.rs

relay-server/src/utils/managed_envelope.rs

iker-barriocanal · 2023-03-21T13:14:56Z

relay-server/src/actors/project_buffer.rs

+    /// Describes the errors linked with the `Sqlite` backed buffer.
+    #[error("failed to fetch data from the database")]
+    DatabaseError(#[from] sqlx::Error),


A database is a type of backend of a buffer. Should we abstract it? Feel free to address this in a follow-up PR, or not do it at all until we actually use another backend.

relay-server/src/utils/buffer.rs

migrations/20230311081917_envelopes_table.sql

relay-server/src/actors/project_cache.rs

jjbayer · 2023-03-23T11:12:36Z

migrations/20230311081917_envelopes_table.sql

+  own_key         TEXT,
+  sampling_key    TEXT,


That should make the lookup faster

I'm basing this on an assumption that TEXT fields store a reference to a string that is saved somewhere else, while fixed-width types are stored "inline". But this assumption might be wrong as I have no idea how sqlite does it.

migrations/20230311081917_envelopes_table.sql

relay-server/src/actors/project_buffer.rs

relay-server/src/utils/buffer.rs

jjbayer

Looks good, I think this is ready for load testing.

migrations/20230311081917_envelopes_table.sql

relay-server/src/actors/project_buffer.rs

relay-server/src/actors/project_cache.rs

relay-server/src/actors/project_buffer.rs

relay-server/src/actors/project_cache.rs

relay-server/src/utils/buffer.rs

relay-server/src/utils/semaphore.rs

relay-server/src/actors/project_buffer.rs

…for the key left

jjbayer

Looks good! There are still some open questions & suggestions, but none of them are blocking from my point of view.

relay-config/src/config.rs

relay-server/src/actors/project_cache.rs

relay-server/src/actors/project_buffer.rs

iker-barriocanal

The PR looks good to me, I'd address the order topic before deploying, in this conversation.

relay-server/src/actors/project_cache.rs

relay-server/src/actors/project_buffer.rs

* master: ref(buffering): Add BufferService with SQLite backend (#1920)

* master: ref(buffering): Add BufferService with SQLite backend (#1920) ref(server): Move from actix-web to axum (#1938) ref(tx-processor): Apply transaction rules after UUID scrubbing (#1964)

olksdr requested a review from a team March 13, 2023 08:00

olksdr self-assigned this Mar 13, 2023

olksdr added 4 commits March 13, 2023 09:23

Add changelog entry and fix lints

baa323e

Add path to db in tmp

825ffc2

sorted relay-server deps

d3a5dc6

Merge branch 'master' into feat/buff-with-lite

ddf87c0

olksdr closed this Mar 13, 2023

olksdr deleted the feat/buff-with-lite branch March 13, 2023 14:30

olksdr restored the feat/buff-with-lite branch March 13, 2023 14:30

olksdr reopened this Mar 13, 2023

olksdr added 2 commits March 20, 2023 16:32

Merge branch 'master' into feat/buff-with-lite

76c102a

Post master merge fixes

7c4f618

olksdr force-pushed the feat/buff-with-lite branch from 69266f2 to 7c4f618 Compare March 20, 2023 15:52

jjbayer reviewed Mar 20, 2023

View reviewed changes

iker-barriocanal reviewed Mar 21, 2023

View reviewed changes

olksdr added 11 commits March 22, 2023 10:57

Merge branch 'master' into feat/buff-with-lite

fe3292d

Addressed some PR review comments

601f947

Merge branch 'master' into feat/buff-with-lite

5a1c17b

Cleanup

60902f2

Extend the config and use BufferPermit

e46b14f

Better error messages

94ceb7a

ref: use one service for in-memory and on disk buffering

de8b39e

Merge branch 'master' into feat/buff-with-lite

74ec158

Fix changelog

dbb3b3b

ref: remove unwrap, add another config option

cb4e5c0

Set limit on batch inserts.

4caed2e

olksdr requested review from jjbayer, iker-barriocanal and a team March 23, 2023 09:42

Request permits per key

c7d4de7

jjbayer reviewed Mar 23, 2023

View reviewed changes

olksdr added 3 commits March 23, 2023 18:05

Review comments

1fd521b

Address more review comments

4a176fd

match on the reuest size

d688a49

jjbayer reviewed Mar 24, 2023

View reviewed changes

olksdr added 7 commits March 24, 2023 14:41

ref: build in the migrations into the binary

54d4c0a

reorganize the config

fcd6740

just use the provided path

4356295

Merge branch 'master' into feat/buff-with-lite

8f73ccc

Add docs and enabled shared cache

ecd339c

Merge branch 'master' into feat/buff-with-lite

97df208

remove un-used imports

b92449e

iker-barriocanal reviewed Mar 27, 2023

View reviewed changes

review comments: fetch all envelopes from disk till there is nothing …

6bc057b

…for the key left

jjbayer approved these changes Mar 28, 2023

View reviewed changes

olksdr added 4 commits March 28, 2023 09:56

Merge branch 'master' into feat/buff-with-lite

28601e4

Review comments

b038237

Chunk up the envelopes with future streams

05050ce

Merge branch 'master' into feat/buff-with-lite

d6d8d06

iker-barriocanal reviewed Mar 28, 2023

View reviewed changes

relay-server/src/actors/project_cache.rs Outdated Show resolved Hide resolved

relay-server/src/actors/project_buffer.rs Outdated Show resolved Hide resolved

olksdr added 4 commits March 29, 2023 07:46

fix docs

2a7e01f

Merge branch 'master' into feat/buff-with-lite

47f32ac

Switch to fetch using Stream to query db

69fce70

small checks and logging

c6a8714

olksdr enabled auto-merge (squash) March 30, 2023 08:27

olksdr merged commit e67f343 into master Mar 30, 2023

olksdr deleted the feat/buff-with-lite branch March 30, 2023 09:16

jan-auer added a commit that referenced this pull request Mar 30, 2023

Merge branch 'master' into build/upgrade-crypto

b104455

* master: ref(buffering): Add BufferService with SQLite backend (#1920)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(buffering): Add BufferService with SQLite backend #1920

ref(buffering): Add BufferService with SQLite backend #1920

olksdr commented Mar 13, 2023 •

edited

Loading

jjbayer left a comment

jjbayer Mar 20, 2023

olksdr Mar 22, 2023

jjbayer Mar 23, 2023

iker-barriocanal left a comment

iker-barriocanal Mar 21, 2023

jjbayer Mar 23, 2023

jjbayer left a comment

jjbayer left a comment

iker-barriocanal left a comment

ref(buffering): Add BufferService with SQLite backend #1920

ref(buffering): Add BufferService with SQLite backend #1920

Conversation

olksdr commented Mar 13, 2023 • edited Loading

jjbayer left a comment

Choose a reason for hiding this comment

jjbayer Mar 20, 2023

Choose a reason for hiding this comment

olksdr Mar 22, 2023

Choose a reason for hiding this comment

jjbayer Mar 23, 2023

Choose a reason for hiding this comment

iker-barriocanal left a comment

Choose a reason for hiding this comment

iker-barriocanal Mar 21, 2023

Choose a reason for hiding this comment

jjbayer Mar 23, 2023

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

iker-barriocanal left a comment

Choose a reason for hiding this comment

olksdr commented Mar 13, 2023 •

edited

Loading