SONAR-11077 add statistics to CE SendIssueNotificationsStep

8 files changed, 65 insertions, 30 deletions

diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/step/SendIssueNotificationsStep.java b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/step/SendIssueNotificationsStep.java
index eb3f044c9e3..c4243867050 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/step/SendIssueNotificationsStep.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/computation/task/projectanalysis/step/SendIssueNotificationsStep.java
@@ -30,6 +30,7 @@ import java.util.Set;
 import java.util.function.Predicate;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.utils.Duration;
+import org.sonar.api.utils.log.Loggers;
 import org.sonar.core.issue.DefaultIssue;
 import org.sonar.core.util.CloseableIterator;
 import org.sonar.server.computation.task.projectanalysis.analysis.AnalysisMetadataHolder;
@@ -83,22 +84,24 @@ public class SendIssueNotificationsStep implements ComputationStep {
 
   @Override
   public void execute() {
+    NotificationStatistics statistics = new NotificationStatistics();
     Component project = treeRootHolder.getRoot();
     if (service.hasProjectSubscribersForTypes(project.getUuid(), NOTIF_TYPES)) {
-      doExecute(project);
+      doExecute(project, statistics);
     }
+    statistics.log();
   }
 
-  private void doExecute(Component project) {
+  private void doExecute(Component project, NotificationStatistics statistics) {
     long analysisDate = analysisMetadataHolder.getAnalysisDate();
     Predicate<DefaultIssue> isOnLeakPredicate = i -> i.isNew() && i.creationDate().getTime() >= truncateToSeconds(analysisDate);
     NewIssuesStatistics newIssuesStats = new NewIssuesStatistics(isOnLeakPredicate);
     try (CloseableIterator<DefaultIssue> issues = issueCache.traverse()) {
-      processIssues(newIssuesStats, issues, project);
+      processIssues(newIssuesStats, issues, project, statistics);
     }
     if (newIssuesStats.hasIssuesOnLeak()) {
-      sendNewIssuesNotification(newIssuesStats, project, analysisDate);
-      sendNewIssuesNotificationToAssignees(newIssuesStats, project, analysisDate);
+      sendNewIssuesNotification(newIssuesStats, project, analysisDate, statistics);
+      sendNewIssuesNotificationToAssignees(newIssuesStats, project, analysisDate, statistics);
     }
   }
 
@@ -112,27 +115,28 @@ public class SendIssueNotificationsStep implements ComputationStep {
     return Date.from(instant).getTime();
   }
 
-  private void processIssues(NewIssuesStatistics newIssuesStats, CloseableIterator<DefaultIssue> issues, Component project) {
+  private void processIssues(NewIssuesStatistics newIssuesStats, CloseableIterator<DefaultIssue> issues, Component project, NotificationStatistics statistics) {
     while (issues.hasNext()) {
       DefaultIssue issue = issues.next();
       if (issue.isNew() && issue.resolution() == null) {
         newIssuesStats.add(issue);
       } else if (issue.isChanged() && issue.mustSendNotifications()) {
-        sendIssueChangeNotification(issue, project);
+        sendIssueChangeNotification(issue, project, statistics);
       }
     }
   }
 
-  private void sendIssueChangeNotification(DefaultIssue issue, Component project) {
+  private void sendIssueChangeNotification(DefaultIssue issue, Component project, NotificationStatistics statistics) {
     IssueChangeNotification changeNotification = new IssueChangeNotification();
     changeNotification.setRuleName(rules.getByKey(issue.ruleKey()).getName());
     changeNotification.setIssue(issue);
     changeNotification.setProject(project.getPublicKey(), project.getName(), getBranchName());
     getComponentKey(issue).ifPresent(c -> changeNotification.setComponent(c.getPublicKey(), c.getName()));
-    service.deliver(changeNotification);
+    statistics.issueChangesDeliveries += service.deliver(changeNotification);
+    statistics.issueChanges++;
   }
 
-  private void sendNewIssuesNotification(NewIssuesStatistics statistics, Component project, long analysisDate) {
+  private void sendNewIssuesNotification(NewIssuesStatistics statistics, Component project, long analysisDate, NotificationStatistics notificationStatistics) {
     NewIssuesStatistics.Stats globalStatistics = statistics.globalStatistics();
     NewIssuesNotification notification = newIssuesNotificationFactory
       .newNewIssuesNotication()
@@ -141,10 +145,11 @@ public class SendIssueNotificationsStep implements ComputationStep {
       .setAnalysisDate(new Date(analysisDate))
       .setStatistics(project.getName(), globalStatistics)
       .setDebt(Duration.create(globalStatistics.effort().getOnLeak()));
-    service.deliver(notification);
+    notificationStatistics.newIssuesDeliveries += service.deliver(notification);
+    notificationStatistics.newIssues++;
   }
 
-  private void sendNewIssuesNotificationToAssignees(NewIssuesStatistics statistics, Component project, long analysisDate) {
+  private void sendNewIssuesNotificationToAssignees(NewIssuesStatistics statistics, Component project, long analysisDate, NotificationStatistics notificationStatistics) {
     statistics.getAssigneesStatistics().entrySet()
       .stream()
       .filter(e -> e.getValue().hasIssuesOnLeak())
@@ -161,7 +166,8 @@ public class SendIssueNotificationsStep implements ComputationStep {
           .setStatistics(project.getName(), assigneeStatistics)
           .setDebt(Duration.create(assigneeStatistics.effort().getOnLeak()));
 
-        service.deliver(myNewIssuesNotification);
+        notificationStatistics.myNewIssuesDeliveries += service.deliver(myNewIssuesNotification);
+        notificationStatistics.myNewIssues++;
       });
   }
 
@@ -190,4 +196,17 @@ public class SendIssueNotificationsStep implements ComputationStep {
     return branch.isMain() ? null : branch.getName();
   }
 
+  private static class NotificationStatistics {
+    private int issueChanges = 0;
+    private int issueChangesDeliveries = 0;
+    private int newIssues = 0;
+    private int newIssuesDeliveries = 0;
+    private int myNewIssues = 0;
+    private int myNewIssuesDeliveries = 0;
+
+    private void log() {
+      Loggers.get(SendIssueNotificationsStep.class).debug("newIssuesNotifs={} | newIssuesDeliveries={} | myNewIssuesNotifs={} | myNewIssuesDeliveries={} | changesNotifs={} | changesDeliveries={}",
+        newIssues, newIssuesDeliveries, myNewIssues, myNewIssuesDeliveries, issueChanges, issueChangesDeliveries);
+    }
+  }
 }
diff --git a/server/sonar-server/src/main/java/org/sonar/server/notification/NotificationService.java b/server/sonar-server/src/main/java/org/sonar/server/notification/NotificationService.java
index 06c5b48733d..30c481fd0ae 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/notification/NotificationService.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/notification/NotificationService.java
@@ -59,7 +59,7 @@ public class NotificationService {
     this(dbClient, new NotificationDispatcher[0]);
   }
 
-  public void deliver(Notification notification) {
+  public int deliver(Notification notification) {
     SetMultimap<String, NotificationChannel> recipients = HashMultimap.create();
     for (NotificationDispatcher dispatcher : dispatchers) {
       NotificationDispatcher.Context context = new ContextImpl(recipients);
@@ -70,23 +70,27 @@ public class NotificationService {
         LOG.warn(String.format("Unable to dispatch notification %s using %s", notification, dispatcher), e);
       }
     }
-    dispatch(notification, recipients);
+    return dispatch(notification, recipients);
   }
 
-  private static void dispatch(Notification notification, SetMultimap<String, NotificationChannel> recipients) {
+  private static int dispatch(Notification notification, SetMultimap<String, NotificationChannel> recipients) {
+    int count = 0;
     for (Map.Entry<String, Collection<NotificationChannel>> entry : recipients.asMap().entrySet()) {
       String username = entry.getKey();
       Collection<NotificationChannel> userChannels = entry.getValue();
       LOG.debug("For user {} via {}", username, userChannels);
       for (NotificationChannel channel : userChannels) {
         try {
-          channel.deliver(notification, username);
+          if (channel.deliver(notification, username)) {
+            count++;
+          }
         } catch (Exception e) {
           // catch all exceptions in order to deliver via other channels
           LOG.warn("Unable to deliver notification " + notification + " for user " + username + " via " + channel, e);
         }
       }
     }
+    return count;
   }
 
   @VisibleForTesting
diff --git a/server/sonar-server/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java b/server/sonar-server/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java
index bd6d0246923..c98cad9cfe7 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/notification/email/EmailNotificationChannel.java
@@ -92,17 +92,18 @@ public class EmailNotificationChannel extends NotificationChannel {
   }
 
   @Override
-  public void deliver(Notification notification, String username) {
+  public boolean deliver(Notification notification, String username) {
     User user = userFinder.findByLogin(username);
     if (user == null || StringUtils.isBlank(user.email())) {
       LOG.debug("User does not exist or has no email: {}", username);
-      return;
+      return false;
     }
     EmailMessage emailMessage = format(notification);
     if (emailMessage != null) {
       emailMessage.setTo(user.email());
-      deliver(emailMessage);
+      return deliver(emailMessage);
     }
+    return false;
   }
 
   private EmailMessage format(Notification notification) {
@@ -119,15 +120,17 @@ public class EmailNotificationChannel extends NotificationChannel {
   /**
    * Visibility has been relaxed for tests.
    */
-  void deliver(EmailMessage emailMessage) {
+  boolean deliver(EmailMessage emailMessage) {
     if (StringUtils.isBlank(configuration.getSmtpHost())) {
       LOG.debug("SMTP host was not configured - email will not be sent");
-      return;
+      return false;
     }
     try {
       send(emailMessage);
+      return true;
     } catch (EmailException e) {
       LOG.error("Unable to send email", e);
+      return false;
     }
   }
 
diff --git a/server/sonar-server/src/test/java/org/sonar/server/notification/NotificationChannelTest.java b/server/sonar-server/src/test/java/org/sonar/server/notification/NotificationChannelTest.java
index 6e426d7042c..104bdbffa42 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/notification/NotificationChannelTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/notification/NotificationChannelTest.java
@@ -36,7 +36,8 @@ public class NotificationChannelTest {
 
   private class FakeNotificationChannel extends NotificationChannel {
     @Override
-    public void deliver(Notification notification, String username) {
+    public boolean deliver(Notification notification, String username) {
+      return true;
     }
   }
 
diff --git a/server/sonar-server/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java b/server/sonar-server/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java
index 8a370c9681b..2ab737ea052 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/notification/email/EmailNotificationChannelTest.java
@@ -95,8 +95,9 @@ public class EmailNotificationChannelTest {
       .setTo("user@nowhere")
       .setSubject("Foo")
       .setMessage("Bar");
-    underTest.deliver(emailMessage);
+    boolean delivered = underTest.deliver(emailMessage);
     assertThat(smtpServer.getMessages()).isEmpty();
+    assertThat(delivered).isFalse();
   }
 
   @Test
@@ -108,8 +109,9 @@ public class EmailNotificationChannelTest {
       .setTo("user@nowhere")
       .setSubject("Review #3")
       .setMessage("I'll take care of this violation.");
-    underTest.deliver(emailMessage);
+    boolean delivered = underTest.deliver(emailMessage);
 
+    assertThat(delivered).isTrue();
     List<WiserMessage> messages = smtpServer.getMessages();
     assertThat(messages).hasSize(1);
 
@@ -136,8 +138,9 @@ public class EmailNotificationChannelTest {
       .setTo("user@nowhere")
       .setSubject("Foo")
       .setMessage("Bar");
-    underTest.deliver(emailMessage);
+    boolean delivered = underTest.deliver(emailMessage);
 
+    assertThat(delivered).isTrue();
     List<WiserMessage> messages = smtpServer.getMessages();
     assertThat(messages).hasSize(1);
 
@@ -166,7 +169,9 @@ public class EmailNotificationChannelTest {
       .setTo("user@nowhere")
       .setSubject("Foo")
       .setMessage("Bar");
-    underTest.deliver(emailMessage);
+    boolean delivered = underTest.deliver(emailMessage);
+
+    assertThat(delivered).isFalse();
   }
 
   @Test
diff --git a/server/sonar-server/src/test/java/org/sonar/server/notification/ws/AddActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/notification/ws/AddActionTest.java
index 3f024bd13ee..65820e04c81 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/notification/ws/AddActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/notification/ws/AddActionTest.java
@@ -336,8 +336,9 @@ public class AddActionTest {
     }
 
     @Override
-    public void deliver(Notification notification, String username) {
+    public boolean deliver(Notification notification, String username) {
       // do nothing
+      return true;
     }
   }
 }
diff --git a/server/sonar-server/src/test/java/org/sonar/server/notification/ws/ListActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/notification/ws/ListActionTest.java
index 554b36f2dd2..fa849d0711e 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/notification/ws/ListActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/notification/ws/ListActionTest.java
@@ -300,8 +300,9 @@ public class ListActionTest {
     }
 
     @Override
-    public void deliver(org.sonar.api.notifications.Notification notification, String username) {
+    public boolean deliver(org.sonar.api.notifications.Notification notification, String username) {
       // do nothing
+      return true;
     }
   }
 }
diff --git a/server/sonar-server/src/test/java/org/sonar/server/notification/ws/RemoveActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/notification/ws/RemoveActionTest.java
index f8a25ade4ee..5938f7ce2ea 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/notification/ws/RemoveActionTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/notification/ws/RemoveActionTest.java
@@ -304,8 +304,9 @@ public class RemoveActionTest {
     }
 
     @Override
-    public void deliver(Notification notification, String username) {
+    public boolean deliver(Notification notification, String username) {
       // do nothing
+      return true;
     }
   }
 }