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.

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-686][Self-service error codes] Removing default error factories #11403

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,13 @@ object LedgerApiErrors extends LedgerApiErrorGroup {
ErrorCategory.SystemInternalAssumptionViolated,
) {

// TODO error codes: This is an internal error not related to the interpreter
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this comment? This error appears under LedgerApiErrors.InternalError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is the what is now called InternalError is really "internal errors related to the interpreter":

@explanation("""This error occurs if there was an unexpected error within the interpreter""")
@resolution("Contact support.")
object InternalError

Copy link
Contributor

@tudor-da tudor-da Nov 2, 2021

Choose a reason for hiding this comment

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

Ah right, the explanation was wrong here. Fine, let's leave as it

case class CommandTrackerInternalError(
message: String
)(implicit
loggingContext: ContextualizedErrorLogger
) extends LoggingTransactionErrorImpl(cause = message)

case class PackageSelfConsistency(
err: LfError.Package.SelfConsistency
)(implicit
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@

package com.daml.error.definitions

import com.daml.error.{BaseError, ErrorCause, ErrorCode, ContextualizedErrorLogger}
import com.daml.error.{BaseError, ContextualizedErrorLogger, ErrorCause, ErrorCode}
import com.daml.ledger.participant.state
import com.daml.lf.engine.Error.{Interpretation, Package, Preprocessing, Validation}
import com.daml.lf.engine.{Error => LfError}
import com.daml.lf.interpretation.{Error => LfInterpretationError}
import io.grpc.Status.Code
import io.grpc.StatusRuntimeException
import io.grpc.protobuf.StatusProto

import scala.util.{Failure, Success, Try}
import com.daml.lf.engine.{Error => LfError}
import com.daml.lf.interpretation.{Error => LfInterpretationError}

class RejectionGenerators(conformanceMode: Boolean) {
private val adjustErrors = Map(
Expand Down
1 change: 1 addition & 0 deletions ledger/indexer-benchmark/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ da_scala_library(
deps = [
"//daml-lf/data",
"//language-support/scala/bindings",
"//ledger/ledger-api-common",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be anymore. Weird. I thought Bazel would error out any time a dependency is not needed.
Could be some bug at play or some unknown to me behavior related to e.g. transitive dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing

"//ledger/ledger-api-health",
"//ledger/ledger-configuration",
"//ledger/ledger-offset",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,20 @@ class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitch
v2 = LedgerApiErrors.ServiceNotRunning.Reject().asGrpcError,
)

def trackerFailure(msg: String)(implicit
contextualizedErrorLogger: ContextualizedErrorLogger
): StatusRuntimeException =
errorCodesVersionSwitcher.choose(
v1 = {
val builder = Status
.newBuilder()
.setCode(Code.INTERNAL.value())
.setMessage(msg)
grpcError(builder.build())
},
v2 = LedgerApiErrors.InternalError.CommandTrackerInternalError(msg).asGrpcError,
)

/** Transforms Protobuf [[Status]] objects, possibly including metadata packed as [[ErrorInfo]] objects,
* into exceptions with metadata in the trailers.
*
Expand All @@ -427,11 +441,7 @@ class ErrorFactories private (errorCodesVersionSwitcher: ErrorCodesVersionSwitch
}
}

/** Object exposing the legacy error factories.
* TODO error codes: Remove default implementation once all Ledger API services
* output versioned error codes.
Comment on lines -443 to -444
Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, one removed! 🍾

*/
object ErrorFactories extends ErrorFactories(new ErrorCodesVersionSwitcher(false)) {
object ErrorFactories {
def apply(errorCodesVersionSwitcher: ErrorCodesVersionSwitcher): ErrorFactories =
new ErrorFactories(errorCodesVersionSwitcher)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,18 @@ import com.google.rpc._
import io.grpc.Status.Code
import io.grpc.StatusRuntimeException
import io.grpc.protobuf.StatusProto
import org.mockito.MockitoSugar
import org.scalatest.matchers.should.Matchers
import org.scalatest.prop.TableDrivenPropertyChecks
import org.scalatest.wordspec.AnyWordSpec

import scala.jdk.CollectionConverters._

class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPropertyChecks {
class ErrorFactoriesSpec
extends AnyWordSpec
with Matchers
with TableDrivenPropertyChecks
with MockitoSugar {
private val correlationId = "trace-id"
private val logger = ContextualizedLogger.get(getClass)
private val loggingContext = LoggingContext.ForTesting
Expand All @@ -35,6 +40,8 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
private val DefaultTraceIdRequestInfo: ErrorDetails.RequestInfoDetail =
ErrorDetails.RequestInfoDetail("trace-id")

val tested = ErrorFactories(mock[ErrorCodesVersionSwitcher])

"ErrorFactories" should {
"return malformedPackageId" in {
assertVersionedError(
Expand Down Expand Up @@ -246,7 +253,7 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
)

forEvery(testCases) { (definiteAnswer, expectedDetails) =>
val exception = aborted("my message", definiteAnswer)
val exception = tested.aborted("my message", definiteAnswer)
val status = StatusProto.fromThrowable(exception)
status.getCode shouldBe Code.ABORTED.value()
status.getMessage shouldBe "my message"
Expand Down Expand Up @@ -303,7 +310,7 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
}

"fail on creating a ledgerIdMismatch error due to a wrong definite answer" in {
an[IllegalArgumentException] should be thrownBy ledgerIdMismatch(
an[IllegalArgumentException] should be thrownBy tested.ledgerIdMismatch(
LedgerId("expected"),
LedgerId("received"),
definiteAnswer = Some(true),
Expand All @@ -324,6 +331,21 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope
)
}

"return a trackerFailure error" in {
assertVersionedError(_.trackerFailure("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 an offsetAfterLedgerEnd error" in {
assertVersionedError(_.offsetAfterLedgerEnd("my message"))(
v1_code = Code.OUT_OF_RANGE,
Expand Down Expand Up @@ -447,7 +469,7 @@ class ErrorFactoriesSpec extends AnyWordSpec with Matchers with TableDrivenPrope

"should create an ApiException without the stack trace" in {
val status = Status.newBuilder().setCode(Code.INTERNAL.value()).build()
val exception = grpcError(status)
val exception = tested.grpcError(status)
exception.getStackTrace shouldBe Array.empty
}
}
Expand Down
4 changes: 3 additions & 1 deletion ledger/participant-integration-api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ compile_deps = [
"//libs-scala/scala-utils",
"//libs-scala/timer-utils",
"//libs-scala/nameof",
"@maven//:com_google_api_grpc_proto_google_common_protos",
"@maven//:com_google_guava_guava",
"@maven//:com_zaxxer_HikariCP",
"@maven//:io_dropwizard_metrics_metrics_core",
Expand Down Expand Up @@ -162,6 +161,7 @@ da_scala_library(
"@maven//:org_scalatest_scalatest_flatspec",
"@maven//:org_scalatest_scalatest_matchers_core",
"@maven//:org_scalatest_scalatest_shouldmatchers",
"@maven//:org_mockito_mockito_scala",
],
scala_runtime_deps = [
"@maven//:com_typesafe_akka_akka_slf4j",
Expand Down Expand Up @@ -211,6 +211,7 @@ da_scala_library(
"@maven//:io_netty_netty_common",
"@maven//:io_netty_netty_handler",
"@maven//:io_netty_netty_transport",
"@maven//:org_mockito_mockito_core",
"@maven//:org_scalatest_scalatest_compatible",
],
)
Expand Down Expand Up @@ -353,6 +354,7 @@ da_scala_test_suite(
"@maven//:org_scalatest_scalatest_matchers_core",
"@maven//:org_scalatest_scalatest_shouldmatchers",
"@maven//:org_scalatest_scalatest_wordspec",
"@maven//:org_mockito_mockito_scala",
],
tags = [] if oracle_testing else ["manual"],
runtime_deps = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ final class StandaloneApiServer(
enableMutableContractStateCache = config.enableMutableContractStateCache,
maxTransactionsInMemoryFanOutBufferSize = config.maxTransactionsInMemoryFanOutBufferSize,
enableInMemoryFanOutForLedgerApi = config.enableInMemoryFanOutForLedgerApi,
enableSelfServiceErrorCodes = config.enableSelfServiceErrorCodes,
)
.map(index => new SpannedIndexService(new TimedIndexService(index, metrics)))
errorCodesVersionSwitcher = new ErrorCodesVersionSwitcher(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,13 @@ private[apiserver] object ApiCommandService {
val submissionTracker = new TrackerMap.SelfCleaning(
configuration.trackerRetentionPeriod,
Tracking.getTrackerKey,
Tracking.newTracker(configuration, submissionFlow, completionServices, metrics),
Tracking.newTracker(
configuration,
submissionFlow,
completionServices,
metrics,
ErrorFactories(errorCodesVersionSwitcher),
),
trackerCleanupInterval,
)
new GrpcCommandService(
Expand Down Expand Up @@ -238,6 +244,7 @@ private[apiserver] object ApiCommandService {
submissionFlow: SubmissionFlow,
completionServices: CompletionServices,
metrics: Metrics,
errorFactories: ErrorFactories,
)(
key: Tracking.Key
)(implicit
Expand Down Expand Up @@ -285,6 +292,7 @@ private[apiserver] object ApiCommandService {
capacityCounter = metrics.daml.commands.inputBufferCapacity,
lengthCounter = metrics.daml.commands.inputBufferLength,
delayTimer = metrics.daml.commands.inputBufferDelay,
errorFactories = errorFactories,
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ private[apiserver] final class ApiConfigManagementService private (
errorFactories,
),
timeToLive = JDuration.ofMillis(params.timeToLive.toMillis),
errorFactories = errorFactories,
)
entry <- synchronousResponse.submitAndWait(
augmentedSubmissionId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ private[apiserver] final class ApiPackageManagementService private (
new DamlContextualizedErrorLogger(logger, loggingContext, None)

private val errorFactories = ErrorFactories(errorCodesVersionSwitcher)

private val synchronousResponse = new SynchronousResponse(
new SynchronousResponseStrategy(
transactionsService,
Expand All @@ -73,6 +72,7 @@ private[apiserver] final class ApiPackageManagementService private (
errorFactories,
),
timeToLive = managementServiceTimeout,
errorFactories = errorFactories,
)

override def close(): Unit = ()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ private[apiserver] final class ApiPartyManagementService private (
errorFactories,
),
timeToLive = managementServiceTimeout,
errorFactories = errorFactories,
)

override def close(): Unit = ()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import scala.concurrent.{ExecutionContext, Future, TimeoutException}
class SynchronousResponse[Input, Entry, AcceptedEntry](
strategy: SynchronousResponse.Strategy[Input, Entry, AcceptedEntry],
timeToLive: Duration,
errorFactories: ErrorFactories,
) {

def submitAndWait(submissionId: Ref.SubmissionId, input: Input)(implicit
Expand All @@ -52,7 +53,7 @@ class SynchronousResponse[Input, Entry, AcceptedEntry](
.runWith(Sink.head)
.recoverWith { case _: TimeoutException =>
Future.failed(
ErrorFactories.aborted("Request timed out", definiteAnswer = Some(false))
errorFactories.aborted("Request timed out", definiteAnswer = Some(false))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi @tudor-da

Copy link
Contributor

@tudor-da tudor-da Nov 2, 2021

Choose a reason for hiding this comment

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

The aborted method does not return self-service error codes, since I thought it was too generic when I read it. I think the correct error category for this one is DeadlineExceededRequestStateUnknown. Please adapt, e.g.

  • Create a new method errorFactories.isTimeoutUnknown_wasAborted and return the versioned error based on the two cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

)
}
.flatten
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import akka.stream.{Materializer, OverflowStrategy, QueueOfferResult}
import akka.{Done, NotUsed}
import com.codahale.metrics.{Counter, Timer}
import com.daml.dec.DirectExecutionContext
import com.daml.error.DamlContextualizedErrorLogger
import com.daml.ledger.client.services.commands.CommandSubmission
import com.daml.ledger.client.services.commands.CommandTrackerFlow.Materialized
import com.daml.ledger.client.services.commands.tracker.CompletionResponse._
Expand All @@ -16,15 +17,14 @@ import com.daml.metrics.InstrumentedSource
import com.daml.platform.apiserver.services.tracking.QueueBackedTracker._
import com.daml.platform.server.api.validation.ErrorFactories
import com.daml.util.Ctx
import com.google.rpc.Status
import io.grpc.Status.Code
import io.grpc.{Status => GrpcStatus}

import scala.concurrent.duration._
import scala.concurrent.{Await, ExecutionContext, Future, Promise}
import scala.util.{Failure, Success}
import scala.util.{Failure, Success, Try}

/** Tracks SubmitAndWaitRequests.
*
* @param queue The input queue to the tracking flow.
*/
private[services] final class QueueBackedTracker(
Expand Down Expand Up @@ -110,6 +110,7 @@ private[services] object QueueBackedTracker {
capacityCounter: Counter,
lengthCounter: Counter,
delayTimer: Timer,
errorFactories: ErrorFactories,
)(implicit materializer: Materializer, loggingContext: LoggingContext): QueueBackedTracker = {
val ((queue, mat), done) = InstrumentedSource
.queue[QueueInput](
Expand Down Expand Up @@ -147,19 +148,17 @@ private[services] object QueueBackedTracker {
mat.trackingMat.onComplete(
// no error expected here -- if there is one, we're at a total loss.
// FIXME(mthvedt): we should shut down everything in this case.
_.get.values
.foreach(
_.failure(
ErrorFactories.grpcError(
Status
.newBuilder()
.setCode(Code.INTERNAL.value())
.setMessage(promiseCancellationDescription)
.build()
)
(aTry: Try[Map[_, Promise[_]]]) => {
val promises: Iterable[Promise[_]] = aTry.get.values
val errorLogger = new DamlContextualizedErrorLogger(logger, loggingContext, None)
promises.foreach(p =>
p.failure(
errorFactories.trackerFailure(msg = promiseCancellationDescription)(errorLogger)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi @tudor-da

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to keep those comments in the right place if changing this piece.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I'm not sure what that comment precisely refers to.
I guess it's that _.get call.

Moving the comments right before it.

)
)
}
)(DirectExecutionContext)

}(DirectExecutionContext)

new QueueBackedTracker(queue, done)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package com.daml.platform.index

import akka.stream.Materializer
import com.daml.error.ErrorCodesVersionSwitcher
import com.daml.ledger.api.domain.LedgerId
import com.daml.ledger.participant.state.index.v2.IndexService
import com.daml.ledger.resources.ResourceOwner
Expand All @@ -12,6 +13,7 @@ import com.daml.lf.engine.ValueEnricher
import com.daml.logging.LoggingContext
import com.daml.metrics.Metrics
import com.daml.platform.configuration.ServerRole
import com.daml.platform.server.api.validation.ErrorFactories
import com.daml.platform.store.LfValueTranslationCache

import scala.concurrent.ExecutionContext
Expand All @@ -36,6 +38,7 @@ private[platform] object JdbcIndex {
enableMutableContractStateCache: Boolean,
maxTransactionsInMemoryFanOutBufferSize: Long,
enableInMemoryFanOutForLedgerApi: Boolean,
enableSelfServiceErrorCodes: Boolean,
)(implicit mat: Materializer, loggingContext: LoggingContext): ResourceOwner[IndexService] =
new ReadOnlySqlLedger.Owner(
serverRole = serverRole,
Expand All @@ -55,7 +58,12 @@ private[platform] object JdbcIndex {
enableInMemoryFanOutForLedgerApi = enableInMemoryFanOutForLedgerApi,
participantId = participantId,
maxTransactionsInMemoryFanOutBufferSize = maxTransactionsInMemoryFanOutBufferSize,
errorFactories = ErrorFactories(new ErrorCodesVersionSwitcher(enableSelfServiceErrorCodes)),
).map { ledger =>
new LedgerBackedIndexService(MeteredReadOnlyLedger(ledger, metrics), participantId)
new LedgerBackedIndexService(
MeteredReadOnlyLedger(ledger, metrics),
participantId,
errorFactories = ErrorFactories(new ErrorCodesVersionSwitcher(enableSelfServiceErrorCodes)),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import scala.concurrent.Future
private[platform] final class LedgerBackedIndexService(
ledger: ReadOnlyLedger,
participantId: Ref.ParticipantId,
errorFactories: ErrorFactories,
) extends IndexService {
private val logger = ContextualizedLogger.get(getClass)

Expand Down Expand Up @@ -144,7 +145,7 @@ private[platform] final class LedgerBackedIndexService(
case Some(end) if begin > end =>
Source.failed(
// TODO error codes: Replace with LedgerApiErrors.ReadErrors.RequestedOffsetAfterLedgerEnd
Copy link
Contributor

@tudor-da tudor-da Oct 29, 2021

Choose a reason for hiding this comment

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

Let's address this TODO:

  • Use errorFactories.offsetAfterLedgerEnd
  • Rename ErrorFactories#offsetAfterLedgerEnd to ErrorFactories#offsetOutOfRange
  • Rename LedgerApiErrors.ReadErrors.RequestedOffsetAfterLedgerEnd to LedgerApiErrors.ReadErrors.RequestedOffsetOutOfRange since each caller specializes it with the reason anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you proposed means changing V1 (from INVALID_ARGUMENT to OUT_OF_RANGE). Is it fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scratch first comment. Actually using errorFactories.readingOffsetOutOfRange_was_invalidArgument since it's already there.

ErrorFactories.invalidArgument(None)(
errorFactories.invalidArgument(None)(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fyi @tudor-da

s"End offset ${end.toApiString} is before Begin offset ${begin.toApiString}."
)(new DamlContextualizedErrorLogger(logger, loggingContext, None))
)
Expand Down
Loading