From 6c4af6780bb800f2f51599b61d57c836cd6d08ae Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Tue, 19 Mar 2024 07:28:01 +0800 Subject: [PATCH 1/4] Improve performance of federated tracing wrapper --- .../tracing/ApolloFederatedTracing.scala | 65 +++++++++++-------- 1 file changed, 37 insertions(+), 28 deletions(-) diff --git a/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala b/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala index e3ee959ec..0a76a3412 100644 --- a/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala +++ b/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala @@ -98,6 +98,38 @@ object ApolloFederatedTracing { wrapPureValues: Boolean ): FieldWrapper[Any] = new FieldWrapper[Any](wrapPureValues) { + + private def updateState(startTime: Long, fieldInfo: FieldInfo)(result: Either[ExecutionError, ResponseValue]) = + ZQuery.succeed { + val endTime = nanoTime + val path = (PathValue.Key(fieldInfo.name) :: fieldInfo.path).toVector + val _ = ref.updateAndGet(state => + state.copy( + root = state.root.insert( + path, + Node( + id = Node.Id.ResponseName(fieldInfo.name), + startTime = startTime - state.startTime, + endTime = endTime - state.startTime, + `type` = fieldInfo.details.fieldType.toType().toString, + parentType = fieldInfo.details.parentType.map(_.toType().toString) getOrElse "", + originalFieldName = fieldInfo.details.alias.map(_ => fieldInfo.details.name) getOrElse "", + error = result.left.toOption.collectFirst { case e: ExecutionError => + Error( + e.getMessage(), + location = e.locationInfo.map(l => Location(l.line, l.column)).toSeq + ) + }.toSeq + ) + ) + ) + ) + result match { + case Right(value) => value + case _ => null + } + } + def wrap[R1]( query: ZQuery[R1, CalibanError.ExecutionError, ResponseValue], fieldInfo: FieldInfo @@ -105,34 +137,11 @@ object ApolloFederatedTracing { if (enabled.get()) ZQuery.suspend { val startTime = nanoTime - query.either.flatMap { result => - ZQuery.fromEither { - val endTime = nanoTime - val path = (PathValue.Key(fieldInfo.name) :: fieldInfo.path).toVector - val _ = ref.updateAndGet(state => - state.copy( - root = state.root.insert( - path, - Node( - id = Node.Id.ResponseName(fieldInfo.name), - startTime = startTime - state.startTime, - endTime = endTime - state.startTime, - `type` = fieldInfo.details.fieldType.toType().toString, - parentType = fieldInfo.details.parentType.map(_.toType().toString) getOrElse "", - originalFieldName = fieldInfo.details.alias.map(_ => fieldInfo.details.name) getOrElse "", - error = result.left.toOption.collectFirst { case e: ExecutionError => - Error( - e.getMessage(), - location = e.locationInfo.map(l => Location(l.line, l.column)).toSeq - ) - }.toSeq - ) - ) - ) - ) - result - } - } + val update0 = updateState(startTime, fieldInfo) _ + query.foldQuery( + error => update0(Left(error)) *> ZQuery.fail(error), + value => update0(Right(value)) + ) } else query From 03f5dc7e56fe6a71ed21e2d6cdb432d6bd425904 Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Tue, 19 Mar 2024 07:31:27 +0800 Subject: [PATCH 2/4] Make wrapper trace implicit --- core/src/main/scala/caliban/wrappers/Wrapper.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/caliban/wrappers/Wrapper.scala b/core/src/main/scala/caliban/wrappers/Wrapper.scala index a6b2b5b10..551796c3f 100644 --- a/core/src/main/scala/caliban/wrappers/Wrapper.scala +++ b/core/src/main/scala/caliban/wrappers/Wrapper.scala @@ -39,7 +39,7 @@ sealed trait Wrapper[-R] extends GraphQLAspect[Nothing, R] { self => that.withWrapper(self) // Disables tracing only for wrappers in the caliban package - final private[caliban] def trace: Trace = Trace.empty + final private[caliban] implicit def trace: Trace = Trace.empty } object Wrapper { From 2d9065e481f8756d71b9019fd6df43528eb2409f Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Sun, 24 Mar 2024 13:35:11 +0800 Subject: [PATCH 3/4] PR comment --- .../tracing/ApolloFederatedTracing.scala | 62 ++++++++++--------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala b/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala index 0a76a3412..dea4bb68a 100644 --- a/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala +++ b/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala @@ -99,36 +99,31 @@ object ApolloFederatedTracing { ): FieldWrapper[Any] = new FieldWrapper[Any](wrapPureValues) { - private def updateState(startTime: Long, fieldInfo: FieldInfo)(result: Either[ExecutionError, ResponseValue]) = - ZQuery.succeed { - val endTime = nanoTime - val path = (PathValue.Key(fieldInfo.name) :: fieldInfo.path).toVector - val _ = ref.updateAndGet(state => - state.copy( - root = state.root.insert( - path, - Node( - id = Node.Id.ResponseName(fieldInfo.name), - startTime = startTime - state.startTime, - endTime = endTime - state.startTime, - `type` = fieldInfo.details.fieldType.toType().toString, - parentType = fieldInfo.details.parentType.map(_.toType().toString) getOrElse "", - originalFieldName = fieldInfo.details.alias.map(_ => fieldInfo.details.name) getOrElse "", - error = result.left.toOption.collectFirst { case e: ExecutionError => - Error( - e.getMessage(), - location = e.locationInfo.map(l => Location(l.line, l.column)).toSeq - ) - }.toSeq - ) + private def updateState(startTime: Long, fieldInfo: FieldInfo, error: Option[ExecutionError]): Unit = { + val endTime = nanoTime + val path = (PathValue.Key(fieldInfo.name) :: fieldInfo.path).toVector + val _ = ref.updateAndGet(state => + state.copy( + root = state.root.insert( + path, + Node( + id = Node.Id.ResponseName(fieldInfo.name), + startTime = startTime - state.startTime, + endTime = endTime - state.startTime, + `type` = fieldInfo.details.fieldType.toType().toString, + parentType = fieldInfo.details.parentType.map(_.toType().toString) getOrElse "", + originalFieldName = fieldInfo.details.alias.map(_ => fieldInfo.details.name) getOrElse "", + error = error.collectFirst { case e: ExecutionError => + Error( + e.getMessage(), + location = e.locationInfo.map(l => Location(l.line, l.column)).toSeq + ) + }.toSeq ) ) ) - result match { - case Right(value) => value - case _ => null - } - } + ) + } def wrap[R1]( query: ZQuery[R1, CalibanError.ExecutionError, ResponseValue], @@ -137,10 +132,17 @@ object ApolloFederatedTracing { if (enabled.get()) ZQuery.suspend { val startTime = nanoTime - val update0 = updateState(startTime, fieldInfo) _ query.foldQuery( - error => update0(Left(error)) *> ZQuery.fail(error), - value => update0(Right(value)) + error => + ZQuery.fail { + updateState(startTime, fieldInfo, Some(error)) + error + }, + value => + ZQuery.succeed { + updateState(startTime, fieldInfo, None) + value + } ) } else query From 5e177a3dfcaddcf519650bb39542ff161e59a96c Mon Sep 17 00:00:00 2001 From: Kyri Petrou Date: Sun, 24 Mar 2024 13:46:43 +0800 Subject: [PATCH 4/4] Use cached `typeNameRepr` --- .../caliban/federation/tracing/ApolloFederatedTracing.scala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala b/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala index dea4bb68a..1f3cdce31 100644 --- a/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala +++ b/federation/src/main/scala/caliban/federation/tracing/ApolloFederatedTracing.scala @@ -110,10 +110,10 @@ object ApolloFederatedTracing { id = Node.Id.ResponseName(fieldInfo.name), startTime = startTime - state.startTime, endTime = endTime - state.startTime, - `type` = fieldInfo.details.fieldType.toType().toString, - parentType = fieldInfo.details.parentType.map(_.toType().toString) getOrElse "", + `type` = fieldInfo.details.fieldType.typeNameRepr, + parentType = fieldInfo.details.parentType.map(_.typeNameRepr) getOrElse "", originalFieldName = fieldInfo.details.alias.map(_ => fieldInfo.details.name) getOrElse "", - error = error.collectFirst { case e: ExecutionError => + error = error.map { e => Error( e.getMessage(), location = e.locationInfo.map(l => Location(l.line, l.column)).toSeq