Improve email sending #6154

zivkan · 2018-07-10T11:44:25Z

Retry failed sends, to improve reliability
Send App Insights telemetry, so we have visibility into the extent of the problem (plus the quantity of messages sent)
NuGetGallery to send emails in the background, so slow/retried emails don't slow HTTP responses

* Retry failed sends, to improve reliability * Send App Insights telemetry, so we have visibility into the extent of the problem (plus the quantity of messages sent) * NuGetGallery to send emails in the background, so slow/retried emails don't slow HTTP responses

scottbommarito

Looks great, welcome to the team!

scottbommarito · 2018-07-10T17:29:58Z

src/NuGetGallery.Core/Services/CoreMessageService.cs

        {
-            SendMessage(mailMessage, copySender: false);
+            int attempt = 0;
+            for (;;)


Not sure exactly what this means. I assume it means "keep looping infinitely".
I'd rather see while (true) than this. Even better, I'd prefer to see this loop rewritten with a clearer condition, e.g. while (attempt < RetryDelays.Count)

Yes, it's an infinite loop. You're right, while (true) is easier to read. I guess some habits die hard.

I'll change the loop to while (!success). The issue with looping until the retry count is exceeded, is that if we want to rethrow the exception and keep the stack trace, we need to keep the exception in a variable, then outside the exception handler do ExceptionDispatchInfo.Capture(exception).Throw(). I think it's much cleaner to just throw; in the exception handler.

shishirx34

Looks good.

shishirx34 · 2018-07-10T17:23:26Z

src/NuGetGallery.Core/Services/CoreMessageService.cs

        {
-            SendMessage(mailMessage, copySender: false);
+            int attempt = 0;
+            for (;;)


we don't really use much infinite loops, but I think a couple of places where we do use while (true), may be use the same for consistency?

loic-sharma · 2018-07-10T18:47:34Z

src/NuGetGallery/Services/MessageService.cs

-                // Log but swallow the exception
-                QuietLog.LogHandledException(ex);
+                sw.Stop();
+                telemetryClient.TrackDependency("SMTP", smtpUri, "SendMessage", null, now, sw.Elapsed, null, success);
            }


The pattern we use for this kind of thing is a TrackMetric, through some sort of descriptive wrapper method. For example: https://github.com/NuGet/NuGet.Jobs/blob/master/src/Validation.PackageSigning.ProcessSignature/Telemetry/TelemetryService.cs#L61

Also, this out of scope for this PR, but we really should merge the Gallery's and ServerCommon's telemetry APIs...

Also, it may be worth tracking whether the operation was a success or not so that we measure how good/bad our mail system is.

In reply to: 201454393 [](ancestors = 201454393)

I wanted to use TrackDependency for a few reasons.

I wanted to have an event which told us if it was successful or failed. Keeping the duration is just a side-effect of using the dependency event type

AI's Smart Monitoring can automatically alert us when failed dependencies increase. Using metrics we would have to create custom alerts, assuming we use App Insights for alerting. Perhaps if we export App Insights logs to something else and do alerting there, using metrics doesn't lose this advantage.

Dependencies are shown in AI's Application Map tab. If all dependencies are recorded in AI, then the application map is an auto-generated, self-updating, service architecture diagram, which is great for newbies like me 😄 (side note, we must be using an old App Insights SDK, because newer ones automatically capture outgoing HTTP calls, including Blob Storage, but ours does not)

Sending emails to a SMTP server really is a service dependency, and not an application specific metric. Therefore, using the dependency event type, rather than metric, seems more semantically correct.

I'm happy to change it to a metric, but those are the reasons I added TrackDependency to ITelemetryClient. Let me know what you think.

Those are very good points! It sounds like TrackDependency is indeed the right pattern here.

loic-sharma

Looks great!

agr · 2018-07-13T19:19:31Z

src/NuGetGallery/Services/BackgroundMessageService.cs

+        {
+            // Send email as background task, as we don't want to delay the HTTP response.
+            // Particularly when sending email fails and needs to be retried with a delay.
+            Task.Run(() =>


I have a concerth that we might run into thread pool depletion if we try to send a lot of email while SMTP server is unresponsive and causes retries. base.SendMessage uses Thread.Sleep which blocks thread preventing other tasks from running on it (including processing the web requests? not sure if asp.net uses the same thread pool as Task.Run for request processing).

agr · 2018-07-13T19:21:19Z

src/NuGetGallery.Core/Services/CoreMessageService.cs

+                    if (attempt < RetryDelays.Count)
+                    {
+                        Thread.Sleep(RetryDelays[attempt]);
+                        attempt++;


We probably need to track the amount of retries we do.

loic-sharma · 2018-07-18T17:43:38Z

Hey @zivkan, it looks like we should make the message service asynchronous to avoid blocking threads during retries. I believe that converting and then testing the code may take 1-2 days. After discussing it with @skofman1, we think it'd be best if I hand off this work back to you for when you return.

…ures

…d message

zivkan · 2018-08-01T23:23:26Z

I pushed changes to address all the comments. Please have another look. Thanks.

…ures # Conflicts: # src/NuGetGallery.Core/Services/CoreMessageService.cs # src/NuGetGallery.Core/Services/ICoreMessageService.cs # src/NuGetGallery/Services/IMessageService.cs # tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs # tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

loic-sharma · 2018-08-02T21:28:54Z

src/NuGetGallery/Controllers/OrganizationsController.cs

@@ -57,13 +57,13 @@ protected override void SendNewAccountEmail(User account)
        {
            var confirmationUrl = Url.ConfirmOrganizationEmail(account.Username, account.EmailConfirmationToken, relativeUrl: false);

-            MessageService.SendNewAccountEmail(account, confirmationUrl);
+            MessageService.SendNewAccountEmailAsync(account, confirmationUrl);
        }


Shouldn't you await this? Same with line 66.

loic-sharma · 2018-08-02T21:29:29Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -892,7 +892,7 @@ private void NotifyReportMyPackageSupportRequest(ReportMyPackageViewModel report
                CopySender = reportForm.CopySender
            };

-            _messageService.ReportMyPackage(request);
+            _messageService.ReportMyPackageAsync(request);



Is this missing an await?

loic-sharma · 2018-08-02T21:29:48Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -989,7 +989,7 @@ public virtual ActionResult ContactOwners(string id, string version, ContactOwne

            var user = GetCurrentUser();
            var fromAddress = new MailAddress(user.EmailAddress, user.Username);
-            _messageService.SendContactOwnersMessage(
+            _messageService.SendContactOwnersMessageAsync(
                fromAddress,


Is this missing an await?

loic-sharma · 2018-08-02T21:30:43Z

src/NuGetGallery/Controllers/UsersController.cs

@@ -1065,7 +1065,7 @@ private ActionResult SendPasswordResetEmail(User user, bool forgotPassword)
                user.PasswordResetToken,
                forgotPassword,
                relativeUrl: false);
-            MessageService.SendPasswordResetInstructions(user, resetPasswordUrl, forgotPassword);
+            MessageService.SendPasswordResetInstructionsAsync(user, resetPasswordUrl, forgotPassword);



Should this be awaited?

loic-sharma · 2018-08-02T21:31:17Z

src/NuGetGallery/Controllers/UsersController.cs

@@ -74,13 +74,13 @@ protected override void SendNewAccountEmail(User account)
        {
            var confirmationUrl = Url.ConfirmEmail(account.Username, account.EmailConfirmationToken, relativeUrl: false);

-            MessageService.SendNewAccountEmail(account, confirmationUrl);
+            MessageService.SendNewAccountEmailAsync(account, confirmationUrl);


SendNewAccountEmailAsync [](start = 27, length = 24)

Should this line and line 83 be awaited?

loic-sharma · 2018-08-03T17:41:59Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -1424,7 +1424,7 @@ private void SendAddPackageOwnerNotification(PackageRegistration package, User n

            // Notify existing owners
            var notNewOwners = package.Owners.Where(notNewOwner).ToList();
-            notNewOwners.ForEach(owner => _messageService.SendPackageOwnerAddedNotice(owner, newOwner, package, packageUrl));
+            notNewOwners.ForEach(owner => _messageService.SendPackageOwnerAddedNoticeAsync(owner, newOwner, package, packageUrl));
        }


I don't think this works with async methods :(

…ures

loic-sharma

Did another pass, looks great! 👍

This reverts commit 2616a99.

* Revert "Don't throw when SmtpUri is not set (#6278)" This reverts commit e60e75f. * Revert "Fix logging exceptions from background thread (#6273)" This reverts commit d1610bb. * Revert "Improve email sending (#6154)" This reverts commit 2616a99.

Improve email sending

0c9f4f6

* Retry failed sends, to improve reliability * Send App Insights telemetry, so we have visibility into the extent of the problem (plus the quantity of messages sent) * NuGetGallery to send emails in the background, so slow/retried emails don't slow HTTP responses

scottbommarito approved these changes Jul 10, 2018

View reviewed changes

shishirx34 approved these changes Jul 10, 2018

View reviewed changes

loic-sharma reviewed Jul 10, 2018

View reviewed changes

loic-sharma approved these changes Jul 10, 2018

View reviewed changes

Change infinite loop with break to loop with condition

9ff2af9

loic-sharma self-assigned this Jul 12, 2018

agr reviewed Jul 13, 2018

View reviewed changes

zivkan added 5 commits July 31, 2018 16:42

Make CoreMessageService, and all usage, async

53bf118

Track send email through telemetry service

7a0b0a2

Merge remote-tracking branch 'origin/dev' into zivkan-retry-smtp-fail…

b6df662

…ures

Clone MailMessage in BackgroundMessageService, to avoid using dispose…

3b07214

…d message

record attempt number for send email telemetry

b59b620

loic-sharma reviewed Aug 2, 2018

View reviewed changes

await all new async methods

7918915

loic-sharma reviewed Aug 3, 2018

View reviewed changes

zivkan added 2 commits August 3, 2018 15:51

IEnumerable<>.ForEach doesn't work with async

164856e

Merge remote-tracking branch 'origin/dev' into zivkan-retry-smtp-fail…

c7d9382

…ures

loic-sharma approved these changes Aug 6, 2018

View reviewed changes

zivkan merged commit 2616a99 into dev Aug 6, 2018

zivkan deleted the zivkan-retry-smtp-failures branch August 6, 2018 21:05

zivkan added a commit that referenced this pull request Aug 9, 2018

Revert "Improve email sending (#6154)"

289c47c

This reverts commit 2616a99.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve email sending #6154

Improve email sending #6154

zivkan commented Jul 10, 2018

scottbommarito left a comment

scottbommarito Jul 10, 2018

zivkan Jul 11, 2018

shishirx34 left a comment

shishirx34 Jul 10, 2018

loic-sharma Jul 10, 2018

loic-sharma Jul 10, 2018

zivkan Jul 11, 2018

loic-sharma Jul 12, 2018 •

edited

Loading

loic-sharma left a comment

agr Jul 13, 2018

agr Jul 13, 2018

loic-sharma commented Jul 18, 2018

zivkan commented Aug 1, 2018

loic-sharma Aug 2, 2018

loic-sharma Aug 2, 2018

loic-sharma Aug 2, 2018

loic-sharma Aug 2, 2018

loic-sharma Aug 2, 2018

loic-sharma Aug 3, 2018 •

edited

Loading

loic-sharma left a comment

Improve email sending #6154

Improve email sending #6154

Conversation

zivkan commented Jul 10, 2018

scottbommarito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shishirx34 left a 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

loic-sharma Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

loic-sharma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma commented Jul 18, 2018

zivkan commented Aug 1, 2018

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

loic-sharma Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

loic-sharma left a comment

Choose a reason for hiding this comment

loic-sharma Jul 12, 2018 •

edited

Loading

loic-sharma Aug 3, 2018 •

edited

Loading