Skip to content
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

ref(cache): Move buffering of pending envelope to ProjectCache #1907

Merged
merged 8 commits into from
Mar 8, 2023

Conversation

olksdr
Copy link
Contributor

@olksdr olksdr commented Mar 7, 2023

Take two as an attempt to fix the problem we had while deploying #1879 which was reverted in #1906


These changes moving buffering of the incoming envelopes in the ProjectCache.

Current implementation still keeps, so called queue in memory and using HashMap with a composite key QueueKey {key, sampling_key}, where sampling_key can be the same as a key if there is no sampling project identified. The values to these keys are Vec of boxed Envelope with their EnvelopeContext.

Once we get an update for project state, we check all variants of QueueKey which contains the current ProjectKey and if all the project states are cached we try to flush buffered envelopes indexed by these QeueuKey.

The envelops will be buffered if:

the project state is not fetched yet
root project is here but the sampling project state is not fetched yet
the sampling project state is here but the root project is not fetched yet
This change also removes all the buffering from the Project and reduces its responsibility. Now it just keeps its own state and configuration and the envelope handling is done outside of it.

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

@olksdr olksdr self-assigned this Mar 7, 2023
@olksdr olksdr marked this pull request as ready for review March 7, 2023 16:10
@olksdr olksdr requested review from a team, jjbayer and jan-auer March 7, 2023 16:10
@olksdr
Copy link
Contributor Author

olksdr commented Mar 7, 2023

Refactor handle_processing a bit and also add more logging and docs in 2389abe

@jjbayer @jan-auer, please, take another look.

///
/// The following pre-conditions must be met before calling this function:
/// - Envelope's project state must be cached and valid.
/// - Optional: if dynamic sampling keys exists, the dyanmic project state must be cached and valid.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - Optional: if dynamic sampling keys exists, the dyanmic project state must be cached and valid.
/// - If dynamic sampling keys exists, the sampling project state must be cached and valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also done in 56f32b8

if let Some(sampling_state) = sampling_state {
if state.organization_id == sampling_state.organization_id {
process.sampling_project_state = Some(sampling_state)
if let Some(project) = self.projects.get_mut(&project_key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would write this as a guard to keep the error handling close to the check, i.e.

let Some(project) = self.projects.get_mut(&project_key) else {
  // log error
  return
};

let Some(own_project_state) = ... else {
   // log error
   return
}

if let Ok(CheckedEnvelope {...}) = ... {
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this can look better. I will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done cdf587a

@olksdr olksdr merged commit 760fbc9 into master Mar 8, 2023
@olksdr olksdr deleted the feat/buff-again branch March 8, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants