From 426a9425f1f34f0a7f59232aea5974bd9ce52ad5 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Fri, 3 Aug 2018 11:33:22 +0200 Subject: [PATCH] SONAR-11077 add statistics to CE SendIssueNotificationsStep --- .../step/SendIssueNotificationsStep.java | 45 +++++++++++++------ .../notification/NotificationService.java | 12 +++-- .../email/EmailNotificationChannel.java | 13 +++--- .../notification/NotificationChannelTest.java | 3 +- .../email/EmailNotificationChannelTest.java | 13 ++++-- .../server/notification/ws/AddActionTest.java | 3 +- .../notification/ws/ListActionTest.java | 3 +- .../notification/ws/RemoveActionTest.java | 3 +- .../notifications/NotificationChannel.java | 2 +- 9 files changed, 66 insertions(+), 31 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 isOnLeakPredicate = i -> i.isNew() && i.creationDate().getTime() >= truncateToSeconds(analysisDate); NewIssuesStatistics newIssuesStats = new NewIssuesStatistics(isOnLeakPredicate); try (CloseableIterator 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 issues, Component project) { + private void processIssues(NewIssuesStatistics newIssuesStats, CloseableIterator 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 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 recipients) { + private static int dispatch(Notification notification, SetMultimap recipients) { + int count = 0; for (Map.Entry> entry : recipients.asMap().entrySet()) { String username = entry.getKey(); Collection 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 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 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; } } } diff --git a/sonar-plugin-api/src/main/java/org/sonar/api/notifications/NotificationChannel.java b/sonar-plugin-api/src/main/java/org/sonar/api/notifications/NotificationChannel.java index f48a78c56ec..a3ed650cc2b 100644 --- a/sonar-plugin-api/src/main/java/org/sonar/api/notifications/NotificationChannel.java +++ b/sonar-plugin-api/src/main/java/org/sonar/api/notifications/NotificationChannel.java @@ -56,7 +56,7 @@ public abstract class NotificationChannel { * @param notification the notification to deliver * @param userlogin the login of the user who should receive the notification */ - public abstract void deliver(Notification notification, String userlogin); + public abstract boolean deliver(Notification notification, String userlogin); @Override public String toString() { -- 2.39.5