Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DPP-621][Self-service error codes] Adopt error codes in ApiVersionService #11302

Merged
merged 5 commits into from
Oct 21, 2021
Merged

[DPP-621][Self-service error codes] Adopt error codes in ApiVersionService #11302

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7306cb2
[DPP-621][Self-service error codes] Adopt error codes in ApiVersionSe…
pbatko-da Oct 20, 2021
0cfa827
unused import
pbatko-da Oct 20, 2021
2c61147
Merge branch 'main' into pbatko/error-codes/dpp-621-version-service
pbatko-da Oct 20, 2021
cb966eb
Merge branch 'main' into pbatko/error-codes/dpp-621-version-service
pbatko-da Oct 21, 2021
178b5ec
MZ review
pbatko-da Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ledger/error/src/main/scala/com/daml/error/definitions/LedgerApiErrors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,12 @@ object LedgerApiErrors extends LedgerApiErrorGroup {
ErrorCategory.SystemInternalAssumptionViolated,
) {

case class Reject(message: String)(implicit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tudor-da FYI: Created a Reject to model a generic case for which we don't have or don't care about having a more specific error.

Question: The explanation for enclosing InternalError states that:

@Explanation("""This error occurs if there was an unexpected error within the interpreter""")

Is the "interpreter" mention accurate for the use case in ApiVersionService that this PR is adopting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, interpreter has a very narrow meaning of a Daml interpreter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up until now almost all InternalErrors we have covered were stemming from the Interpreter, so this explanation was correct.
Version service and likely other services will encounter errors that don't have anything to do with the interpreter.
In fact something like this has happened for the package service, where a separate category PackageServiceError.InternalError has been introduced. I'd suggest we do the same for Version Service.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining this!

So I'm creating a new error group dedicated to the version service: VersionServiceError (LedgerApiErrors.VersionServiceError.InternalError.Reject)

fyi @tudor-da

Copy link
Contributor

@tudor-da tudor-da Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a generic error grouping for this internal error, since a specialized grouping for this doesn't bring anything to the client via GRPC (errors with status INTERNAL don't propagate any detailed information to clients because they are considered security sensitive) or in the docs (it doesn't provide any entropy - the client already knows this error originated in ApiVersionService). On the other hand, participant operators can inspect the logged error and derive what's wrong - the specialized grouping doesn't help much here as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd propose creating a generic LedgerApi.InternalError.Error and pass the reason to it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I am fine to merge this as is. I'll add the note about it in the cleanup task.

loggingContext: ContextualizedErrorLogger
) extends LoggingTransactionErrorImpl(
cause = message
)

case class PackageSelfConsistency(
err: LfError.Package.SelfConsistency
)(implicit
Expand Down
10 changes: 10 additions & 0 deletions ...ommon/src/main/scala/com/digitalasset/platform/server/api/validation/ErrorFactories.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@ import scalaz.syntax.tag._

class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitcher) {

def internalError(message: String)(implicit
contextualizedErrorLogger: ContextualizedErrorLogger
): StatusRuntimeException =
errorCodesVersionSwitcher.choose(
v1 = io.grpc.Status.INTERNAL
.withDescription(message)
.asRuntimeException(),
v2 = LedgerApiErrors.InternalError.Reject(message).asGrpcError,
)

def duplicateCommandException(implicit
contextualizedErrorLogger: ContextualizedErrorLogger
): StatusRuntimeException =
Expand Down
16 changes: 16 additions & 0 deletions ...test/suite/scala/com/digitalasset/platform/server/api/validation/ErrorFactoriesSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,22 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
ErrorDetails.RequestInfoDetail("trace-id")

"ErrorFactories" should {

"return the internalError" in {
assertVersionedError(_.internalError("message123"))(
v1_code = Code.INTERNAL,
v1_message = "message123",
v1_details = Seq.empty,
v2_code = Code.INTERNAL,
v2_message =
s"An error occurred. Please contact the operator and inquire about the request trace-id",
v2_details = Seq[ErrorDetails.ErrorDetail](
ErrorDetails.ErrorInfoDetail("LEDGER_API_INTERNAL_ERROR"),
DefaultTraceIdRequestInfo,
),
)
}

"return the DuplicateCommandException" in {
assertVersionedError(_.duplicateCommandException)(
v1_code = Code.ALREADY_EXISTS,
Expand Down
2 changes: 1 addition & 1 deletion ledger/participant-integration-api/src/main/scala/platform/apiserver/ApiServices.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ private[daml] object ApiServices {
ApiLedgerIdentityService.create(() => identityService.getLedgerId())

val apiVersionService =
ApiVersionService.create()
ApiVersionService.create(errorsVersionsSwitcher)

val apiPackageService = ApiPackageService.create(ledgerId, packagesService)

Expand Down
24 changes: 18 additions & 6 deletions ...cipant-integration-api/src/main/scala/platform/apiserver/services/ApiVersionService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,19 @@

package com.daml.platform.apiserver.services

import com.daml.error.{
ContextualizedErrorLogger,
DamlContextualizedErrorLogger,
ErrorCodesVersionSwitcher,
}
import com.daml.ledger.api.v1.version_service.GetLedgerApiVersionRequest
import com.daml.ledger.api.v1.version_service.GetLedgerApiVersionResponse
import com.daml.ledger.api.v1.version_service.VersionServiceGrpc
import com.daml.ledger.api.v1.version_service.VersionServiceGrpc.VersionService
import com.daml.logging.ContextualizedLogger
import com.daml.logging.LoggingContext
import com.daml.platform.api.grpc.GrpcApiService
import com.daml.platform.server.api.validation.ErrorFactories
import io.grpc.ServerServiceDefinition
import io.grpc.Status

Expand All @@ -19,14 +25,20 @@ import scala.io.Source
import scala.util.Try
import scala.util.control.NonFatal

private[apiserver] final class ApiVersionService private (implicit
private[apiserver] final class ApiVersionService private (
errorCodesVersionSwitcher: ErrorCodesVersionSwitcher
)(implicit
loggingContext: LoggingContext,
ec: ExecutionContext,
) extends VersionService
with GrpcApiService {

private val logger = ContextualizedLogger.get(this.getClass)

private val errorFactories = ErrorFactories(errorCodesVersionSwitcher)
private implicit val contextualizedErrorLogger: ContextualizedErrorLogger =
new DamlContextualizedErrorLogger(logger, loggingContext, None)

private val versionFile: String = "ledger-api/VERSION"
private lazy val apiVersion: Try[String] = readVersion(versionFile)

Expand All @@ -43,9 +55,7 @@ private[apiserver] final class ApiVersionService private (implicit

private lazy val internalError: Future[Nothing] =
Future.failed(
Status.INTERNAL
.withDescription("Cannot read Ledger API version")
.asRuntimeException()
errorFactories.internalError(message = "Cannot read Ledger API version")
)

private def readVersion(versionFileName: String): Try[String] =
Expand All @@ -65,6 +75,8 @@ private[apiserver] final class ApiVersionService private (implicit
}

private[apiserver] object ApiVersionService {
def create()(implicit loggingContext: LoggingContext, ec: ExecutionContext): ApiVersionService =
new ApiVersionService
def create(
errorCodesVersionSwitcher: ErrorCodesVersionSwitcher
)(implicit loggingContext: LoggingContext, ec: ExecutionContext): ApiVersionService =
new ApiVersionService(errorCodesVersionSwitcher)
}