Add topic routing and rate limiting #34

zzirnheld-mia · 2024-08-20T14:59:39Z

Short Summary of the Issue

Currently, the MiddleMail instance can end up clogging the SMTP server by sending too many bulk emails before any transactional emails can get through.

Changes made to Resolve the Issue

Added the ability to set a topic string, so a given MiddleMail instance can be tuned to listen for a specific type of email messages.
Added the ability to set a rate limit per minute, so that if two MiddleMail instances send mails to the same SMTP server, we can limit the number of a given type of email that reaches the SMTP server in a given timespan.

Instructions for Testers

Once this has been merged, please test the following:

Create multiple MiddleMail server instances with different topics
- Verify that, as long as the two instances have different subscription IDs and different topics, emails sent to one topic only make it through the corresponding MiddleMail instance.
Use a rate limited MiddleMail instance
- Configure the instance with a specified number of emails per minute
- Attempt to send more than the limit, and verify that that limit many emails are sent out each minute, until the attempted number are sent.

Checklist

Tested with a unit test.
Tested locally.
Checked for typos.
I have made sure that the Instructions for Testers are self-contained. Just by reading the "Instructions for Testers" it is clear what needs to be tested without consulting this Merge Request or the Issue.
I have self-reviewed the Description and the Changes of this Merge Request, usually at least one day after making the latest changes here.
Has been approved by a developer.
Has been approved by a reviewer.

tests/MiddleMailServiceTests.cs

src/MiddleMail.MessageSource.RabbitMQ/RabbitMQMessageSource.cs

src/MiddleMail/MiddleMailService.cs

leo-labs · 2024-09-10T14:15:06Z

src/MiddleMail.Delivery.Smtp/MiddleMail.Delivery.Smtp.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFramework>net6</TargetFramework>
+    <TargetFramework>net7</TargetFramework>


I believe net7 is out of support since May this year. If we upgrade we should directly go to net8.

leo-labs · 2024-09-10T14:15:14Z

src/MiddleMail.MessageSource.RabbitMQ/MiddleMail.MessageSource.RabbitMQ.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
-    <TargetFramework>net6</TargetFramework>
+    <TargetFramework>net7</TargetFramework>


src/MiddleMail.MessageSource.RabbitMQ/RabbitMQMessageSourceOptions.cs

src/MiddleMail.Client.RabbitMQ/RabbitMQOptions.cs

leo-labs · 2024-09-10T14:17:45Z

src/MiddleMail.Server/MiddleMail.Server.csproj

@@ -2,7 +2,7 @@

  <PropertyGroup>
    <OutputType>Exe</OutputType>
-    <TargetFramework>net6</TargetFramework>
+    <TargetFramework>net7</TargetFramework>


tests/MiddleMailServiceTests.cs

leo-labs · 2024-10-01T14:42:45Z

tests/MiddleMailServiceTests.cs

+			// and change the number of verified sent emails to 4.
+			// DateTime before = DateTime.Now;
+			var notSentMessage = FakerFactory.EmailMessageFaker.Generate();
+			await Task.WhenAny(rateLimitedCallback(notSentMessage), Task.Delay(TimeSpan.FromSeconds(5)));


I think that this test could be more written in a more readable way.

What part of how it's written confuses you? The concept of the test is as follows:
3 times:

send a message

verify that the message is delivered

one more time:

initiate sending a message

wait less time than the rate limit window

verify that the message is not delivered

In other words, where should I add a comment?

I believe it would be already good if you move the commented out things that test a separate thing into a different method and separate the tests into Arrange / Act / Assert sections.
Anyway, I don't think you really need to test that the ratelimiter works, but just that it is called correctly and that you behave correctly depending on the lease (wait or process).

Looking at this again now, I still think this test is helpful to check that the behavior works as intended, even if it doesn't fit into the canonical Arrange / Act / Assert sections. I've separated the test into two tests, one of which could be commented out to speed up manual retesting if necessary.

leo-labs · 2024-10-01T14:45:15Z

tests/MiddleMailServiceTests.cs

+				rateLimitedProcessorMock.Verify(m => m.ProcessAsync(It.IsAny<EmailMessage>()), Times.Exactly(i + 1));
+			}
+
+			// If you want to test that it takes the correct amount of time to delay,


one test method should only represent one test.

Yes, this method represents testing that, if you send 4 messages with a rate limit of 3, it allows exactly 3 messages through, not 4

my comment here was about the commented out test code:

If you want to test that it takes the correct amount of time to delay

That would be a different test.

leo-labs · 2024-10-01T14:45:52Z

tests/MiddleMailServiceTests.cs

+
+			// If you want to test that it takes the correct amount of time to delay,
+			// uncomment each commented line,
+			// and change the number of verified sent emails to 4.


Can we not just make this a configuration option, so we can actually have a working automated test?

I can do that. What do you mean by "configuration option"?

You can make this part of the options, so you can inject it and thus control it during tests.

leo-labs · 2024-10-02T10:31:51Z

src/MiddleMail/MiddleMailService.cs

 			this.processor = processor;
 			this.logger = logger;
 			this.consumerTasksPending = 0;
 			this.messageSource = messageSource;
+			this.options = options.Value;
+
+			if (this.options.RateLimited) {


For a better integration with DI you can have a look at https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/RateLimiting/src/RateLimitingMiddleware.cs and https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/RateLimiting/src/RateLimiterOptions.cs.

If you do it like that, you are probably able to improve the tests significantly, because you can mock the rate limiter :)

I took a look at the examples you linked, but I can't understand how GlobalLimiter gets assigned in RateLimiterOptions. However it's assigned in those examples, we need to have the value be populated from environment variables for this program. Do you know how to do that with something like an object (as opposed to a string)?

Have you seen https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit?view=aspnetcore-8.0#fixed-window-limiter and https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.ratelimiting.ratelimiteroptionsextensions.addfixedwindowlimiter?view=aspnetcore-8.0

What they do is to use the Options system to inject and construct a rate limiter. You can do the same here. E.g. like this:

services.AddOptions<RateLimiterOptions>() .Configure(options => options.RateLimiter = new FixedWindowRateLimiter( new FixedWindowRateLimiterOptions() { PermitLimit = options.LimitPerMinute, Window = TimeSpan.FromMinutes(1), }) ); public class RateLimiterOptions { public RateLimiter RateLimiter { get; set;} }

This also allows you to mock your RateLimiter when testing

What's the correct way to access options like MiddleMailOptions in that have just been configured? (To get the LimitPerMinute)

Never mind, I think I understand, the LimitPerMinute is accessed from the same options the RateLimiter is defined on.

My snippet might not be perfect. I probably would not mix options that are (mis?-)used as a wrapper for DI and for configuration binding, since configuration can actually trigger a recreation of the option object.
There are multiple ways of solving this: You could have an option type for your configuration (so it is typed) and then use a different Configure overload when configuring RateLimiterOptions as above, the one where you can inject other services and inject the other option. Another way is to not use an option for the ratelimiter but just a singleton RateLimitAccessor service with a public property to access the ratelimiter object.

leo-labs · 2024-10-03T12:51:37Z

src/MiddleMail/MiddleMailService.cs

 			this.processor = processor;
 			this.logger = logger;
 			this.consumerTasksPending = 0;
 			this.messageSource = messageSource;
+			this.options = options.Value;
+
+			if (this.options.RateLimited) {


Have you seen https://learn.microsoft.com/en-us/aspnet/core/performance/rate-limit?view=aspnetcore-8.0#fixed-window-limiter and https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.ratelimiting.ratelimiteroptionsextensions.addfixedwindowlimiter?view=aspnetcore-8.0

src/MiddleMail/MiddleMailService.cs

leo-labs · 2024-10-03T13:41:05Z

tests/MiddleMailServiceTests.cs

+				rateLimitedProcessorMock.Verify(m => m.ProcessAsync(It.IsAny<EmailMessage>()), Times.Exactly(i + 1));
+			}
+
+			// If you want to test that it takes the correct amount of time to delay,


my comment here was about the commented out test code:

If you want to test that it takes the correct amount of time to delay

That would be a different test.

leo-labs · 2024-10-03T17:52:28Z

tests/MiddleMailServiceTests.cs

+
+			// If you want to test that it takes the correct amount of time to delay,
+			// uncomment each commented line,
+			// and change the number of verified sent emails to 4.


You can make this part of the options, so you can inject it and thus control it during tests.

leo-labs · 2024-10-03T18:05:05Z

tests/MiddleMailServiceTests.cs

+			// and change the number of verified sent emails to 4.
+			// DateTime before = DateTime.Now;
+			var notSentMessage = FakerFactory.EmailMessageFaker.Generate();
+			await Task.WhenAny(rateLimitedCallback(notSentMessage), Task.Delay(TimeSpan.FromSeconds(5)));


I believe it would be already good if you move the commented out things that test a separate thing into a different method and separate the tests into Arrange / Act / Assert sections.
Anyway, I don't think you really need to test that the ratelimiter works, but just that it is called correctly and that you behave correctly depending on the lease (wait or process).

leo-labs · 2025-02-17T18:57:30Z

src/MiddleMail.Client.RabbitMQ/MiddleMailClient.cs

+				if (topic != null) {
+					await bus.PublishAsync(emailMessage, topic: topic);
+				}
+				else {


What about the formatting do you want changed here?

We place else { on the same line as the closing bracket from the if clause, so } else {.

src/MiddleMail/MiddleMailService.cs

src/MiddleMail/MiddleMailOptions.cs

tests/MiddleMailServiceTests.cs

leo-labs · 2025-03-01T13:52:14Z

src/MiddleMail/MiddleMailService.cs

 			this.processor = processor;
 			this.logger = logger;
 			this.consumerTasksPending = 0;
 			this.messageSource = messageSource;
+			this.options = options.Value;
+
+			if (this.options.RateLimited) {


What they do is to use the Options system to inject and construct a rate limiter. You can do the same here. E.g. like this:

services.AddOptions<RateLimiterOptions>() .Configure(options => options.RateLimiter = new FixedWindowRateLimiter( new FixedWindowRateLimiterOptions() { PermitLimit = options.LimitPerMinute, Window = TimeSpan.FromMinutes(1), }) ); public class RateLimiterOptions { public RateLimiter RateLimiter { get; set;} }

This also allows you to mock your RateLimiter when testing

leo-labs · 2025-03-06T17:16:59Z

src/MiddleMail.Server/Program.cs

+					services.AddOptions<MiddleMailOptions>()
+						.Bind(hostContext.Configuration.GetSection(MiddleMailOptions.SECTION))
+						.ValidateDataAnnotations()
+						.Configure(options =>


Have you read #34 (comment) where I mentioned that mixing configuration binding and configure is probably not safe to do?

leo-labs · 2025-03-06T17:18:47Z

src/MiddleMail/MiddleMailOptions.cs

+		/// of mails should be sent each minute.
+		/// </summary>
+		[Required]
+		public bool RateLimited { get; set; }


Do you still need this?

I was under the impression we wanted to maintain backwards-compatibility, so we wanted the option to not be rate-limited. Are you suggesting I should make the LimitPerMinute nullable instead, and treat a null value as "no rate limit"? I don't like that option, since it ascribes a meaning to a null value

I guess we're not explicitly setting null, just using the lack of setting the value. It's certainly convenient

we wanted the option to not be rate-limited
Yes, we do. But at the moment you do not have that (you are always register a ratelimiter) plus you do not use RateLimited anywhere

leo-labs · 2025-03-06T17:20:03Z

src/MiddleMail.Server/Program.cs

+						.Bind(hostContext.Configuration.GetSection(MiddleMailOptions.SECTION))
+						.ValidateDataAnnotations()
+						.Configure(options =>
+							options.RateLimiter = new FixedWindowRateLimiter(


Do you always want to configure a rate limiter?

leo-labs · 2025-03-06T17:21:25Z

src/MiddleMail/MiddleMailOptions.cs

+		/// If <see cref="RateLimited"/> behavior is enabled,
+		/// no more than this many mails will be sent each minute.
+		/// </summary>
+		public int LimitPerMinute { get; set; }


Maybe you should make this nullable instead and null meaning no rate limit, feels like the best UX to me..

leo-labs · 2025-03-06T17:22:48Z

src/MiddleMail/MiddleMailService.cs

@@ -1,10 +1,14 @@
+#nullable enable


I noticed that you have not added that to MiddleMailOptions.cs. Can you remove it here and set it on the project level?

leo-labs · 2025-03-06T17:33:12Z

src/MiddleMail/MiddleMailService.cs

+
+		public override void Dispose() {
+			base.Dispose();
+			rateLimiter?.Dispose();


I don't think that this is a good pattern (disposing an object that is managed by some other object (that is in our case even managed by DI)).
Microsoft/AspNet is a bit sloppy here (https://github.com/dotnet/aspnetcore/blob/main/src/Middleware/RateLimiting/src/RateLimiterOptions.cs#L26C49-L26C62) and does not dispose their rate limiter. On the other hand one can argue that stuff that has the same lifetime as the application does not necessarily need to be disposed of.
The cleanest solution is probably implementing a RateLimitAccessor : IDisposable singleton service. See also https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines#design-services-for-dependency-injection

leo-labs · 2025-03-06T17:38:17Z

tests/MiddleMailServiceTests.cs

+				.Returns(lease.Object);
+
+			async ValueTask<RateLimitLease> simulateAwaitingLease() {
+				await Task.Delay(10000);


Can you make this more explicit, that this is longer than delayBeforeCheckingIfEmailSent?

I mean, why not do (2 * delayBeforeCheckingIfEmailSent).TotalMilliseconds, if that is what you intended here? :)

leo-labs

To be able to deploy these changes, you also need to make a few changes to the chart. Are you planning to do those in this MR?

leo-labs · 2025-03-06T18:54:07Z

src/MiddleMail/MiddleMailService.cs

@@ -13,18 +15,23 @@ namespace MiddleMail {
 	/// <see cref="IMessageProcess" />.
 	/// Implements a graceful shutdown by waiting for all processing tasks to finish work if cancellation is requested.
 	/// </summary>
-	public class MiddleMailService : BackgroundService {
+	public sealed class MiddleMailService : BackgroundService {
+		public static readonly TimeSpan RateLimitWindow = TimeSpan.FromMinutes(1);


Should this really be here?

leo-labs · 2025-03-06T18:56:53Z

src/MiddleMail/RateLimiterAccessor.cs

+using Microsoft.Extensions.Options;
+
+namespace MiddleMail {
+	public class RateLimiterAccessor : IRateLimiterAccessor {


This should probably implement IDisposable then?!

zzirnheld-mia · 2025-03-06T20:30:38Z

To be able to deploy these changes, you also need to make a few changes to the chart. Are you planning to do those in this MR?

Are there changes necessary to deploy without rate limiting or topics? If so, what are they? (I'm not sure what I'm missing)

leo-labs · 2025-03-13T14:24:38Z

Are there changes necessary to deploy without rate limiting or topics? If so, what are they? (I'm not sure what I'm missing)

I don't think so. I was referring to necessary changes to the chart when you actually want to deploy this with rate limiting and different topics (because right now you cannot set these configuration options).

leo-labs · 2025-03-13T14:19:15Z

src/MiddleMail/RateLimiterAccessor.cs

+
+namespace MiddleMail {
+	public class RateLimiterAccessor : IRateLimiterAccessor, IDisposable {
+		public static readonly TimeSpan RateLimitWindow = TimeSpan.FromMinutes(1);


Is there any reason why this is public?

leo-labs · 2025-03-13T14:22:18Z

tests/MiddleMailServiceTests.cs

+				.Returns(lease.Object);
+
+			async ValueTask<RateLimitLease> simulateAwaitingLease() {
+				await Task.Delay(10000);


I mean, why not do (2 * delayBeforeCheckingIfEmailSent).TotalMilliseconds, if that is what you intended here? :)

leo-labs · 2025-03-13T16:02:10Z

src/MiddleMail/RateLimiterAccessor.cs

@@ -4,7 +4,7 @@

 namespace MiddleMail {
 	public class RateLimiterAccessor : IRateLimiterAccessor, IDisposable {
-		public static readonly TimeSpan RateLimitWindow = TimeSpan.FromMinutes(1);
+		private static readonly TimeSpan RateLimitWindow = TimeSpan.FromMinutes(1);


if you make it private, it should not start with an uppercase letter anymore

…ingly

…ocess of being canceled

… dotnet 6 applications

…ts the full rate limiting behavior

…ed on startup, eliminate unnecessary delay on acquiring lease

…es use of Protected()

…ove RateLimited boolean flag in favor of nullable int for LimitPerMinute

leo-labs · 2025-03-24T14:56:06Z

@zzirnheld-mia This looks good to me 🚀
What's left to do is:

Add the new config options to the readme
Bump package, and chart versions. (Maybe wait for Provide cc support #38 to be merged first)

zzirnheld-mia marked this pull request as ready for review August 22, 2024 14:12

zzirnheld-mia commented Aug 22, 2024

View reviewed changes

tests/MiddleMailServiceTests.cs Show resolved Hide resolved

SashaKorniichuk reviewed Aug 29, 2024

View reviewed changes

src/MiddleMail.MessageSource.RabbitMQ/RabbitMQMessageSource.cs Outdated Show resolved Hide resolved

src/MiddleMail/MiddleMailService.cs Outdated Show resolved Hide resolved

SashaKorniichuk approved these changes Sep 2, 2024

View reviewed changes

leo-labs requested changes Oct 2, 2024

View reviewed changes

leo-labs requested changes Oct 3, 2024

View reviewed changes

zzirnheld-mia mentioned this pull request Dec 3, 2024

Document options properties in RabbitMQMessageSourceOptions and RabbitMQOptions #36

Open

zzirnheld-mia requested a review from leo-labs December 10, 2024 17:37

leo-labs requested changes Mar 1, 2025

View reviewed changes

zzirnheld-mia requested a review from leo-labs March 4, 2025 20:58

leo-labs requested changes Mar 6, 2025

View reviewed changes

zzirnheld-mia requested a review from leo-labs March 6, 2025 18:40

leo-labs requested changes Mar 6, 2025

View reviewed changes

leo-labs reviewed Mar 6, 2025

View reviewed changes

zzirnheld-mia requested a review from leo-labs March 6, 2025 19:13

leo-labs requested changes Mar 13, 2025

View reviewed changes

zzirnheld-mia requested a review from leo-labs March 13, 2025 15:48

leo-labs reviewed Mar 13, 2025

View reviewed changes

zzirnheld-mia added 3 commits March 13, 2025 14:25

Add options for rate limiting; upgrade targeted Csharp version accord…

219e132

…ingly

Make rate limiting work as intended

8f8da26

Don't pass the cancelation token if we're in a catch block, in the pr…

d37a4d8

…ocess of being canceled

zzirnheld-mia force-pushed the AddTopicRouting branch from 821ae71 to 91f3ff1 Compare March 13, 2025 19:26

zzirnheld-mia added 6 commits March 13, 2025 14:27

Finalize topic as string, limit as per minute

31796b3

Change client and model back to dotnet 6 for compatibility with other…

caed18f

… dotnet 6 applications

Add unit test with optional test that runs for a full minute, but tes…

5bf5084

…ts the full rate limiting behavior

Add block comment explaining acquireLease

34136e3

Upgrade from .net 6.0->7.0->8.0

0c425f7

Separate test that was doing too much into two separate tests

f6912aa

zzirnheld-mia added 6 commits March 13, 2025 14:27

Dispose correctly of the fixed window rate limiter

06462ad

Allow null topic for backwards-compatibility

ae69dde

Address code review feedback - use DI to inject rate limiter configur…

3dc5ec1

…ed on startup, eliminate unnecessary delay on acquiring lease

Review feedback: Make unit test used mocked RateLimiter, which requir…

16c0787

…es use of Protected()

Move rate limiter to singleton service for easy DI-based testing, rem…

c5ae807

…ove RateLimited boolean flag in favor of nullable int for LimitPerMinute

Add support for rate limit and topic env vars to helm yaml files

4897fed

zzirnheld-mia force-pushed the AddTopicRouting branch from 91f3ff1 to 4897fed Compare March 13, 2025 19:27

Add topic routing and rate limiting #34

Are you sure you want to change the base?

Add topic routing and rate limiting #34

Conversation

zzirnheld-mia commented Aug 20, 2024 • edited Loading

Short Summary of the Issue

Changes made to Resolve the Issue

Instructions for Testers

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo-labs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzirnheld-mia commented Mar 6, 2025 • edited Loading

leo-labs commented Mar 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo-labs commented Mar 24, 2025

zzirnheld-mia commented Aug 20, 2024 •

edited

Loading

zzirnheld-mia commented Mar 6, 2025 •

edited

Loading