feat(replays): lower max replay recording segment size to 10 MiB #2031
Conversation
This file contains 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
1 task
olksdr
requested changes
Apr 14, 2023
| @@ -542,8 +542,10 @@ struct Limits { | |||
| max_api_chunk_upload_size: ByteSize, | |||
| /// The maximum payload size for a profile | |||
| max_profile_size: ByteSize, | |||
| /// The maximum payload size for a replay. | |||
| max_replay_size: ByteSize, | |||
There was a problem hiding this comment.
Can we keep old config and use max_replay_size as incoming check, the one is currently called max_replay_compressed_size, which clearly signals what the check is for, and if this is really necessary also use serde alias to rename it to the new name?
What do you think?
There was a problem hiding this comment.
The new naming looks OK to me, but I would add the old name as an alias as @olksdr suggests, such that this is not a breaking change.
jjbayer
reviewed
Apr 17, 2023
| @@ -542,8 +542,10 @@ struct Limits { | |||
| max_api_chunk_upload_size: ByteSize, | |||
| /// The maximum payload size for a profile | |||
| max_profile_size: ByteSize, | |||
| /// The maximum payload size for a replay. | |||
| max_replay_size: ByteSize, | |||
There was a problem hiding this comment.
The new naming looks OK to me, but I would add the old name as an alias as @olksdr suggests, such that this is not a breaking change.
olksdr
reviewed
Apr 18, 2023
olksdr
approved these changes
Apr 19, 2023
JoshFerge
added a commit
that referenced
this pull request
Apr 20, 2023
We were previously chunking kafka messages on the replay recording topic to 1 MiB. We are aiming to eliminate this chunking as it requires us to maintain a redis cluster / constrains us to in-order processing. Other infra at Sentry has successfully experimented with larger kafka messages, so we feel this is safe. With #2031 this should pretty much mean we no longer chunk any messages. We can remove the chunking logic once this is confirmed after deploy. - [x] Increase chunking limit from 1MiB to 15 MiB. We chose 15MiB as the limit, as we're eventually going to combine the replay event with the replay recording, and will want some extra headroom for this in addition to our 10 MiB recording limit. Depends on ops increasing our kafka payload size limit first.
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.
max_replay_compressed) and one for uncompressed sizemax_replay_uncompressed_size. The original value now represents the uncompressed size (100 MiB), which is utilized during processing. The new value,max_replay_compressed, is applied during the ingest stage.10 MiB) setting for check_envelope_size_limits.We are making this change as we have done analysis around our payload sizes, and if replays are above this size there is usually something off with them (mass mutation bugs, accidental inclusion of files, old SDK versions, etc.)