code review

sangria-graphql · Nov 8, 2024 · e405963 · e405963
1 parent 420e78d
commit e405963
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 23 deletions.
diff --git a/build.sbt b/build.sbt
@@ -26,7 +26,13 @@ ThisBuild / githubWorkflowBuildPreamble ++= List(
 )
 // Binary Incompatible Changes, we'll document.
 ThisBuild / mimaBinaryIssueFilters ++= Seq(
-  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.SlowLog.even")
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.SlowLog.even"),
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.DefaultMetricRenderer.renderHistogram"),
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.DefaultMetricRenderer.renderTimeUnit"),
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.DefaultMetricRenderer.timeUnitSuffix"),
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.QueryMetrics.findVariableNames"),
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.QueryMetrics.addComments"),
+  ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.slowlog.QueryMetrics.toComments")
 )
 
 scalacOptions += { if (isScala3.value) "-Xtarget:8" else "-target:jvm-1.8" }

diff --git a/src/main/scala/sangria/slowlog/MetricRenderer.scala b/src/main/scala/sangria/slowlog/MetricRenderer.scala
@@ -40,7 +40,7 @@ class DefaultMetricRenderer(val unit: TimeUnit) extends MetricRenderer {
     val histogram = metrics.snapshot
     val count = metrics.count
 
-    val countStr = s"[$prefix$typeName] count: $success${if (failure > 0) "/" + failure else ""}"
+    val countStr = s"[$prefix$typeName] count: $success${if (failure > 0) s"/$failure" else ""}"
 
     (countStr +: renderHistogram(count, histogram, unit).map { case (n, v) => s"$n: $v" })
       .mkString(", ")
@@ -49,7 +49,10 @@ class DefaultMetricRenderer(val unit: TimeUnit) extends MetricRenderer {
   def renderExecution(durationNanos: Long, validationNanos: Long, queryReducerNanos: Long) =
     s"[Execution Metrics] duration: ${renderDuration(durationNanos)}, validation: ${renderDuration(validationNanos)}, reducers: ${renderDuration(queryReducerNanos)}"
 
-  def renderHistogram(count: Long, snap: Snapshot, unit: TimeUnit): Vector[(String, String)] =
+  private def renderHistogram(
+      count: Long,
+      snap: Snapshot,
+      unit: TimeUnit): Vector[(String, String)] =
     if (count == 1)
       Vector("time" -> renderDuration(snap.getMax))
     else
@@ -97,10 +100,10 @@ class DefaultMetricRenderer(val unit: TimeUnit) extends MetricRenderer {
       if (unit == TimeUnit.NANOSECONDS) value
       else unit.convert(value, TimeUnit.NANOSECONDS)
 
-    ast.ObjectField(name + timeUnitSuffix, ast.BigIntValue(correctValue))
+    ast.ObjectField(s"$name$timeUnitSuffix", ast.BigIntValue(correctValue))
   }
 
-  def renderTimeUnit(unit: TimeUnit): String = unit match {
+  private def renderTimeUnit(unit: TimeUnit): String = unit match {
     case TimeUnit.DAYS => "d"
     case TimeUnit.HOURS => "h"
     case TimeUnit.MICROSECONDS => "μs"
@@ -110,7 +113,7 @@ class DefaultMetricRenderer(val unit: TimeUnit) extends MetricRenderer {
     case TimeUnit.SECONDS => "s"
   }
 
-  lazy val timeUnitSuffix: String = unit match {
+  private lazy val timeUnitSuffix: String = unit match {
     case TimeUnit.DAYS => "Day"
     case TimeUnit.HOURS => "Hour"
     case TimeUnit.MICROSECONDS => "Micros"

diff --git a/src/main/scala/sangria/slowlog/OpenTracing.scala b/src/main/scala/sangria/slowlog/OpenTracing.scala
@@ -56,16 +56,14 @@ class OpenTracing(parentSpan: Option[Span] = None, defaultOperationName: String
     val spanBuilder =
       queryVal
         .get(parentPath)
-        .map { case (parentSpan, _) =>
+        .fold(
           tracer
             .buildSpan(ctx.field.name)
-            .withTag("type", "graphql-field")
-            .asChildOf(parentSpan)
-        }
-        .getOrElse {
+            .withTag("type", "graphql-field")) { case (parentSpan, _) =>
           tracer
             .buildSpan(ctx.field.name)
             .withTag("type", "graphql-field")
+            .asChildOf(parentSpan)
         }
 
     val span = spanBuilder.start()

diff --git a/src/main/scala/sangria/slowlog/QueryMetrics.scala b/src/main/scala/sangria/slowlog/QueryMetrics.scala
@@ -79,7 +79,7 @@ case class QueryMetrics(
               if (varNames.nonEmpty) {
                 val rendered = renderer.renderVariables(variables, varNames)
 
-                if (rendered.trim.nonEmpty) toComments("\n" + rendered)
+                if (rendered.trim.nonEmpty) toComments(s"\n$rendered")
                 else Vector.empty
               } else Vector.empty
 
@@ -103,7 +103,7 @@ case class QueryMetrics(
               val paths =
                 originPaths.iterator.map {
                   case (_, hoPath) if hoPath.isEmpty => "* (query root)"
-                  case (_, hoPath) => "* " + hoPath.mkString(".")
+                  case (_, hoPath) => s"* ${hoPath.mkString(".")}"
                 }
 
               val usages =
@@ -143,10 +143,7 @@ case class QueryMetrics(
                     metricComments(
                       typeInfo,
                       typeMetrics,
-                      hoPath.mkString("", ".", if (hoPath.nonEmpty) "." else "") + path.mkString(
-                        "",
-                        ".",
-                        " "))
+                      s"${hoPath.mkString("", ".", if (hoPath.nonEmpty) "." else "")}${path.mkString("", ".", " ")}")
                   case None => Vector.empty
                 }
               }
@@ -203,7 +200,7 @@ case class QueryMetrics(
     if (varNames.nonEmpty) {
       val renderedVars = renderer.renderVariables(variables, varNames)
 
-      if (renderedVars.nonEmpty) toComments("\n" + renderedVars)
+      if (renderedVars.nonEmpty) toComments(s"\n$renderedVars")
       else Vector.empty
     } else Vector.empty
   }
@@ -270,7 +267,7 @@ case class QueryMetrics(
     builder.result()
   }
 
-  def findVariableNames(node: ast.AstNode): Vector[String] = {
+  private def findVariableNames(node: ast.AstNode): Vector[String] = {
     val names = new mutable.HashSet[String]
 
     AstVisitor.visit(
@@ -282,11 +279,13 @@ case class QueryMetrics(
     names.toVector
   }
 
-  def addComments(existing: Vector[ast.Comment], added: Vector[ast.Comment]): Vector[Comment] =
+  private def addComments(
+      existing: Vector[ast.Comment],
+      added: Vector[ast.Comment]): Vector[Comment] =
     if (existing.nonEmpty) (existing :+ ast.Comment("")) ++ added
     else added
 
-  def toComments(s: String): Vector[ast.Comment] =
+  private def toComments(s: String): Vector[ast.Comment] =
     s.split("\n").iterator.map(c => ast.Comment(c.trim)).toVector
 
   def extension(
@@ -298,7 +297,7 @@ case class QueryMetrics(
     val sortedTypes =
       fieldData.iterator
         .map { case (typeName, fields) =>
-          typeName -> fields.map { case (fieldName, metrics) =>
+          typeName -> fields.iterator.map { case (_, metrics) =>
             metrics.snapshot.get98thPercentile()
           }.max
         }

diff --git a/src/main/scala/sangria/slowlog/SlowLog.scala b/src/main/scala/sangria/slowlog/SlowLog.scala
@@ -150,7 +150,7 @@ object SlowLog {
       addExtensions)
 
   def print(
-      threshold: FiniteDuration = 0 seconds,
+      threshold: FiniteDuration = 0.seconds,
       addExtensions: Boolean = false,
       separateOp: Boolean = true)(implicit renderer: MetricRenderer): SlowLog =
     new SlowLog(