-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(replays): add replays to self hosted #1990
Conversation
1fc598c
to
4c2a020
Compare
head /dev/urandom | tr -dc "a-f0-9" | head -c 32 | ||
) | ||
TIME_IN_SECONDS=$(date +%s) | ||
curl -sf --data '{"event_id":"'"$TEST_REPLAY_ID"'","sdk":{"name":"sentry.javascript.browser","version":"7.38.0"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason the bash syntax of $'
for C escaped strings for newlines wasn't working so just put in the real values
printf "Getting the test replay back" | ||
REPLAY_SEGMENT_PATH="api/0/projects/sentry/internal/replays/$TEST_EVENT_ID/recording-segments/?download" | ||
REPLAY_EVENT_PATH="api/0/projects/sentry/internal/replays/$TEST_EVENT_ID/" | ||
timeout 60 bash -c 'until $(sentry_api_request "$REPLAY_EVENT_PATH" -Isf -X GET -o /dev/null); do printf '.'; sleep 0.5; done' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this test itself will take more than 2 minutes? Is there any way to cut down on that time? I know dev-infra isn't super happy about self-hosted e2e tests taking over 15 minutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm it shouldn't, i just copied the defaults from the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we noticing its increasing the time of the test by two minutes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can lower the timeout here if needed i don't think the tests take more than a few seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I read that wrong, guess I'm still waking up 😅
Awesome work @JoshFerge and the rest of the @getsentry/replay team. Approximately how much more disk space will need to be used for replays? Right now we have a minimum requirement of 20 GB of free space to run self-hosted |
I don't think it meaningfully impacts the amount of default disk space. if they are sending a lot of replays, they'll want to add some more disk space but i assume that's the same with any other usage of the software. |
Yeah I agree with this about the default disk space part. Maybe this comes from me not understanding how replays work, but I assumed that replays would take up a good amount of storage compared to before. Is that the case? If so I'm thinking it might be useful to warn them about this. Reason I say this is we've gotten multiple issues of people filing issues and complaining about disk space usage in the past and it'd be nice to point them to some documentation warning them if it's expected |
replays are about ~10x larger than an error / transaction, but generally with how people use them they send a lot less. so just kind of hard to say. the average replay is ~200KB (will vary on the characteristics of their website), so users can calculate how much extra space they need based on their expected volume. |
Gotcha, thanks for the context 😁. I think it'd be a nice to add that somewhere for our users. Maybe under https://develop.sentry.dev/self-hosted/? Any thoughts @chadwhitacre? |
it seems like we don't really call out resource usage anywhere on this page -- would lean towards not adding it for now from what I see. |
@@ -322,6 +325,9 @@ services: | |||
ingest-consumer: | |||
<<: *sentry_defaults | |||
command: run ingest-consumer --all-consumer-types | |||
ingest-replay-recordings: | |||
<<: *sentry_defaults | |||
command: run ingest-replay-recordings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang 33 services 😭
clickhouse
cron
geoipupdate
ingest-consumer
ingest-replay-recordings
kafka
memcached
nginx
post-process-forwarder-errors
post-process-forwarder-transactions
postgres
redis
relay
sentry-cleanup
smtp
snuba-api
snuba-cleanup
snuba-consumer
snuba-outcomes-consumer
snuba-replacer
snuba-replays-consumer
snuba-sessions-consumer
snuba-subscription-consumer-events
snuba-subscription-consumer-transactions
snuba-transactions-cleanup
snuba-transactions-consumer
subscription-consumer-events
subscription-consumer-transactions
symbolicator
symbolicator-cleanup
web
worker
zookeeper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 snubas 😖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said this would only help with a 33% reduction.
Looks like we need a similar issue for Sentry to consolidate all those query-subscription-consumer
and post-process-forwarder
services.
We've decided to document the changes in the changelog for the next release and include some documentation around resource usage for replays there. Documentation in develop can come later as another issue of migrating README details to the develop docs. PR is good to merge by me |
ingest-replay-recordings
andsnuba-replays-consumer
to the docker-composeingest-replay-recordings
to kafka creation script (ingest-replay-events
is automatically handled by the snuba creation script)also confirmed this works w/ self hosted by running a webpage with our SDK pointed at the DSN of the self hosted server, then seeing it in the UI.