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(server): Add a message to capture envelopes in capture mode #1409

Merged
merged 1 commit into from
Aug 11, 2022

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Aug 11, 2022

Relay has a capture mode used for testing. In this mode it does not send envelopes to Kafka or the upstream, but rather keeps them in an internal map. There is a hidden endpoint with which a tester can extract the captured envelopes and inspect them for errors.

Until now, envelopes were all handled and captured by the envelope manager. The final goal is to remove the long-running handler future in EnvelopeManager, which requires to capture rejections from other places than the envelope manager.

This change introduces a message to capture envelopes or rejections from any place in the code base. In subsequent refactors, individual error handlers can send their errors into capture mode using this message.

Follow-Ups

Follow-ups continued from #1406 and #1408:

  • Break down the envelope manager future into an actual pipeline of sequential messages.
  • Combine the envelope context and envelope in a single carrier type. Keeping the two separate required fewer code changes initially, however the context and the envelope always need to be passed and modified together.
  • (optional) Introduce safer APIs to modify the envelope's contents and update the envelope context at the same time.

#skip-changelog

@jan-auer jan-auer self-assigned this Aug 11, 2022
@jan-auer jan-auer force-pushed the ref/capture-message branch from 6647fd7 to ee3ace2 Compare August 11, 2022 13:55
@jan-auer jan-auer marked this pull request as ready for review August 11, 2022 13:56
@jan-auer jan-auer requested a review from a team August 11, 2022 13:56
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

This looks good to me, I have two questions though (see comments).

@@ -369,6 +369,10 @@ where
metric!(counter(RelayCounters::EnvelopeRejected) += 1);
let event_id = *event_id.borrow();

if Capture::should_capture(&config) {
EnvelopeManager::from_registry().do_send(Capture::rejected(event_id, &error));
}
Copy link
Member

Choose a reason for hiding this comment

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

Was this case previously uncaptured? Or did it happen somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was previously not captured, correct. The fast path also bypassed other means of the envelope manager, such as the buffer capacity check.

}

if Capture::should_capture(&self.config) {
context.notify(Capture::accepted(envelope));
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to .notify, or to call .handle() directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I went with notify just because that's how actix documents sending messages to the own actor. Probably calling handle would be more direct or even efficient. The immediate next PR is going to move this to another actor anyway, so calling notify here is closer to target behavior.

@jan-auer jan-auer merged commit 72ea13d into master Aug 11, 2022
@jan-auer jan-auer deleted the ref/capture-message branch August 11, 2022 21:06
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