[Self-service error codes] No redundant logging in Ledger API (#11649)

CHANGELOG_BEGIN CHANGELOG_END
digital-asset · Nov 15, 2021 · 8e08450 · 8e08450
1 parent 18433eb
commit 8e08450
Show file tree

Hide file tree

Showing 13 changed files with 73 additions and 78 deletions.
diff --git a/ledger/error/src/main/scala/com/daml/error/ErrorCode.scala b/ledger/error/src/main/scala/com/daml/error/ErrorCode.scala
@@ -4,6 +4,7 @@
 package com.daml.error
 
 import com.daml.error.ErrorCode.{ValidMetadataKeyRegex, truncateResourceForTransport}
+import com.daml.error.definitions.LoggingTransactionErrorImpl
 import com.google.rpc.Status
 import io.grpc.Status.Code
 import io.grpc.StatusRuntimeException
@@ -137,7 +138,15 @@ abstract class ErrorCode(val id: String, val category: ErrorCategory)(implicit
     // Builder methods for metadata are not exposed, so going route via creating an exception
     val ex = StatusProto.toStatusRuntimeException(status)
     // Strip stack trace from exception
-    new ErrorCode.ApiException(ex.getStatus, ex.getTrailers)
+    // TODO error codes: Define a generic mechanism (trait or method) to check if errors are logged on creation,
+    //                   instead of checking every implementation.
+    err match {
+      case _: LoggingTransactionErrorImpl =>
+        new ErrorCode.LoggingApiException(ex.getStatus, ex.getTrailers)
+      case err: BaseError.Impl if err.logOnCreation =>
+        new ErrorCode.LoggingApiException(ex.getStatus, ex.getTrailers)
+      case _ => new ErrorCode.ApiException(ex.getStatus, ex.getTrailers)
+    }
   }
 
   /** log level of the error code
@@ -218,6 +227,9 @@ object ErrorCode {
       extends StatusRuntimeException(status, metadata)
       with NoStackTrace
 
+  class LoggingApiException(status: io.grpc.Status, metadata: io.grpc.Metadata)
+      extends ApiException(status, metadata)
+
   case class StatusInfo(
       codeGrpc: io.grpc.Status.Code,
       message: String,

diff --git a/ledger/error/src/main/scala/com/daml/error/definitions/PackageServiceError.scala b/ledger/error/src/main/scala/com/daml/error/definitions/PackageServiceError.scala
@@ -19,7 +19,14 @@ import com.daml.lf.data.Ref.PackageId
 import com.daml.lf.engine.Error
 import com.daml.lf.{VersionRange, language, validation}
 
-trait PackageServiceError extends BaseError
+abstract class PackageServiceError(
+    override val cause: String,
+    override val throwableO: Option[Throwable] = None,
+)(implicit override val code: ErrorCode)
+    extends BaseError.Impl(cause, throwableO) {
+  final override def logOnCreation: Boolean = true
+}
+
 object PackageServiceError extends PackageServiceErrorGroup {
 
   object Reading extends ErrorGroup {
@@ -35,11 +42,9 @@ object PackageServiceError extends PackageServiceErrorGroup {
         ) {
       final case class Error(reason: String)(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Dar file name is invalid"
           )
-          with PackageServiceError
-
     }
 
     @Explanation("""This error indicates that the supplied dar file was invalid.""")
@@ -48,22 +53,20 @@ object PackageServiceError extends PackageServiceErrorGroup {
         extends ErrorCode(id = "INVALID_DAR", ErrorCategory.InvalidIndependentOfSystemState) {
       final case class Error(entries: Seq[String], throwable: Throwable)(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Dar file is corrupt",
             throwableO = Some(throwable),
           )
-          with PackageServiceError
     }
     @Explanation("""This error indicates that the supplied zipped dar file was invalid.""")
     @Resolution("Inspect the error message for details and contact support.")
     object InvalidZipEntry
         extends ErrorCode(id = "INVALID_ZIP_ENTRY", ErrorCategory.InvalidIndependentOfSystemState) {
       final case class Error(name: String, entries: Seq[String])(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Dar zip file is corrupt"
           )
-          with PackageServiceError
     }
 
     @Explanation(
@@ -77,10 +80,9 @@ object PackageServiceError extends PackageServiceErrorGroup {
         ) {
       final case class Error(entries: Seq[String])(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Unsupported legacy Dar zip file"
           )
-          with PackageServiceError
     }
 
     @Explanation("""This error indicates that the supplied zipped dar is regarded as zip-bomb.""")
@@ -89,10 +91,9 @@ object PackageServiceError extends PackageServiceErrorGroup {
         extends ErrorCode(id = "ZIP_BOMB", ErrorCategory.InvalidIndependentOfSystemState) {
       final case class Error(msg: String)(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Dar zip file seems to be a zip bomb."
           )
-          with PackageServiceError
     }
 
     @Explanation(
@@ -103,10 +104,9 @@ object PackageServiceError extends PackageServiceErrorGroup {
         extends ErrorCode(id = "DAR_PARSE_ERROR", ErrorCategory.InvalidIndependentOfSystemState) {
       final case class Error(reason: String)(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Failed to parse the dar file content."
           )
-          with PackageServiceError
     }
 
   }
@@ -120,35 +120,31 @@ object PackageServiceError extends PackageServiceErrorGroup {
       ) {
     final case class Validation(nameOfFunc: String, msg: String, detailMsg: String = "")(implicit
         val loggingContext: ContextualizedErrorLogger
-    ) extends BaseError.Impl(
+    ) extends PackageServiceError(
           cause = "Internal package validation error."
         )
-        with PackageServiceError
     final case class Error(missing: Set[PackageId])(implicit
         val loggingContext: ContextualizedErrorLogger
-    ) extends BaseError.Impl(
+    ) extends PackageServiceError(
           cause = "Failed to resolve package ids locally."
         )
-        with PackageServiceError
     final case class Generic(reason: String)(implicit
         val loggingContext: ContextualizedErrorLogger
-    ) extends BaseError.Impl(
+    ) extends PackageServiceError(
           cause = "Generic error (please check the reason string)."
         )
-        with PackageServiceError
     final case class Unhandled(throwable: Throwable)(implicit
         val loggingContext: ContextualizedErrorLogger
-    ) extends BaseError.Impl(
+    ) extends PackageServiceError(
           cause = "Failed with an unknown error cause",
           throwableO = Some(throwable),
         )
-        with PackageServiceError
   }
 
   object Validation {
     def handleLfArchiveError(
         lfArchiveError: LfArchiveError
-    )(implicit contextualizedErrorLogger: ContextualizedErrorLogger): BaseError.Impl =
+    )(implicit contextualizedErrorLogger: ContextualizedErrorLogger): PackageServiceError =
       lfArchiveError match {
         case LfArchiveError.InvalidDar(entries, cause) =>
           PackageServiceError.Reading.InvalidDar
@@ -168,7 +164,7 @@ object PackageServiceError extends PackageServiceErrorGroup {
 
     def handleLfEnginePackageError(err: Error.Package.Error)(implicit
         loggingContext: ContextualizedErrorLogger
-    ): BaseError.Impl = err match {
+    ): PackageServiceError = err match {
       case Error.Package.Internal(nameOfFunc, msg) =>
         PackageServiceError.InternalError.Validation(nameOfFunc, msg)
       case Error.Package.Validation(validationError) =>
@@ -195,10 +191,9 @@ object PackageServiceError extends PackageServiceErrorGroup {
         ) {
       final case class Error(validationError: validation.ValidationError)(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause = "Package validation failed."
           )
-          with PackageServiceError
     }
 
     final case class AllowedLanguageMismatchError(
@@ -207,11 +202,10 @@ object PackageServiceError extends PackageServiceErrorGroup {
         allowedLanguageVersions: VersionRange[language.LanguageVersion],
     )(implicit
         val loggingContext: ContextualizedErrorLogger
-    ) extends BaseError.Impl(
+    ) extends PackageServiceError(
           cause = LedgerApiErrors.Package.AllowedLanguageVersions
             .buildCause(packageId, languageVersion, allowedLanguageVersions)
         )(LedgerApiErrors.Package.AllowedLanguageVersions) // reuse error code of ledger api server
-        with PackageServiceError
 
     @Explanation(
       """This error indicates that the uploaded Dar is broken because it is missing internal dependencies."""
@@ -227,11 +221,10 @@ object PackageServiceError extends PackageServiceErrorGroup {
           missingDependencies: Set[Ref.PackageId],
       )(implicit
           val loggingContext: ContextualizedErrorLogger
-      ) extends BaseError.Impl(
+      ) extends PackageServiceError(
             cause =
               "The set of packages in the dar is not self-consistent and is missing dependencies"
           )
-          with PackageServiceError
     }
   }
 }
diff --git a/...ger-api-common/src/main/scala/com/digitalasset/platform/server/api/ValidationLogger.scala b/...ger-api-common/src/main/scala/com/digitalasset/platform/server/api/ValidationLogger.scala
@@ -3,6 +3,7 @@
 
 package com.daml.platform.server.api
 
+import com.daml.error.ErrorCode.LoggingApiException
 import com.daml.logging.{ContextualizedLogger, LoggingContext}
 
 object ValidationLogger {
@@ -11,16 +12,10 @@ object ValidationLogger {
       loggingContext: LoggingContext,
   ): Throwable = {
     logger.debug(s"Request validation failed for $request. Message: ${t.getMessage}")
-    logger.info(t.getMessage)
-    t
-  }
-
-  def logFailureWithContext[Request, T <: Throwable](request: Request, t: T)(implicit
-      logger: ContextualizedLogger,
-      loggingContext: LoggingContext,
-  ): T = {
-    logger.debug(s"Request validation failed for $request. Message: ${t.getMessage}")
-    logger.info(t.getMessage)
+    t match {
+      case _: LoggingApiException => ()
+      case _ => logger.info(t.getMessage)
+    }
     t
   }
 }
diff --git a/...ommon/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala b/...ommon/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala
@@ -12,12 +12,11 @@ import com.daml.ledger.grpc.GrpcStatuses
 import com.daml.lf.data.Ref.TransactionId
 import com.daml.lf.transaction.GlobalKey
 import com.daml.lf.value.Value
-import com.daml.logging.{ContextualizedLogger, LoggingContext}
 import com.daml.platform.server.api.validation.ErrorFactories.{
   addDefiniteAnswerDetails,
   definiteAnswers,
 }
-import com.daml.platform.server.api.{ValidationLogger, ApiException => NoStackTraceApiException}
+import com.daml.platform.server.api.{ApiException => NoStackTraceApiException}
 import com.google.protobuf.{Any => AnyProto}
 import com.google.rpc.status.{Status => RpcStatus}
 import com.google.rpc.{ErrorInfo, Status}
@@ -56,21 +55,16 @@ class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitch
         .asGrpcError,
     )
 
-  def malformedPackageId[Request](request: Request, message: String)(implicit
-      contextualizedErrorLogger: ContextualizedErrorLogger,
-      logger: ContextualizedLogger,
-      loggingContext: LoggingContext,
+  def malformedPackageId[Request](message: String)(implicit
+      contextualizedErrorLogger: ContextualizedErrorLogger
   ): StatusRuntimeException =
     errorCodesVersionSwitcher.choose(
-      v1 = ValidationLogger.logFailureWithContext(
-        request,
-        grpcError(
-          Status
-            .newBuilder()
-            .setCode(Code.INVALID_ARGUMENT.value())
-            .setMessage(message)
-            .build()
-        ),
+      v1 = grpcError(
+        Status
+          .newBuilder()
+          .setCode(Code.INVALID_ARGUMENT.value())
+          .setMessage(message)
+          .build()
       ),
       v2 = LedgerApiErrors.ReadErrors.MalformedPackageId
         .Reject(

diff --git a/...test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala b/...test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala
@@ -81,10 +81,8 @@ class ErrorFactoriesSpec
 
     "return malformedPackageId" in {
       assertVersionedError(
-        _.malformedPackageId(request = "request123", message = "message123")(
-          contextualizedErrorLogger = contextualizedErrorLogger,
-          logger = logger,
-          loggingContext = loggingContext,
+        _.malformedPackageId("message123")(
+          contextualizedErrorLogger = contextualizedErrorLogger
         )
       )(
         v1_code = Code.INVALID_ARGUMENT,

diff --git a/...ntegration-api/src/main/scala/platform/apiserver/services/ApiActiveContractsService.scala b/...ntegration-api/src/main/scala/platform/apiserver/services/ApiActiveContractsService.scala
@@ -53,7 +53,7 @@ private[apiserver] final class ApiActiveContractsService private (
     transactionFilterValidator
       .validate(request.getFilter)
       .fold(
-        t => Source.failed(ValidationLogger.logFailureWithContext(request, t)),
+        t => Source.failed(ValidationLogger.logFailure(request, t)),
         filters =>
           withEnrichedLoggingContext(logging.filters(filters)) { implicit loggingContext =>
             logger.info(s"Received request for active contracts: $request")

diff --git a/...cipant-integration-api/src/main/scala/platform/apiserver/services/ApiPackageService.scala b/...cipant-integration-api/src/main/scala/platform/apiserver/services/ApiPackageService.scala
@@ -13,6 +13,7 @@ import com.daml.lf.data.Ref
 import com.daml.logging.LoggingContext.withEnrichedLoggingContext
 import com.daml.logging.{ContextualizedLogger, LoggingContext}
 import com.daml.platform.api.grpc.GrpcApiService
+import com.daml.platform.server.api.ValidationLogger
 import com.daml.platform.server.api.validation.{
   ErrorFactories,
   FieldValidations,
@@ -93,10 +94,11 @@ private[apiserver] final class ApiPackageService private (
       .fold(
         errorMessage =>
           Future.failed[T](
-            errorFactories.malformedPackageId(request = request, message = errorMessage)(
-              createContextualizedErrorLogger,
-              logger,
-              loggingContext,
+            ValidationLogger.logFailure(
+              request,
+              errorFactories.malformedPackageId(errorMessage)(
+                createContextualizedErrorLogger
+              ),
             )
           ),
         packageId => block(packageId),

diff --git a/...ant-integration-api/src/main/scala/platform/apiserver/services/ApiSubmissionService.scala b/...ant-integration-api/src/main/scala/platform/apiserver/services/ApiSubmissionService.scala
@@ -4,6 +4,7 @@
 package com.daml.platform.apiserver.services
 
 import com.daml.api.util.TimeProvider
+import com.daml.error.ErrorCode.LoggingApiException
 import com.daml.error.{
   ContextualizedErrorLogger,
   DamlContextualizedErrorLogger,
@@ -199,6 +200,8 @@ private[apiserver] final class ApiSubmissionService private[services] (
         logger.info(s"Rejected: ${result.description}")
         Failure(result.exception)
 
+      // Do not log again on errors that are logging on creation
+      case Failure(error: LoggingApiException) => Failure(error)
       case Failure(error) =>
         logger.info(s"Rejected: ${error.getMessage}")
         Failure(error)

diff --git a/...rticipant-integration-api/src/main/scala/platform/apiserver/services/ApiTimeService.scala b/...rticipant-integration-api/src/main/scala/platform/apiserver/services/ApiTimeService.scala
@@ -62,7 +62,7 @@ private[apiserver] final class ApiTimeService private (
     val validated =
       matchLedgerId(ledgerId)(LedgerId(request.ledgerId))
     validated.fold(
-      t => Source.failed(ValidationLogger.logFailureWithContext(request, t)),
+      t => Source.failed(ValidationLogger.logFailure(request, t)),
       { ledgerId =>
         logger.info(s"Received request for time with ledger ID $ledgerId")
         dispatcher

diff --git a/...ion-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala b/...ion-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala
@@ -103,7 +103,7 @@ private[apiserver] final class ApiConfigManagementService private (
         val response = for {
           // Validate and convert the request parameters
           params <- validateParameters(request).fold(
-            t => Future.failed(ValidationLogger.logFailureWithContext(request, t)),
+            t => Future.failed(ValidationLogger.logFailure(request, t)),
             Future.successful,
           )
 
@@ -126,7 +126,7 @@ private[apiserver] final class ApiConfigManagementService private (
           _ <-
             if (request.configurationGeneration != expectedGeneration) {
               Future.failed(
-                ValidationLogger.logFailureWithContext(
+                ValidationLogger.logFailure(
                   request,
                   invalidArgument(None)(
                     s"Mismatching configuration generation, expected $expectedGeneration, received ${request.configurationGeneration}"