From 36d57a2dc6493ba42c18137a57b026445a238834 Mon Sep 17 00:00:00 2001
From: adamnfish <adam.fisher@guardian.co.uk>
Date: Wed, 22 Jan 2025 14:33:50 +0000
Subject: [PATCH] Improve error handling / logging

Log messages on Failures now include a summary of the underlying
cause, if present. This should improve error visibility across the
board in Security HQ.

We also add additional error handling to the cache service. On every
cache update we include a summary of which accounts have failures in
their cache data.
---
 hq/app/services/CacheService.scala | 39 +++++++++++++++++++++++++++---
 hq/app/utils/attempt/Failure.scala |  3 ++-
 2 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/hq/app/services/CacheService.scala b/hq/app/services/CacheService.scala
index 21230ae2..2a87e689 100644
--- a/hq/app/services/CacheService.scala
+++ b/hq/app/services/CacheService.scala
@@ -120,8 +120,7 @@ class CacheService(
         regions
       )
     } yield {
-      logger.info("Sending the refreshed data to the Credentials Box")
-
+      logCacheDataStatus("Credentials", updatedCredentialReports)
       credentialsBox.send(updatedCredentialReports.toMap)
     }
   }
@@ -135,7 +134,7 @@ class CacheService(
         s3Clients
       )
     } yield {
-      logger.info("Sending the refreshed data to the Public Buckets Box")
+      logCacheDataStatus("Public buckets", allPublicBuckets)
       publicBucketsBox.send(allPublicBuckets.toMap)
     }
   }
@@ -148,7 +147,7 @@ class CacheService(
         taClients
       )
     } yield {
-      logger.info("Sending the refreshed data to the Exposed Keys Box")
+      logCacheDataStatus("Exposed Keys", allExposedKeys)
       exposedKeysBox.send(allExposedKeys.toMap)
     }
   }
@@ -183,4 +182,36 @@ class CacheService(
       Future.successful(())
     }
   }
+
+  /**
+   * Prints an overview of this cache data.
+   *
+   * If everything succeeded then we say as much. If the cache data contains failures
+   * we log a warning that shows which accounts are affected and give one failure
+   * as the underlying cause, if available.
+   */
+  def logCacheDataStatus[A](cacheName: String, data: Seq[(AwsAccount, Either[FailedAttempt, A])]): Unit = {
+    val (successful, failed) = data.partition { case (_, result) => result.isRight }
+
+    if (failed.isEmpty) {
+      logger.info(s"$cacheName updated: All ${data.size} accounts successful")
+    } else {
+      val failedAccountsDetails = failed.flatMap {
+        case (account, Left(failedAttempt)) =>
+          Some(s"${account.name}: ${failedAttempt.logMessage}")
+        case _ => None
+      }.mkString(", ")
+      val logMessage = s"$cacheName updated: ${successful.size}/${data.size} accounts succeeded. Failed accounts: $failedAccountsDetails"
+      failed.flatMap {
+        case (_, Left(failedAttempt)) =>
+          failedAttempt.firstException
+        case _ => None
+      }.headOption match {
+        case None =>
+          logger.warn(logMessage)
+        case Some(exampleCausedBy) =>
+          logger.warn(s"$logMessage - see stacktrace for an example cause", exampleCausedBy)
+      }
+    }
+  }
 }
diff --git a/hq/app/utils/attempt/Failure.scala b/hq/app/utils/attempt/Failure.scala
index db49935c..32d9b6c4 100644
--- a/hq/app/utils/attempt/Failure.scala
+++ b/hq/app/utils/attempt/Failure.scala
@@ -7,7 +7,8 @@ case class FailedAttempt(failures: List[Failure]) {
 
   def logMessage: String = failures.map { failure =>
     val context = failure.context.fold("")(c => s" ($c)")
-    s"${failure.message}$context"
+    val causedBy = firstException.fold("")(err => s" caused by: ${err.getMessage}")
+    s"${failure.message}$context$causedBy"
   }.mkString(", ")
 
   def firstException: Option[Throwable] = {