diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala index cd99503bc..289aa625e 100644 --- a/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala +++ b/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterExpr.scala @@ -24,9 +24,21 @@ trait FilterExpr extends TimeSeriesExpr object FilterExpr { - case class Stat(expr: TimeSeriesExpr, stat: String) extends FilterExpr { + /** + * Represents a basic summary stat for an input time series. + * + * @param expr + * Input expression to compute the stat over. + * @param stat + * Which summary stat to compute. + * @param str + * Optional string representation of the expression. Used for common-cases of helper + * functions to avoid duplication of the original expression. + */ + case class Stat(expr: TimeSeriesExpr, stat: String, str: Option[String] = None) + extends FilterExpr { - override def toString: String = s"$expr,$stat,:stat" + override def toString: String = str.getOrElse(s"$expr,$stat,:stat") def dataExprs: List[DataExpr] = expr.dataExprs diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterVocabulary.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterVocabulary.scala index cf25adf24..edc9056d5 100644 --- a/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterVocabulary.scala +++ b/atlas-core/src/main/scala/com/netflix/atlas/core/model/FilterVocabulary.scala @@ -220,7 +220,7 @@ object FilterVocabulary extends Vocabulary { private def rewriteStatExprs(t1: TimeSeriesExpr, t2: TimeSeriesExpr): TimeSeriesExpr = { val r = t2.rewrite { - case s: FilterExpr.StatExpr => FilterExpr.Stat(t1, s.name) + case s: FilterExpr.StatExpr => FilterExpr.Stat(t1, s.name, Some(s.toString)) } r.asInstanceOf[TimeSeriesExpr] } diff --git a/atlas-core/src/test/scala/com/netflix/atlas/core/model/FilterSuite.scala b/atlas-core/src/test/scala/com/netflix/atlas/core/model/FilterSuite.scala index 4cb7fef48..9aeeb5879 100644 --- a/atlas-core/src/test/scala/com/netflix/atlas/core/model/FilterSuite.scala +++ b/atlas-core/src/test/scala/com/netflix/atlas/core/model/FilterSuite.scala @@ -15,6 +15,7 @@ */ package com.netflix.atlas.core.model +import com.netflix.atlas.core.stacklang.Interpreter import munit.FunSuite class FilterSuite extends FunSuite { @@ -57,4 +58,30 @@ class FilterSuite extends FunSuite { val expr = MathExpr.Add(filteredExpr, filteredExpr) assert(expr.eval(context, Nil).data.isEmpty) } + + private val interpreter = Interpreter(FilterVocabulary.allWords) + + private def parse(str: String): TimeSeriesExpr = { + interpreter.execute(str).stack match { + case ModelExtractors.TimeSeriesType(t) :: Nil => t + case _ => throw new MatchError(str) + } + } + + test("toString for max,:stat") { + val expr = + "name,sps,:eq,:sum,(,app,),:by,name,sps,:eq,:sum,(,app,),:by,max,:stat,5.0,:const,:gt,:filter" + assertEquals(parse(expr).toString, expr) + } + + test("toString for stat-max") { + val expr = "name,sps,:eq,:sum,(,app,),:by,:stat-max,5.0,:const,:gt,:filter" + assertEquals(parse(expr).toString, expr) + } + + test("toString for several stat-aggr uses") { + val filter = ":stat-max,:stat-avg,5.0,:const,:add,:div,:stat-min,:gt" + val expr = s"name,sps,:eq,:sum,(,app,),:by,$filter,:filter" + assertEquals(parse(expr).toString, expr) + } } diff --git a/atlas-webapi/src/main/scala/com/netflix/atlas/webapi/ExprApi.scala b/atlas-webapi/src/main/scala/com/netflix/atlas/webapi/ExprApi.scala index 9dd01ca1f..404f81ee4 100644 --- a/atlas-webapi/src/main/scala/com/netflix/atlas/webapi/ExprApi.scala +++ b/atlas-webapi/src/main/scala/com/netflix/atlas/webapi/ExprApi.scala @@ -23,14 +23,11 @@ import akka.http.scaladsl.server.Directives._ import akka.http.scaladsl.server.Route import com.netflix.atlas.akka.CustomDirectives._ import com.netflix.atlas.akka.WebApi -import com.netflix.atlas.core.model.DataExpr import com.netflix.atlas.core.model.Expr import com.netflix.atlas.core.model.FilterExpr -import com.netflix.atlas.core.model.MathExpr import com.netflix.atlas.core.model.ModelExtractors import com.netflix.atlas.core.model.Query import com.netflix.atlas.core.model.StyleExpr -import com.netflix.atlas.core.model.TimeSeriesExpr import com.netflix.atlas.core.stacklang.Context import com.netflix.atlas.core.stacklang.Interpreter import com.netflix.atlas.core.stacklang.Word @@ -94,8 +91,8 @@ class ExprApi extends WebApi { case "query" => // Expectation is that there would be a single query on the stack stack match { - case v :: Nil => - case v :: vs => + case _ :: Nil => + case _ :: _ => val summary = Interpreter.typeSummary(stack) throw new IllegalArgumentException(s"expected a single query, found $summary") case Nil => @@ -243,7 +240,7 @@ class ExprApi extends WebApi { private def stripFilter(expr: Expr): Expr = { expr.rewrite { - case FilterExpr.Stat(e, _) => e + case FilterExpr.Stat(e, _, _) => e case FilterExpr.Filter(e, _) => e case e: FilterExpr.PriorityFilterExpr => e.expr } @@ -271,53 +268,14 @@ object ExprApi { } def normalize(exprs: List[StyleExpr]): List[String] = { - - // If named rewrites are used, then map the eval expression to match the display - // expression. This avoids the complexity of the eval expression showing up in the - // extracted data expressions. - import MathExpr.NamedRewrite - val cleaned = exprs.map { e => - val clean = e.rewrite { - case r @ NamedRewrite(_, orig: Query, _, _, _) => - r.copy(evalExpr = DataExpr.Sum(orig)) - case r @ NamedRewrite(_, orig: TimeSeriesExpr, _, _, _) => - r.copy(evalExpr = orig) - } - clean.asInstanceOf[StyleExpr] - } - - // Extract the distinct queries from all data expressions - val queries = cleaned.flatMap(_.expr.dataExprs.map(_.query)).distinct - - // Map from original query to CNF form. This is used to find the common clauses that - // can be extracted and applied to the overall query using :cq. - val cnfQueries = queries.map(q => q -> Query.cnfList(q).toSet).toMap - - // Find the set of common query clauses - val commonQueries = - if (cnfQueries.isEmpty) Set.empty[Query] - else - cnfQueries.values.reduceLeft { (s1, s2) => - s1.intersect(s2) - } - // Normalize the legend vars val styleExprs = exprs.map(normalizeLegendVars) - // Normalize without extracting common queries - val normalized = exprStrings(styleExprs, cnfQueries, Set.empty) - val fullExpr = normalized.mkString(",") - - // Normalize with extracting common queries - val cq = s":list,(,${sort(commonQueries, Set.empty)},:cq,),:each" - val normalizedCQ = cq :: exprStrings(styleExprs, cnfQueries, commonQueries) - val fullExprCQ = normalizedCQ.mkString(",") - - // If needed, then prepend a common query conversion to the list - val finalList = if (fullExpr.length < fullExprCQ.length) normalized else normalizedCQ + // Normalized expression strings + val normalized = exprStrings(styleExprs) // Reverse the list to match the order the user would expect - finalList.reverse + normalized.reverse } // For use-cases such as performing automated rewrites of expressions to move off of legacy @@ -337,19 +295,11 @@ object ExprApi { } } - private def exprStrings( - exprs: List[StyleExpr], - cnf: Map[Query, Set[Query]], - cq: Set[Query] - ): List[String] = { - - // Map from original query to sorted query without the common clauses excluded - val sortedQueries = cnf.map { case (q, qs) => q -> sort(qs, cq) } - + private def exprStrings(exprs: List[StyleExpr]): List[String] = { // Rewrite the expressions and convert to a normalized strings exprs.map { expr => val rewritten = expr.rewrite { - case q: Query => sortedQueries.getOrElse(q, q) + case q: Query => sort(q) } // Remove explicit :const, it can be determined from implicit conversion // and adds visual clutter @@ -364,13 +314,18 @@ object ExprApi { * will have an equal result query even if they were in different orders in the input * expression string. */ - private def sort(qs: Set[Query], exclude: Set[Query]): Query = { - val matches = qs.toList.filter(q => !exclude.contains(q)) - if (matches.isEmpty) - Query.True - else - matches.sortWith(_.toString < _.toString).reduce { (q1, q2) => - Query.And(q1, q2) + private def sort(query: Query): Query = { + val simplified = Query.simplify(query) + Query + .dnfList(simplified) + .map { q => + Query.cnfList(q).sortWith(_.toString < _.toString).reduce { (q1, q2) => + Query.And(q1, q2) + } + } + .sortWith(_.toString < _.toString) // order OR clauses + .reduce { (q1, q2) => + Query.Or(q1, q2) } } } diff --git a/atlas-webapi/src/test/scala/com/netflix/atlas/webapi/ExprApiSuite.scala b/atlas-webapi/src/test/scala/com/netflix/atlas/webapi/ExprApiSuite.scala index e03e1985b..39303637f 100644 --- a/atlas-webapi/src/test/scala/com/netflix/atlas/webapi/ExprApiSuite.scala +++ b/atlas-webapi/src/test/scala/com/netflix/atlas/webapi/ExprApiSuite.scala @@ -32,7 +32,7 @@ class ExprApiSuite extends MUnitRouteSuite { import scala.concurrent.duration._ - implicit val routeTestTimeout = RouteTestTimeout(5.second) + private implicit val routeTestTimeout: RouteTestTimeout = RouteTestTimeout(5.second) val endpoint = new ExprApi @@ -118,8 +118,10 @@ class ExprApiSuite extends MUnitRouteSuite { ) { assertEquals(response.status, StatusCodes.OK) val data = Json.decode[List[String]](responseAs[String]) - val expected = - List("name,cpu,:eq,:sum", "name,disk,:eq,:sum", ":list,(,nf.cluster,foo,:eq,:cq,),:each") + val expected = List( + "name,cpu,:eq,nf.cluster,foo,:eq,:and,:sum", + "name,disk,:eq,nf.cluster,foo,:eq,:and,:sum" + ) assertEquals(data, expected) } @@ -246,7 +248,7 @@ class ExprApiSuite extends MUnitRouteSuite { testGet("/api/v1/expr/strip?q=name,sps,:eq,:stat-max,5,:gt,:filter&r=style") { assertEquals(response.status, StatusCodes.OK) val data = Json.decode[List[String]](responseAs[String]) - assertEquals(data, List("name,sps,:eq,:sum,name,sps,:eq,:sum,max,:stat,5.0,:const,:gt,:filter")) + assertEquals(data, List("name,sps,:eq,:sum,:stat-max,5.0,:const,:gt,:filter")) } testGet( @@ -300,21 +302,21 @@ class ExprApiSuite extends MUnitRouteSuite { assertEquals( normalize(add.toString), List( - ":true,:sum,:true,:sum,:add", - ":list,(,app,foo,:eq,name,cpuUser,:eq,:and,:cq,),:each" + "app,foo,:eq,name,cpuUser,:eq,:and,:sum,app,foo,:eq,name,cpuUser,:eq,:and,:sum,:add" ) ) } test("normalize common query") { + // No longer performed, UI team prefers not having the cq in the + // normalized query val e1 = DataExpr.Sum(And(Equal("app", "foo"), Equal("name", "cpuUser"))) val e2 = DataExpr.Sum(And(Equal("app", "foo"), Equal("name", "cpuSystem"))) val add = StyleExpr(MathExpr.Add(e1, e2), Map.empty) assertEquals( normalize(add.toString), List( - "name,cpuUser,:eq,:sum,name,cpuSystem,:eq,:sum,:add", - ":list,(,app,foo,:eq,:cq,),:each" + "app,foo,:eq,name,cpuUser,:eq,:and,:sum,app,foo,:eq,name,cpuSystem,:eq,:and,:sum,:add" ) ) } @@ -350,9 +352,8 @@ class ExprApiSuite extends MUnitRouteSuite { assertEquals( normalize(avg), List( - "name,cpuUser,:eq,:dist-avg", - "name,cpuSystem,:eq,:max", - ":list,(,app,foo,:eq,:cq,),:each" + "app,foo,:eq,name,cpuUser,:eq,:and,:dist-avg", + "app,foo,:eq,name,cpuSystem,:eq,:and,:max" ) ) } @@ -362,6 +363,29 @@ class ExprApiSuite extends MUnitRouteSuite { assertEquals(normalize(avg), List(avg)) } + test("normalize :stat-aggr filters") { + val avg = "app,foo,:eq,name,cpuUser,:eq,:and,:sum,(,nf.cluster,),:by,:stat-max,5.0,:gt,:filter" + assertEquals(normalize(avg), List(avg)) + } + + test("normalize simplify query") { + val input = "app,foo,:eq,name,cpuUser,:eq,:and,:true,:and,:sum" + val expected = "app,foo,:eq,name,cpuUser,:eq,:and,:sum" + assertEquals(normalize(input), List(expected)) + } + + test("normalize sort query") { + val input = "name,cpuUser,:eq,app,foo,:eq,:and,:sum" + val expected = "app,foo,:eq,name,cpuUser,:eq,:and,:sum" + assertEquals(normalize(input), List(expected)) + } + + test("normalize sensible OR handling") { + val input = "name,cpuUser,:eq,app,foo,:eq,:and,name,cpuUser2,:eq,app,bar,:eq,:and,:or,:sum" + val expected = "app,bar,:eq,name,cpuUser2,:eq,:and,app,foo,:eq,name,cpuUser,:eq,:and,:or,:sum" + assertEquals(normalize(input), List(expected)) + } + test("normalize :des-fast") { val expr = "app,foo,:eq,name,cpuUser,:eq,:and,:sum,:des-fast" assertEquals(normalize(expr), List(expr))