From 8e08450220e19802209ee48690eaf1e247272930 Mon Sep 17 00:00:00 2001 From: tudor-da Date: Mon, 15 Nov 2021 09:13:50 +0100 Subject: [PATCH] [Self-service error codes] No redundant logging in Ledger API (#11649) CHANGELOG_BEGIN CHANGELOG_END --- .../main/scala/com/daml/error/ErrorCode.scala | 14 ++++- .../definitions/PackageServiceError.scala | 53 ++++++++----------- .../server/api/ValidationLogger.scala | 15 ++---- .../api/validation/ErrorFactories.scala | 24 ++++----- .../api/validation/ErrorFactoriesSpec.scala | 6 +-- .../services/ApiActiveContractsService.scala | 2 +- .../services/ApiPackageService.scala | 10 ++-- .../services/ApiSubmissionService.scala | 3 ++ .../apiserver/services/ApiTimeService.scala | 2 +- .../admin/ApiConfigManagementService.scala | 4 +- .../admin/ApiPackageManagementService.scala | 14 +++-- .../admin/ApiParticipantPruningService.scala | 2 +- .../admin/ApiPartyManagementService.scala | 2 +- 13 files changed, 73 insertions(+), 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 index df5251bd46a0..4f26eac5477d 100644 --- 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 index 5fb5343ab1c8..42ba4ce7756e 100644 --- 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,11 +53,10 @@ 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.") @@ -60,10 +64,9 @@ object PackageServiceError extends PackageServiceErrorGroup { 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/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/ValidationLogger.scala b/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/ValidationLogger.scala index ef8d9facdebb..23b8b20400af 100644 --- a/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/ValidationLogger.scala +++ b/ledger/ledger-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/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala b/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala index 7b492201ea7a..2a9a131bd1b8 100644 --- a/ledger/ledger-api-common/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala +++ b/ledger/ledger-api-common/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/ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala b/ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala index 64b159e475b3..f81ec47619ef 100644 --- a/ledger/ledger-api-common/src/test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala +++ b/ledger/ledger-api-common/src/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/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiActiveContractsService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiActiveContractsService.scala index 95922584d085..20d819323aba 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiActiveContractsService.scala +++ b/ledger/participant-integration-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/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiPackageService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiPackageService.scala index 5f1eb95d20ec..3dbff4a4886b 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiPackageService.scala +++ b/ledger/participant-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/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiSubmissionService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiSubmissionService.scala index 76e3e815d08d..929b1b141247 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiSubmissionService.scala +++ b/ledger/participant-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/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiTimeService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiTimeService.scala index e6d9016cee8e..dd3fef0f65bd 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/ApiTimeService.scala +++ b/ledger/participant-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/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala index 06619fd9ba0d..fa69da6a5e8a 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiConfigManagementService.scala +++ b/ledger/participant-integration-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}" diff --git a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPackageManagementService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPackageManagementService.scala index 32db98b21965..995c6023762a 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPackageManagementService.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPackageManagementService.scala @@ -9,9 +9,9 @@ import akka.stream.Materializer import akka.stream.scaladsl.Source import com.daml.api.util.TimestampConversion import com.daml.daml_lf_dev.DamlLf.Archive +import com.daml.error.definitions.PackageServiceError import com.daml.error.definitions.PackageServiceError.Validation import com.daml.error.{ - BaseError, ContextualizedErrorLogger, DamlContextualizedErrorLogger, ErrorCodesVersionSwitcher, @@ -106,13 +106,13 @@ private[apiserver] final class ApiPackageManagementService private ( for { dar <- darReader .readArchive("package-upload", stream) - .loggingValidation(Validation.handleLfArchiveError) + .handleError(Validation.handleLfArchiveError) packages <- dar.all .traverse(Decode.decodeArchive(_)) - .loggingValidation(Validation.handleLfArchiveError) + .handleError(Validation.handleLfArchiveError) _ <- engine .validatePackages(packages.toMap) - .loggingValidation(Validation.handleLfEnginePackageError) + .handleError(Validation.handleLfEnginePackageError) } yield dar override def uploadDarFile(request: UploadDarFileRequest): Future[UploadDarFileResponse] = @@ -145,11 +145,9 @@ private[apiserver] final class ApiPackageManagementService private ( } private implicit class ErrorValidations[E, R](result: Either[E, R]) { - def loggingValidation(validate: E => BaseError.Impl): Try[R] = + def handleError(toSelfServiceErrorCode: E => PackageServiceError): Try[R] = result.left.map { err => - val baseError = validate(err) - baseError.log() - baseError.asGrpcError + toSelfServiceErrorCode(err).asGrpcError }.toTry } } diff --git a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiParticipantPruningService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiParticipantPruningService.scala index 4a7c397f38ca..83980f71a2c0 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiParticipantPruningService.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiParticipantPruningService.scala @@ -99,7 +99,7 @@ final class ApiParticipantPruningService private ( pruneUpTo <- checkOffsetIsHexadecimal(pruneUpToString) } yield (pruneUpTo, pruneUpToString)) .fold( - t => Future.failed(ValidationLogger.logFailureWithContext(request, t)), + t => Future.failed(ValidationLogger.logFailure(request, t)), o => checkOffsetIsBeforeLedgerEnd(o._1, o._2), ) } diff --git a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala index df5d697640a5..a7fff73e87a8 100644 --- a/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala +++ b/ledger/participant-integration-api/src/main/scala/platform/apiserver/services/admin/ApiPartyManagementService.scala @@ -115,7 +115,7 @@ private[apiserver] final class ApiPartyManagementService private ( error => Future.failed( ValidationLogger - .logFailureWithContext(request, errorFactories.invalidArgument(None)(error)) + .logFailure(request, errorFactories.invalidArgument(None)(error)) ), party => Future.successful(Some(party)), )