fix(core): Create better task exception summaries (#3979)

spinnaker · Aug 23, 2019 · 8cdb301 · 8cdb301
1 parent 831e118
commit 8cdb301
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 4 deletions.
diff --git a/...oovy/com/netflix/spinnaker/clouddriver/orchestration/DefaultOrchestrationProcessor.groovy b/...oovy/com/netflix/spinnaker/clouddriver/orchestration/DefaultOrchestrationProcessor.groovy
@@ -16,12 +16,14 @@
 
 package com.netflix.spinnaker.clouddriver.orchestration
 
+import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.clouddriver.data.task.Task
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository
 import com.netflix.spinnaker.clouddriver.metrics.TimedCallable
 import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEvent
 import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEventHandler
+import com.netflix.spinnaker.kork.exceptions.ExceptionSummary
 import com.netflix.spinnaker.security.AuthenticatedRequest
 import groovy.util.logging.Slf4j
 import org.slf4j.MDC
@@ -62,6 +64,9 @@ class DefaultOrchestrationProcessor implements OrchestrationProcessor {
   @Autowired(required = false)
   Collection<OperationEventHandler> operationEventHandlers = []
 
+  @Autowired
+  ObjectMapper objectMapper
+
   @Override
   Task process(List<AtomicOperation> atomicOperations, String clientRequestId) {
 
@@ -104,7 +109,7 @@ class DefaultOrchestrationProcessor implements OrchestrationProcessor {
             }.call()
           } catch (AtomicOperationException e) {
             task.updateStatus TASK_PHASE, "Orchestration failed: ${atomicOperation.class.simpleName} | ${e.class.simpleName}: [${e.errors.join(', ')}]"
-            task.addResultObjects([[type: "EXCEPTION", operation: atomicOperation.class.simpleName, cause: e.class.simpleName, message: e.errors.join(", ")]])
+            task.addResultObjects([extractExceptionSummary(e, e.errors.join(", "), [operation: atomicOperation.class.simpleName])])
             task.fail()
           } catch (e) {
             def message = e.message
@@ -116,7 +121,7 @@ class DefaultOrchestrationProcessor implements OrchestrationProcessor {
               message = stackTrace
             }
             task.updateStatus TASK_PHASE, "Orchestration failed: ${atomicOperation.class.simpleName} | ${e.class.simpleName}: [${message}]"
-            task.addResultObjects([[type: "EXCEPTION", operation: atomicOperation.class.simpleName, cause: e.class.simpleName, message: message]])
+            task.addResultObjects([extractExceptionSummary(e, message, [operation: atomicOperation.class.simpleName])])
 
             log.error(stackTrace)
             task.fail()
@@ -131,14 +136,14 @@ class DefaultOrchestrationProcessor implements OrchestrationProcessor {
         registry.counter(tasksId.withTag("success", "false").withTag("cause", e.class.simpleName)).increment()
         if (e instanceof TimeoutException) {
           task.updateStatus "INIT", "Orchestration timed out."
-          task.addResultObjects([[type: "EXCEPTION", cause: e.class.simpleName, message: "Orchestration timed out."]])
+          task.addResultObjects([extractExceptionSummary(e, "Orchestration timed out.")])
           task.fail()
         } else {
           def stringWriter = new StringWriter()
           def printWriter = new PrintWriter(stringWriter)
           e.printStackTrace(printWriter)
           task.updateStatus("INIT", "Unknown failure -- ${stringWriter.toString()}")
-          task.addResultObjects([[type: "EXCEPTION", cause: e.class.simpleName, message: "Failed for unknown reason."]])
+          task.addResultObjects([extractExceptionSummary(e, "Failed for unknown reason.")])
           task.fail()
         }
       } finally {
@@ -172,4 +177,28 @@ class DefaultOrchestrationProcessor implements OrchestrationProcessor {
       log.error("Unable to clear thread locals, reason: ${e.message}")
     }
   }
+
+  /**
+   * For backwards compatibility.
+   *
+   * TODO(rz): Not 100% sure we should keep these two methods.
+   */
+  Map<String, Object> extractExceptionSummary(Throwable e, String userMessage) {
+    def summary = ExceptionSummary.from(e)
+    Map<String, Object> map = objectMapper.convertValue(summary, Map)
+    map["message"] = userMessage
+    map["type"] = "EXCEPTION"
+    return map
+  }
+
+  /**
+   * For backwards compatibility.
+   *
+   * TODO(rz): Add "additionalFields" to ExceptionSummary?
+   */
+  Map<String, Object> extractExceptionSummary(Throwable e, String userMessage, Map<String, Object> additionalFields) {
+    Map<String, Object> summary = extractExceptionSummary(e, userMessage)
+    summary.putAll(additionalFields)
+    return summary
+  }
 }
diff --git a/.../com/netflix/spinnaker/clouddriver/orchestration/DefaultOrchestrationProcessorSpec.groovy b/.../com/netflix/spinnaker/clouddriver/orchestration/DefaultOrchestrationProcessorSpec.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.clouddriver.orchestration
 
+import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spectator.api.Spectator
 import com.netflix.spinnaker.clouddriver.data.task.DefaultTask
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository
@@ -50,6 +51,7 @@ class DefaultOrchestrationProcessorSpec extends Specification {
     processor.applicationContext = applicationContext
     processor.taskRepository = taskRepository
     processor.registry = Spectator.globalRegistry()
+    processor.objectMapper = new ObjectMapper( )
   }
 
   void "complete the task when everything goes as planned"() {