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

[WIP] Implement ServiceCall exception logging using HttpErrorHandler interface #2251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,13 @@ trait LagomServiceApiBridge {
def rawExceptionMessageMessageAsText(rem: RawExceptionMessage): String
def rawExceptionMessageToResponseHeader(rem: RawExceptionMessage): ResponseHeader
def newRawExceptionMessage(errorCode: ErrorCode, protocol: MessageProtocol, message: ByteString): RawExceptionMessage
def errorCodeFromHttpStatus(status: Int): ErrorCode

type ErrorCode

type ServiceCall[Request, Response]
type Call[Request, Response]
def callForRequest(descriptor: Descriptor, requestHeader: RequestHeader): Call[_, _]
def methodForCall(call: Call[_, _]): Method
def callRequestSerializer[Request, W](call: Call[Request, _]): MessageSerializer[Request, W]
def callResponseSerializer[Response, W](call: Call[_, Response]): MessageSerializer[Response, W]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import play.api.Logger
import play.api.http.HeaderNames
import play.api.http.HttpConfiguration
import play.api.http.HttpEntity.Strict
import play.api.http.HttpErrorHandler
import play.api.http.websocket.BinaryMessage
import play.api.http.websocket.CloseMessage
import play.api.http.websocket.Message
Expand All @@ -39,16 +40,18 @@ import play.api.mvc.PlayBodyParsers
import play.api.mvc.Result
import play.api.mvc.Results
import play.api.mvc.WebSocket
import play.api.mvc.{ RequestHeader => PlayRequestHeader }
import play.api.routing.Router.Routes
import play.api.mvc.{RequestHeader => PlayRequestHeader}
import play.api.routing.HandlerDef
import play.api.routing.Router
import play.api.routing.Router.Routes
import play.api.routing.SimpleRouter

import scala.collection.immutable
import scala.concurrent.Await
import scala.concurrent.ExecutionContext
import scala.concurrent.Future
import scala.concurrent.Promise
import scala.concurrent.duration.Duration
import scala.util.Try
import scala.util.control.NonFatal

Expand All @@ -68,6 +71,7 @@ private[lagom] abstract class ServiceRouter(httpConfiguration: HttpConfiguration

protected val descriptor: Descriptor
protected val serviceRoutes: Seq[ServiceRoute]
protected val errorHandler = ServiceCallErrorHandler // todo: inject

import ServiceRouter._

Expand Down Expand Up @@ -183,13 +187,15 @@ private[lagom] abstract class ServiceRouter(httpConfiguration: HttpConfiguration
handleServiceCall(serviceCall, descriptor, requestSerializer, responseSerializer, filteredHeaders, request)
.recover {
case NonFatal(e) =>
logException(e, descriptor, call)
exceptionToResult(descriptor, filteredHeaders, e)
// logException(e, descriptor, call)
val result = processError(request, e)
Await.result(result, Duration.Inf)
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be an Await here, as this will cause the thread to block. I believe the right way to do this would be to to replace recover above with recoverWith and return result.

}
} catch {
case NonFatal(e) =>
logException(e, descriptor, call)
Accumulator.done(exceptionToResult(descriptor, filteredHeaders, e))
// logException(e, descriptor, call)
val result = processError(request, e)
Accumulator.done(Await.result(result, Duration.Inf))
}
}

Expand Down Expand Up @@ -261,7 +267,7 @@ private[lagom] abstract class ServiceRouter(httpConfiguration: HttpConfiguration
def log = Logger(descriptorName(descriptor))

val cause = exc match {
case c: CompletionException => c.getCause
case c: CompletionException => Option(c.getCause).getOrElse(c)
case e => e
}
maybeLogException(cause, log, call)
Expand Down Expand Up @@ -554,4 +560,24 @@ private[lagom] abstract class ServiceRouter(httpConfiguration: HttpConfiguration
request: Request
): Future[(ResponseHeader, Response)]

protected def processError(requestHeader: PlayRequestHeader, throwable: Throwable): Future[Result]

protected object ServiceCallErrorHandler extends HttpErrorHandler {
override def onClientError(request: PlayRequestHeader, statusCode: Int, message: String): Future[Result] = {
val unfilteredHeader = toLagomRequestHeader(request)
val filteredHeaders = headerFilterTransformServerRequest(descriptorHeaderFilter(descriptor), unfilteredHeader)
val exception = newTransportException(errorCodeFromHttpStatus(statusCode), message)
val result = exceptionToResult(descriptor, filteredHeaders, exception)
logException(exception, descriptor, callForRequest(descriptor, toLagomRequestHeader(request)))
Future.successful(result)
}

override def onServerError(request: PlayRequestHeader, exception: Throwable): Future[Result] = {
val unfilteredHeader = toLagomRequestHeader(request)
val filteredHeaders = headerFilterTransformServerRequest(descriptorHeaderFilter(descriptor), unfilteredHeader)
val result = exceptionToResult(descriptor, filteredHeaders, exception)
logException(exception, descriptor, callForRequest(descriptor, toLagomRequestHeader(request)))
Future.successful(result)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import akka.stream.scaladsl.Source
import akka.util.ByteString
import com.lightbend.lagom.internal.api.HeaderUtils
import com.lightbend.lagom.internal.api.transport.LagomServiceApiBridge
import com.lightbend.lagom.internal.javadsl.api.JavadslPath
import com.lightbend.lagom.javadsl.api
import com.lightbend.lagom.javadsl.api.Descriptor.RestCallId
import com.lightbend.lagom.javadsl.api.deser.ExceptionMessage
Expand Down Expand Up @@ -168,11 +169,20 @@ trait JavadslServiceApiBridge extends LagomServiceApiBridge {
message: ByteString
): RawExceptionMessage =
new deser.RawExceptionMessage(errorCode, protocol, message)

override def errorCodeFromHttpStatus(status: Int): ErrorCode = transport.TransportErrorCode.fromHttp(status)
override type ErrorCode = transport.TransportErrorCode

override type ServiceCall[Request, Response] = api.ServiceCall[Request, Response]
override type Call[Request, Response] = api.Descriptor.Call[Request, Response]
override def callForRequest(descriptor: Descriptor, requestHeader: RequestHeader): Call[_, _] = {
descriptor
.calls()
.asScala
.groupBy(methodForCall)
.getOrElse(requestHeader.method, Vector.empty)
.find(call => JavadslPath.fromCallId(call.callId()).regex.findFirstIn(requestHeader.uri.getPath).nonEmpty)
.get
}
override def methodForCall(call: Call[_, _]): Method =
call.callId match {
case rest: RestCallId => rest.method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,21 @@
package com.lightbend.lagom.internal.javadsl.server

import java.util.function.BiFunction
import java.util.function.{ Function => JFunction }
import java.util.{ List => JList }
import java.util.function.{Function => JFunction}
import java.util.{List => JList}

import akka.stream.Materializer
import akka.util.ByteString
import com.lightbend.lagom.internal.api._
import com.lightbend.lagom.internal.javadsl.api._
import com.lightbend.lagom.internal.javadsl.client.JavadslServiceApiBridge
import com.lightbend.lagom.internal.server.ServiceRouter
import com.lightbend.lagom.javadsl.api.Descriptor.RestCallId
import com.lightbend.lagom.javadsl.api.Descriptor
import com.lightbend.lagom.javadsl.api.Descriptor.RestCallId
import com.lightbend.lagom.javadsl.api.Service
import com.lightbend.lagom.javadsl.api.ServiceInfo
import com.lightbend.lagom.javadsl.api.deser.StreamedMessageSerializer
import com.lightbend.lagom.javadsl.api.transport.{ RequestHeader => _, _ }
import com.lightbend.lagom.javadsl.api.transport._
import com.lightbend.lagom.javadsl.jackson.JacksonExceptionSerializer
import com.lightbend.lagom.javadsl.jackson.JacksonSerializerFactory
import com.lightbend.lagom.javadsl.server.LagomServiceRouter
Expand All @@ -34,9 +34,11 @@ import play.api.Environment
import play.api.Logger
import play.api.http.HttpConfiguration
import play.api.inject.Injector
import play.api.mvc.{ RequestHeader => PlayRequestHeader, ResponseHeader => _, _ }
import play.api.routing.Router.Routes
import play.api.mvc
import play.api.mvc.{RequestHeader => PlayRequestHeader}
import play.api.mvc._
import play.api.routing.Router
import play.api.routing.Router.Routes
import play.api.routing.SimpleRouter

import scala.collection.JavaConverters._
Expand Down Expand Up @@ -101,8 +103,8 @@ class JavadslServerBuilder @Inject()(
)
}

import org.pcollections._
import com.lightbend.lagom.javadsl.api._
import org.pcollections._
val acls: PSequence[ServiceAcl] = descriptors
.filter(_.locatableService())
.map(_.acls())
Expand Down Expand Up @@ -265,4 +267,10 @@ class JavadslServiceRouter(
}
}

override protected def processError(requestHeader: mvc.RequestHeader, throwable: Throwable): Future[Result] = {
throwable match {
case p: PayloadTooLarge => errorHandler.onClientError(requestHeader, p.errorCode().http(), throwable.getMessage)
case _ => errorHandler.onServerError(requestHeader, throwable)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,12 @@ import java.security.Principal
import akka.stream.scaladsl.Source
import akka.util.ByteString
import com.lightbend.lagom.internal.api.transport.LagomServiceApiBridge
import com.lightbend.lagom.scaladsl.api.deser
import com.lightbend.lagom.scaladsl.api.transport
import com.lightbend.lagom.internal.scaladsl.api.ScaladslPath
import com.lightbend.lagom.scaladsl.api
import com.lightbend.lagom.scaladsl.api.Descriptor.RestCallId
import com.lightbend.lagom.scaladsl.api.deser
import com.lightbend.lagom.scaladsl.api.security.ServicePrincipal
import com.lightbend.lagom.scaladsl.api.transport
import com.lightbend.lagom.scaladsl.api.transport.ExceptionMessage

import scala.collection.immutable
Expand Down Expand Up @@ -134,11 +135,18 @@ trait ScaladslServiceApiBridge extends LagomServiceApiBridge {
message: ByteString
): RawExceptionMessage =
deser.RawExceptionMessage(errorCode, protocol, message)

override def errorCodeFromHttpStatus(status: Int): ErrorCode = transport.TransportErrorCode.fromHttp(status)
override type ErrorCode = transport.TransportErrorCode

override type ServiceCall[Request, Response] = api.ServiceCall[Request, Response]
override type Call[Request, Response] = api.Descriptor.Call[Request, Response]
override def callForRequest(descriptor: Descriptor, requestHeader: RequestHeader): Call[_, _] = {
descriptor.calls
.groupBy(methodForCall)
.getOrElse(requestHeader.method, Vector.empty)
.find(call => ScaladslPath.fromCallId(call.callId).regex.findFirstIn(requestHeader.uri.getPath).nonEmpty)
.get
}
override def methodForCall(call: Call[_, _]): Method =
call.callId match {
case rest: RestCallId => rest.method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import com.lightbend.lagom.scaladsl.server.LagomServiceRouter
import com.lightbend.lagom.scaladsl.server.PlayServiceCall
import play.api.Logger
import play.api.http.HttpConfiguration
import play.api.mvc
import play.api.mvc.EssentialAction
import play.api.mvc.PlayBodyParsers
import play.api.mvc.Result

import scala.collection.immutable
import scala.concurrent.ExecutionContext
Expand Down Expand Up @@ -118,4 +120,10 @@ class ScaladslServiceRouter(
}
}

override protected def processError(requestHeader: mvc.RequestHeader, throwable: Throwable): Future[Result] = {
throwable match {
case p: PayloadTooLarge => errorHandler.onClientError(requestHeader, p.errorCode.http, throwable.getMessage)
case _ => errorHandler.onServerError(requestHeader, throwable)
}
}
}