From f99e7648fa9c386cb014b50f4858de8d30c85552 Mon Sep 17 00:00:00 2001 From: rish691 Date: Tue, 30 Nov 2021 10:34:25 +0530 Subject: [PATCH 01/31] Add promql query converter --- .../core/query/service/ExecutionContext.java | 18 +- .../core/query/service/QueryTimeRange.java | 27 +++ .../query/service/promql/PromqlQuery.java | 22 ++ .../promql/QueryRequestToPromqlConverter.java | 224 ++++++++++++++++++ .../service/pinot/ExecutionContextTest.java | 4 +- 5 files changed, 286 insertions(+), 9 deletions(-) create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java index e84612ed..b62c667b 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java @@ -48,7 +48,7 @@ public class ExecutionContext { private final LinkedHashSet allSelections; private final Optional timeSeriesPeriod; private final Filter queryRequestFilter; - private final Supplier> timeRangeDurationSupplier; + private final Supplier> queryTimeRangeSupplier; public ExecutionContext(String tenantId, QueryRequest request) { this.tenantId = tenantId; @@ -56,9 +56,9 @@ public ExecutionContext(String tenantId, QueryRequest request) { this.allSelections = new LinkedHashSet<>(); this.timeSeriesPeriod = calculateTimeSeriesPeriod(request); this.queryRequestFilter = request.getFilter(); - timeRangeDurationSupplier = + queryTimeRangeSupplier = Suppliers.memoize( - () -> findTimeRangeDuration(this.queryRequestFilter, this.timeFilterColumn)); + () -> buildQueryTimeRange(this.queryRequestFilter, this.timeFilterColumn)); analyze(request); } @@ -192,7 +192,7 @@ private void extractColumns(List columns, Expression expression) { } } - private Optional findTimeRangeDuration(Filter filter, String timeFilterColumn) { + private Optional buildQueryTimeRange(Filter filter, String timeFilterColumn) { // time filter will always be present with AND operator if (filter.getOperator() != Operator.AND) { @@ -218,11 +218,11 @@ private Optional findTimeRangeDuration(Filter filter, String timeFilte .findFirst(); if (timeRangeStart.isPresent() && timeRangeEnd.isPresent()) { - return Optional.of(Duration.ofMillis(timeRangeEnd.get() - timeRangeStart.get())); + return Optional.of(new QueryTimeRange(timeRangeStart.get(), timeRangeEnd.get())); } return filter.getChildFilterList().stream() - .map(childFilter -> this.findTimeRangeDuration(childFilter, timeFilterColumn)) + .map(childFilter -> this.buildQueryTimeRange(childFilter, timeFilterColumn)) .flatMap(Optional::stream) .findFirst(); } @@ -274,6 +274,10 @@ public Optional getTimeSeriesPeriod() { } public Optional getTimeRangeDuration() { - return timeRangeDurationSupplier.get(); + return queryTimeRangeSupplier.get().map(QueryTimeRange::getDuration); + } + + public Optional getQueryTimeRange() { + return queryTimeRangeSupplier.get(); } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java new file mode 100644 index 00000000..0c7943c8 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java @@ -0,0 +1,27 @@ +package org.hypertrace.core.query.service; + +import java.time.Duration; + +public class QueryTimeRange { + private final long startTime; + private final long endTime; + private final Duration duration; + + public QueryTimeRange(long startTime, long endTime) { + this.startTime = startTime; + this.endTime = endTime; + this.duration = Duration.ofMillis(startTime - endTime); + } + + public long getStartTime() { + return startTime; + } + + public long getEndTime() { + return endTime; + } + + public Duration getDuration() { + return duration; + } +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java new file mode 100644 index 00000000..c01cad02 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java @@ -0,0 +1,22 @@ +package org.hypertrace.core.query.service.promql; + +import java.util.List; + +public class PromqlQuery { + + private List queries; + private boolean isInstantRequest; + private long startTimeMs; + private long endTimeMs; + private long stepMs; + + public PromqlQuery( + List queries, boolean isInstantRequest, + long startTimeMs, long endTimeMs, long stepMs) { + this.queries = queries; + this.isInstantRequest = isInstantRequest; + this.startTimeMs = startTimeMs; + this.endTimeMs = endTimeMs; + this.stepMs = stepMs; + } +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java new file mode 100644 index 00000000..820fd49a --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java @@ -0,0 +1,224 @@ +package org.hypertrace.core.query.service.promql; + +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_AVG; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MAX; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; + +import com.google.common.base.Joiner; +import com.google.common.collect.Lists; +import com.google.protobuf.ByteString; +import java.time.Duration; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.commons.codec.binary.Hex; +import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.QueryTimeRange; +import org.hypertrace.core.query.service.api.Expression; +import org.hypertrace.core.query.service.api.Expression.ValueCase; +import org.hypertrace.core.query.service.api.Filter; +import org.hypertrace.core.query.service.api.LiteralConstant; +import org.hypertrace.core.query.service.api.QueryRequest; +import org.hypertrace.core.query.service.api.Value; +import org.hypertrace.core.query.service.pinot.ViewDefinition; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +class QueryRequestToPromqlConverter { + + private static final Logger LOG = LoggerFactory.getLogger(QueryRequestToPromqlConverter.class); + + private final ViewDefinition viewDefinition; + private final Joiner joiner = Joiner.on(", ").skipNulls(); + + QueryRequestToPromqlConverter( + ViewDefinition viewDefinition) { + this.viewDefinition = viewDefinition; + } + + /** + * 1. selection should be equal to group by list + * 2. number of function selection is equal to number of query fired + * 3. count(*) type of query can't be served + */ + PromqlQuery toPromql( + ExecutionContext executionContext, + QueryRequest request, + LinkedHashSet allSelections) { + String groupByList = getGroupByList(request); + + StringBuilder filter = new StringBuilder(); + convertFilter2String(request.getFilter(), filter); + + + QueryTimeRange queryTimeRange = executionContext.getQueryTimeRange().orElseThrow( + () -> new RuntimeException("Time Range missing in query")); + + List queries = allSelections.stream().filter(expression -> expression.getValueCase().equals(ValueCase.FUNCTION)) + .map(functionExpression -> { + String functionName = getFunctionName(functionExpression); + String metricName = getMetricNameFromFunction(functionExpression); + return buildQuery(metricName, functionName, groupByList, filter.toString(), queryTimeRange.getDuration().toMillis()); + }).collect(Collectors.toUnmodifiableList()); + + return new PromqlQuery( + queries, executionContext.getTimeSeriesPeriod().isEmpty(), + queryTimeRange.getStartTime(), queryTimeRange.getEndTime(), + getStep(executionContext)); + } + + private String getGroupByList(QueryRequest queryRequest) { + List groupByList = Lists.newArrayList(); + if (queryRequest.getGroupByCount() > 0) { + for (Expression expression : queryRequest.getGroupByList()) { + groupByList.add(convertColumnIdentifierExpression2String(expression)); + } + } + return String.join(",", groupByList); + } + + private long getStep(ExecutionContext executionContext) { + return executionContext + .getTimeSeriesPeriod().map(Duration::toMillis) + .orElse(-1L); + } + + private String buildQuery( + String metricName, String function, + String groupByList, String filter, long durationMillis) { + String template = "%s by (%s) (%s(%s{%s}[%sms]))"; // sum by (a1, a2) (sum_over_time(num_calls{a4="..", a5=".."}[xms])) + + return String.format(template, function, groupByList, function + "_over_time", metricName, filter, durationMillis); + } + + private String getFunctionName( + Expression functionSelection) { + switch (functionSelection.getFunction().getFunctionName().toUpperCase()) { + case QUERY_FUNCTION_SUM: + return "sum"; + case QUERY_FUNCTION_MAX: + return "max"; + case QUERY_FUNCTION_MIN: + return "min"; + case QUERY_FUNCTION_AVG: + return "avg"; + default: + throw new RuntimeException(""); + } + } + + private String getMetricNameFromFunction( + Expression functionSelection) { + // todo map columnName + return functionSelection.getColumnIdentifier().getColumnName(); + } + + private String convertColumnIdentifierExpression2String( + Expression expression) { + String logicalColumnName = expression.getColumnIdentifier().getColumnName(); + List columnNames = viewDefinition.getPhysicalColumnNames(logicalColumnName); + return joiner.join(columnNames); + } + + /** + * only `AND` operator in filter is allowed + * + * rhs of leaf filter should be literal + */ + private void convertFilter2String(Filter filter, StringBuilder builder) { + if (filter.getChildFilterCount() > 0) { + String delim = ","; + for (Filter childFilter : filter.getChildFilterList()) { + builder.append(delim); + convertFilter2String(childFilter, builder); + builder.append(" "); + } + } else { + builder.append( + convertColumnIdentifierExpression2String(filter.getLhs())); + switch (filter.getOperator()) { + case IN: + case EQ: + builder.append("="); + break; + case NEQ: + builder.append("!="); + break; + case LIKE: + builder.append("=~"); + break; + default: + // throw exception + } + builder.append(convertLiteralToString(filter.getRhs().getLiteral())); + } + } + + private String convertLiteralToString(LiteralConstant literal) { + Value value = literal.getValue(); + String ret = null; + switch (value.getValueType()) { + case STRING_ARRAY: + StringBuilder builder = new StringBuilder("\""); + for (String item : value.getStringArrayList()) { + if (builder.length() > 1) { + builder.append("|"); + } + builder.append(item); + } + builder.append("\""); + ret = builder.toString(); + break; + case BYTES_ARRAY: + builder = new StringBuilder("\""); + for (ByteString item : value.getBytesArrayList()) { + if (builder.length() > 1) { + builder.append("|"); + } + builder.append(Hex.encodeHexString(item.toByteArray())); + } + builder.append("\""); + ret = builder.toString(); + break; + case STRING: + ret = value.getString(); + break; + case LONG: + ret = String.valueOf(value.getLong()); + break; + case INT: + ret = String.valueOf(value.getInt()); + break; + case FLOAT: + ret = String.valueOf(value.getFloat()); + break; + case DOUBLE: + ret = String.valueOf(value.getDouble()); + break; + case BYTES: + ret = Hex.encodeHexString(value.getBytes().toByteArray()); + break; + case BOOL: + ret = String.valueOf(value.getBoolean()); + break; + case TIMESTAMP: + ret = String.valueOf(value.getTimestamp()); + break; + case NULL_NUMBER: + ret = "0"; + break; + case NULL_STRING: + ret = "null"; + break; + case LONG_ARRAY: + case INT_ARRAY: + case FLOAT_ARRAY: + case DOUBLE_ARRAY: + case BOOLEAN_ARRAY: + case UNRECOGNIZED: + break; + } + return ret; + } +} \ No newline at end of file diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java index 1aea0fbc..51cc1880 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java @@ -389,7 +389,7 @@ public void testEmptyGetTimeRangeDuration() { .build(); ExecutionContext context = new ExecutionContext("test", queryRequest); context.setTimeFilterColumn("SERVICE.startTime"); - assertEquals(Optional.empty(), context.getTimeRangeDuration()); + assertEquals(Optional.empty(), context.getQueryTimeRange()); } @ParameterizedTest @@ -397,7 +397,7 @@ public void testEmptyGetTimeRangeDuration() { public void testGetTimeRangeDuration(QueryRequest queryRequest) { ExecutionContext context = new ExecutionContext("test", queryRequest); context.setTimeFilterColumn("SERVICE.startTime"); - assertEquals(Optional.of(Duration.ofMinutes(60)), context.getTimeRangeDuration()); + assertEquals(Optional.of(Duration.ofMinutes(60)), context.getQueryTimeRange()); } private static Stream provideQueryRequest() { From d69709043b650eeb0261564e64d71905f6e656e7 Mon Sep 17 00:00:00 2001 From: rish691 Date: Tue, 30 Nov 2021 13:08:47 +0530 Subject: [PATCH 02/31] Update package name --- .../core/query/service/{promql => prometheus}/PromqlQuery.java | 2 +- .../{promql => prometheus}/QueryRequestToPromqlConverter.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename query-service-impl/src/main/java/org/hypertrace/core/query/service/{promql => prometheus}/PromqlQuery.java (90%) rename query-service-impl/src/main/java/org/hypertrace/core/query/service/{promql => prometheus}/QueryRequestToPromqlConverter.java (99%) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java similarity index 90% rename from query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java rename to query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java index c01cad02..f9e5d545 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/PromqlQuery.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java @@ -1,4 +1,4 @@ -package org.hypertrace.core.query.service.promql; +package org.hypertrace.core.query.service.prometheus; import java.util.List; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java similarity index 99% rename from query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java rename to query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 820fd49a..efe17b13 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -1,4 +1,4 @@ -package org.hypertrace.core.query.service.promql; +package org.hypertrace.core.query.service.prometheus; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_AVG; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MAX; From 330c8932907fd0cacd1a98e53588bc667d66d770 Mon Sep 17 00:00:00 2001 From: rish691 Date: Tue, 30 Nov 2021 13:09:07 +0530 Subject: [PATCH 03/31] spotless --- .../query/service/prometheus/PromqlQuery.java | 7 +- .../QueryRequestToPromqlConverter.java | 82 +++++++++++-------- 2 files changed, 52 insertions(+), 37 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java index f9e5d545..3acff8f2 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java @@ -11,8 +11,11 @@ public class PromqlQuery { private long stepMs; public PromqlQuery( - List queries, boolean isInstantRequest, - long startTimeMs, long endTimeMs, long stepMs) { + List queries, + boolean isInstantRequest, + long startTimeMs, + long endTimeMs, + long stepMs) { this.queries = queries; this.isInstantRequest = isInstantRequest; this.startTimeMs = startTimeMs; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index efe17b13..1bb204ce 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -32,15 +32,13 @@ class QueryRequestToPromqlConverter { private final ViewDefinition viewDefinition; private final Joiner joiner = Joiner.on(", ").skipNulls(); - QueryRequestToPromqlConverter( - ViewDefinition viewDefinition) { + QueryRequestToPromqlConverter(ViewDefinition viewDefinition) { this.viewDefinition = viewDefinition; } /** - * 1. selection should be equal to group by list - * 2. number of function selection is equal to number of query fired - * 3. count(*) type of query can't be served + * 1. selection should be equal to group by list 2. number of function selection is equal to + * number of query fired 3. count(*) type of query can't be served */ PromqlQuery toPromql( ExecutionContext executionContext, @@ -51,20 +49,32 @@ PromqlQuery toPromql( StringBuilder filter = new StringBuilder(); convertFilter2String(request.getFilter(), filter); - - QueryTimeRange queryTimeRange = executionContext.getQueryTimeRange().orElseThrow( - () -> new RuntimeException("Time Range missing in query")); - - List queries = allSelections.stream().filter(expression -> expression.getValueCase().equals(ValueCase.FUNCTION)) - .map(functionExpression -> { - String functionName = getFunctionName(functionExpression); - String metricName = getMetricNameFromFunction(functionExpression); - return buildQuery(metricName, functionName, groupByList, filter.toString(), queryTimeRange.getDuration().toMillis()); - }).collect(Collectors.toUnmodifiableList()); + QueryTimeRange queryTimeRange = + executionContext + .getQueryTimeRange() + .orElseThrow(() -> new RuntimeException("Time Range missing in query")); + + List queries = + allSelections.stream() + .filter(expression -> expression.getValueCase().equals(ValueCase.FUNCTION)) + .map( + functionExpression -> { + String functionName = getFunctionName(functionExpression); + String metricName = getMetricNameFromFunction(functionExpression); + return buildQuery( + metricName, + functionName, + groupByList, + filter.toString(), + queryTimeRange.getDuration().toMillis()); + }) + .collect(Collectors.toUnmodifiableList()); return new PromqlQuery( - queries, executionContext.getTimeSeriesPeriod().isEmpty(), - queryTimeRange.getStartTime(), queryTimeRange.getEndTime(), + queries, + executionContext.getTimeSeriesPeriod().isEmpty(), + queryTimeRange.getStartTime(), + queryTimeRange.getEndTime(), getStep(executionContext)); } @@ -79,21 +89,26 @@ private String getGroupByList(QueryRequest queryRequest) { } private long getStep(ExecutionContext executionContext) { - return executionContext - .getTimeSeriesPeriod().map(Duration::toMillis) - .orElse(-1L); + return executionContext.getTimeSeriesPeriod().map(Duration::toMillis).orElse(-1L); } private String buildQuery( - String metricName, String function, - String groupByList, String filter, long durationMillis) { - String template = "%s by (%s) (%s(%s{%s}[%sms]))"; // sum by (a1, a2) (sum_over_time(num_calls{a4="..", a5=".."}[xms])) - - return String.format(template, function, groupByList, function + "_over_time", metricName, filter, durationMillis); + String metricName, String function, String groupByList, String filter, long durationMillis) { + String template = + "%s by (%s) (%s(%s{%s}[%sms]))"; // sum by (a1, a2) (sum_over_time(num_calls{a4="..", + // a5=".."}[xms])) + + return String.format( + template, + function, + groupByList, + function + "_over_time", + metricName, + filter, + durationMillis); } - private String getFunctionName( - Expression functionSelection) { + private String getFunctionName(Expression functionSelection) { switch (functionSelection.getFunction().getFunctionName().toUpperCase()) { case QUERY_FUNCTION_SUM: return "sum"; @@ -108,14 +123,12 @@ private String getFunctionName( } } - private String getMetricNameFromFunction( - Expression functionSelection) { + private String getMetricNameFromFunction(Expression functionSelection) { // todo map columnName return functionSelection.getColumnIdentifier().getColumnName(); } - private String convertColumnIdentifierExpression2String( - Expression expression) { + private String convertColumnIdentifierExpression2String(Expression expression) { String logicalColumnName = expression.getColumnIdentifier().getColumnName(); List columnNames = viewDefinition.getPhysicalColumnNames(logicalColumnName); return joiner.join(columnNames); @@ -124,7 +137,7 @@ private String convertColumnIdentifierExpression2String( /** * only `AND` operator in filter is allowed * - * rhs of leaf filter should be literal + *

rhs of leaf filter should be literal */ private void convertFilter2String(Filter filter, StringBuilder builder) { if (filter.getChildFilterCount() > 0) { @@ -135,8 +148,7 @@ private void convertFilter2String(Filter filter, StringBuilder builder) { builder.append(" "); } } else { - builder.append( - convertColumnIdentifierExpression2String(filter.getLhs())); + builder.append(convertColumnIdentifierExpression2String(filter.getLhs())); switch (filter.getOperator()) { case IN: case EQ: @@ -221,4 +233,4 @@ private String convertLiteralToString(LiteralConstant literal) { } return ret; } -} \ No newline at end of file +} From 97ae47011e9711b5a49c92f17bc5ef5c8bc14c20 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 1 Dec 2021 16:00:48 +0530 Subject: [PATCH 04/31] Add handler and prometheus view def --- .../query/service/QueryServiceModule.java | 2 + .../PrometheusBasedRequestHandler.java | 94 +++++++++++++ .../service/prometheus/PrometheusModule.java | 18 +++ .../PrometheusRequestHandlerBuilder.java | 39 ++++++ .../prometheus/PrometheusViewDefinition.java | 96 +++++++++++++ .../QueryRequestEligibilityValidator.java | 131 ++++++++++++++++++ .../QueryRequestToPromqlConverter.java | 24 ++-- .../src/test/resources/application.conf | 27 ++++ .../resources/configs/common/application.conf | 26 ++++ 9 files changed, 443 insertions(+), 14 deletions(-) create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryServiceModule.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryServiceModule.java index b87ea932..09c5d84e 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryServiceModule.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryServiceModule.java @@ -8,6 +8,7 @@ import org.hypertrace.core.query.service.api.QueryServiceGrpc.QueryServiceImplBase; import org.hypertrace.core.query.service.pinot.PinotModule; import org.hypertrace.core.query.service.projection.ProjectionModule; +import org.hypertrace.core.query.service.prometheus.PrometheusModule; import org.hypertrace.core.serviceframework.spi.PlatformServiceLifecycle; class QueryServiceModule extends AbstractModule { @@ -31,5 +32,6 @@ protected void configure() { .in(Singleton.class); install(new PinotModule()); install(new ProjectionModule()); + install(new PrometheusModule()); } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java new file mode 100644 index 00000000..e3a2a9d7 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -0,0 +1,94 @@ +package org.hypertrace.core.query.service.prometheus; + +import com.google.common.base.Preconditions; +import com.typesafe.config.Config; +import io.reactivex.rxjava3.core.Observable; +import java.util.Optional; +import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.QueryCost; +import org.hypertrace.core.query.service.RequestHandler; +import org.hypertrace.core.query.service.api.QueryRequest; +import org.hypertrace.core.query.service.api.Row; +import org.hypertrace.core.query.service.pinot.PinotBasedRequestHandler; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class PrometheusBasedRequestHandler implements RequestHandler { + + + private static final Logger LOG = LoggerFactory.getLogger(PinotBasedRequestHandler.class); + + public static final String VIEW_DEFINITION_CONFIG_KEY = "viewDefinition"; + private static final String TENANT_COLUMN_NAME_CONFIG_KEY = "tenantAttributeName"; + private static final String START_TIME_ATTRIBUTE_NAME_CONFIG_KEY = "startTimeAttributeName"; + + private final QueryRequestEligibilityValidator queryRequestEligibilityValidator; + private final String name; + private PrometheusViewDefinition prometheusViewDefinition; + private Optional startTimeAttributeName; + private QueryRequestToPromqlConverter requestToPromqlConverter; + + PrometheusBasedRequestHandler( + String name, + Config config) { + this.name = name; + this.processConfig(config); + this.queryRequestEligibilityValidator = new QueryRequestEligibilityValidator( + prometheusViewDefinition); + } + + @Override + public String getName() { + return name; + } + + @Override + public Optional getTimeFilterColumn() { + return this.startTimeAttributeName; + } + + private void processConfig(Config config) { + + if (!config.hasPath(TENANT_COLUMN_NAME_CONFIG_KEY)) { + throw new RuntimeException( + TENANT_COLUMN_NAME_CONFIG_KEY + " is not defined in the " + name + " request handler."); + } + + String tenantColumnName = config.getString(TENANT_COLUMN_NAME_CONFIG_KEY); + this.prometheusViewDefinition = + PrometheusViewDefinition.parse(config.getConfig(VIEW_DEFINITION_CONFIG_KEY), + tenantColumnName); + + this.startTimeAttributeName = + config.hasPath(START_TIME_ATTRIBUTE_NAME_CONFIG_KEY) + ? Optional.of(config.getString(START_TIME_ATTRIBUTE_NAME_CONFIG_KEY)) + : Optional.empty(); + + this.requestToPromqlConverter = + new QueryRequestToPromqlConverter(prometheusViewDefinition); + } + + /** + * Returns a QueryCost that is an indication of whether the given query can be handled by this + * handler and if so, how costly is it to handle that query. + */ + @Override + public QueryCost canHandle(QueryRequest request, ExecutionContext executionContext) { + return queryRequestEligibilityValidator.isEligible(request, executionContext); + } + + @Override + public Observable handleRequest( + QueryRequest originalRequest, ExecutionContext executionContext) { + + // Validate QueryContext and tenant id presence + Preconditions.checkNotNull(executionContext); + Preconditions.checkNotNull(executionContext.getTenantId()); + + PromqlQuery promqlQuery = + requestToPromqlConverter.toPromql( + executionContext, originalRequest, executionContext.getAllSelections()); + + return null; + } +} \ No newline at end of file diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java new file mode 100644 index 00000000..c51ac15f --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java @@ -0,0 +1,18 @@ +package org.hypertrace.core.query.service.prometheus; + +import com.google.inject.AbstractModule; +import com.google.inject.multibindings.Multibinder; +import org.hypertrace.core.query.service.RequestHandlerBuilder; +import org.hypertrace.core.query.service.RequestHandlerClientConfigRegistry; +import org.hypertrace.core.query.service.pinot.PinotRequestHandlerBuilder; + +public class PrometheusModule extends AbstractModule { + + @Override + protected void configure() { + Multibinder.newSetBinder(binder(), RequestHandlerBuilder.class) + .addBinding() + .to(PrometheusRequestHandlerBuilder.class); + requireBinding(RequestHandlerClientConfigRegistry.class); + } +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java new file mode 100644 index 00000000..8763cea7 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java @@ -0,0 +1,39 @@ +package org.hypertrace.core.query.service.prometheus; + +import javax.inject.Inject; +import org.hypertrace.core.query.service.QueryServiceConfig.RequestHandlerClientConfig; +import org.hypertrace.core.query.service.QueryServiceConfig.RequestHandlerConfig; +import org.hypertrace.core.query.service.RequestHandler; +import org.hypertrace.core.query.service.RequestHandlerBuilder; +import org.hypertrace.core.query.service.RequestHandlerClientConfigRegistry; + +public class PrometheusRequestHandlerBuilder implements RequestHandlerBuilder { + + private final RequestHandlerClientConfigRegistry clientConfigRegistry; + + @Inject + PrometheusRequestHandlerBuilder(RequestHandlerClientConfigRegistry clientConfigRegistry) { + this.clientConfigRegistry = clientConfigRegistry; + } + + @Override + public boolean canBuild(RequestHandlerConfig config) { + return "prometheus".equals(config.getType()); + } + + @Override + public RequestHandler build(RequestHandlerConfig config) { + + RequestHandlerClientConfig clientConfig = + this.clientConfigRegistry + .get(config.getClientConfig()) + .orElseThrow( + () -> + new UnsupportedOperationException( + "Client config requested but not registered: " + config.getClientConfig())); + + // todo build prom client + + return new PrometheusBasedRequestHandler(config.getName(), config.getRequestHandlerInfo()); + } +} \ No newline at end of file diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java new file mode 100644 index 00000000..43e51f9a --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -0,0 +1,96 @@ +package org.hypertrace.core.query.service.prometheus; + +import com.google.common.collect.Maps; +import com.typesafe.config.Config; +import com.typesafe.config.ConfigUtil; +import com.typesafe.config.ConfigValue; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; + +/** + * Prometheus metric & attribute mapping for a pinot view + */ +public class PrometheusViewDefinition { + + private static final String VIEW_NAME_CONFIG_KEY = "viewName"; + private static final String FIELD_MAP_CONFIG_KEY = "fieldMap"; + private static final String METRIC_MAP_CONFIG_KEY = "metricMap"; + private static final String METRIC_NAME_CONFIG_KEY = "metricName"; + private static final String METRIC_TYPE_CONFIG_KEY = "metricType"; + + private final String viewName; + private final String tenantColumnName; + private final Map metricMap; + private final Map columnMap; + + public PrometheusViewDefinition( + String viewName, + String tenantColumnName, + Map metricMap, + Map columnMap) { + this.viewName = viewName; + this.tenantColumnName = tenantColumnName; // tenantAttributeName + this.metricMap = metricMap; + this.columnMap = columnMap; + } + + public static PrometheusViewDefinition parse(Config config, String tenantColumnName) { + String viewName = config.getString(VIEW_NAME_CONFIG_KEY); + + final Map fieldMap = Maps.newHashMap(); + Config fieldMapConfig = config.getConfig(FIELD_MAP_CONFIG_KEY); + for (Entry element : fieldMapConfig.entrySet()) { + List keys = ConfigUtil.splitPath(element.getKey()); + fieldMap.put(keys.get(0), fieldMapConfig.getString(element.getKey())); + } + + final Map metricMap = Maps.newHashMap(); + Config metricMapConfig = config.getConfig(METRIC_MAP_CONFIG_KEY); + for (Entry element : metricMapConfig.entrySet()) { + List keys = ConfigUtil.splitPath(element.getKey()); + Config metricDef = metricMapConfig.getConfig(element.getKey()); + metricMap.put( + keys.get(0), + new MetricConfig( + metricDef.getString(METRIC_NAME_CONFIG_KEY), + MetricType.valueOf(metricDef.getString(METRIC_TYPE_CONFIG_KEY)))); + } + + return new PrometheusViewDefinition( + viewName, tenantColumnName, + metricMap, fieldMap); + } + + public String getPhysicalColumnName(String logicalColumnName) { + return columnMap.get(logicalColumnName); + } + + public MetricConfig getMetricConfig(String logicalMetricName) { + return metricMap.get(logicalMetricName); + } + + public static class MetricConfig { + private final String name; + private final MetricType metricType; + + public MetricConfig(String name, + MetricType metricType) { + this.name = name; + this.metricType = metricType; + } + + public String getName() { + return name; + } + + public MetricType getMetricType() { + return metricType; + } + } + + public enum MetricType { + GAUGE, + COUNTER + } +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java new file mode 100644 index 00000000..4d58b294 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -0,0 +1,131 @@ +package org.hypertrace.core.query.service.prometheus; + +import com.google.common.base.Preconditions; +import com.google.common.collect.Sets; +import java.util.List; +import java.util.Set; +import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.QueryCost; +import org.hypertrace.core.query.service.api.Expression; +import org.hypertrace.core.query.service.api.Expression.ValueCase; +import org.hypertrace.core.query.service.api.Filter; +import org.hypertrace.core.query.service.api.Function; +import org.hypertrace.core.query.service.api.Operator; +import org.hypertrace.core.query.service.api.QueryRequest; + +/** + * Set of rules to check if the given request can be served by prometheus + */ +class QueryRequestEligibilityValidator { + + private final PrometheusViewDefinition prometheusViewDefinition; + + public QueryRequestEligibilityValidator( + PrometheusViewDefinition prometheusViewDefinition) { + this.prometheusViewDefinition = prometheusViewDefinition; + } + + QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContext) { + // orderBy to be supported later + if (queryRequest.getOrderByCount() > 0) { + return QueryCost.UNSUPPORTED; + } + + // only aggregation queries are supported + if (queryRequest.getAggregationCount() == 0 || + queryRequest.getGroupByCount() == 0 || queryRequest.getDistinctSelections()) { + return QueryCost.UNSUPPORTED; + } + + Set referencedColumns = executionContext.getReferencedColumns(); + Preconditions.checkArgument(!referencedColumns.isEmpty()); + // all the columns in the request should have a mapping in the config + for (String referencedColumn : referencedColumns) { + if (prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null + && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { + return QueryCost.UNSUPPORTED; + } + } + + if (!analyseAggregationColumns(queryRequest.getAggregationList())) { + return QueryCost.UNSUPPORTED; + } + + if (!selectionAndGroupByOnSameColumn(queryRequest.getSelectionList(), queryRequest.getGroupByList())) { + return QueryCost.UNSUPPORTED; + } + + if (!analyseFilter(queryRequest.getFilter())) { + return QueryCost.UNSUPPORTED; + } + + // value 1.0 so that prometheus is preferred over others + return new QueryCost(1.0); + } + + private boolean selectionAndGroupByOnSameColumn( + List selectionList, List groupByList) { + Set selections = Sets.newHashSet(); + for (Expression expression : selectionList) { + if (expression.getValueCase() != ValueCase.COLUMNIDENTIFIER) { + return false; + } + selections.add(expression.getColumnIdentifier().getColumnName()); + } + + for (Expression expression : groupByList) { + if (expression.getValueCase() != ValueCase.COLUMNIDENTIFIER) { + return false; + } + if (!selections.remove(expression.getColumnIdentifier().getColumnName())) { + return false; + } + } + return selections.isEmpty(); + } + + private boolean analyseAggregationColumns(List aggregationList) { + for (Expression expression : aggregationList) { + Function function = expression.getFunction(); + if (function.getArgumentsCount() > 1) { + return false; + } + if (function.getArgumentsList().get(0).getValueCase() != ValueCase.COLUMNIDENTIFIER) { + return false; + } + if (prometheusViewDefinition.getMetricConfig( + function.getArgumentsList().get(0).getColumnIdentifier().getColumnName()) == null) { + return false; + } + // todo check if the function is supported or not + } + + return true; + } + + private boolean analyseFilter(Filter filter) { + if (filter.getChildFilterCount() > 0) { + if (filter.getOperator() != Operator.AND) { + return false; + } + for (Filter childFilter : filter.getChildFilterList()) { + if (!analyseFilter(childFilter)) { + return false; + } + } + } + + // filter rhs should be literal only + if (filter.getRhs().getValueCase() != ValueCase.LITERAL) { + return false; + } + + // filter lhs should be column + if (filter.getLhs().getValueCase() != ValueCase.COLUMNIDENTIFIER) { + return false; + } + + // todo check for valid operators here + return true; + } +} \ No newline at end of file diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 1bb204ce..f2509f27 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -5,7 +5,6 @@ import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; -import com.google.common.base.Joiner; import com.google.common.collect.Lists; import com.google.protobuf.ByteString; import java.time.Duration; @@ -21,7 +20,7 @@ import org.hypertrace.core.query.service.api.LiteralConstant; import org.hypertrace.core.query.service.api.QueryRequest; import org.hypertrace.core.query.service.api.Value; -import org.hypertrace.core.query.service.pinot.ViewDefinition; +import org.hypertrace.core.query.service.prometheus.PrometheusViewDefinition.MetricConfig; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -29,11 +28,10 @@ class QueryRequestToPromqlConverter { private static final Logger LOG = LoggerFactory.getLogger(QueryRequestToPromqlConverter.class); - private final ViewDefinition viewDefinition; - private final Joiner joiner = Joiner.on(", ").skipNulls(); + private final PrometheusViewDefinition prometheusViewDefinition; - QueryRequestToPromqlConverter(ViewDefinition viewDefinition) { - this.viewDefinition = viewDefinition; + QueryRequestToPromqlConverter(PrometheusViewDefinition prometheusViewDefinition) { + this.prometheusViewDefinition = prometheusViewDefinition; } /** @@ -60,9 +58,9 @@ PromqlQuery toPromql( .map( functionExpression -> { String functionName = getFunctionName(functionExpression); - String metricName = getMetricNameFromFunction(functionExpression); + MetricConfig metricConfig = getMetricConfigForFunction(functionExpression); return buildQuery( - metricName, + metricConfig.getName(), functionName, groupByList, filter.toString(), @@ -102,7 +100,7 @@ private String buildQuery( template, function, groupByList, - function + "_over_time", + function + "_over_time", // assuming gauge type of metric metricName, filter, durationMillis); @@ -123,15 +121,13 @@ private String getFunctionName(Expression functionSelection) { } } - private String getMetricNameFromFunction(Expression functionSelection) { - // todo map columnName - return functionSelection.getColumnIdentifier().getColumnName(); + private MetricConfig getMetricConfigForFunction(Expression functionSelection) { + return prometheusViewDefinition.getMetricConfig(functionSelection.getColumnIdentifier().getColumnName()); } private String convertColumnIdentifierExpression2String(Expression expression) { String logicalColumnName = expression.getColumnIdentifier().getColumnName(); - List columnNames = viewDefinition.getPhysicalColumnNames(logicalColumnName); - return joiner.join(columnNames); + return prometheusViewDefinition.getPhysicalColumnName(logicalColumnName); } /** diff --git a/query-service-impl/src/test/resources/application.conf b/query-service-impl/src/test/resources/application.conf index 0be1f02a..c149dc75 100644 --- a/query-service-impl/src/test/resources/application.conf +++ b/query-service-impl/src/test/resources/application.conf @@ -160,5 +160,32 @@ service.config = { } } } + { + name = raw-service-prometheus-handler + type = prometheus + clientConfig = "" + requestHandlerInfo = { + tenantAttributeName = tenant_id + metricMap = { + "API.numCalls" = { + "metricName": "num_calls", + "metricType": "gauge" + } + "SERVICE.numCalls" = { + "metricName": "num_calls", + "metricType": "gauge" + } + } + fieldMap = { + "SERVICE.id": "service_id", + "SERVICE.name": "service_name", + "API.id": "api_id", + "API.name": "api_name", + "EVENT.protocolName": "protocol_name", + "EVENT.status_code": "status_code", + "API_TRACE.status_code": "status_code" + } + } + } ] } diff --git a/query-service/src/main/resources/configs/common/application.conf b/query-service/src/main/resources/configs/common/application.conf index 0f5a2bf1..d385455d 100644 --- a/query-service/src/main/resources/configs/common/application.conf +++ b/query-service/src/main/resources/configs/common/application.conf @@ -315,6 +315,32 @@ service.config = { } } } + { + name = raw-service-prometheus-handler + type = prometheus + requestHandlerInfo = { + tenantAttributeName = tenant_id + metricMap = { + "API.numCalls" = { + "metricName": "num_calls", + "metricType": "gauge" + } + "SERVICE.numCalls" = { + "metricName": "num_calls", + "metricType": "gauge" + } + } + fieldMap = { + "SERVICE.id": "service_id", + "SERVICE.name": "service_name", + "API.id": "api_id", + "API.name": "api_name", + "EVENT.protocolName": "protocol_name", + "EVENT.status_code": "status_code", + "API_TRACE.status_code": "status_code" + } + } + } ] } From 69c5ea7b02070113dbb8d719ef7b7375024666b7 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 1 Dec 2021 16:01:13 +0530 Subject: [PATCH 05/31] Spotless --- .../PrometheusBasedRequestHandler.java | 18 +++++------ .../service/prometheus/PrometheusModule.java | 1 - .../PrometheusRequestHandlerBuilder.java | 2 +- .../prometheus/PrometheusViewDefinition.java | 7 ++--- .../QueryRequestEligibilityValidator.java | 30 +++++++++---------- .../QueryRequestToPromqlConverter.java | 3 +- 6 files changed, 27 insertions(+), 34 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index e3a2a9d7..a88b1e1a 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -15,7 +15,6 @@ public class PrometheusBasedRequestHandler implements RequestHandler { - private static final Logger LOG = LoggerFactory.getLogger(PinotBasedRequestHandler.class); public static final String VIEW_DEFINITION_CONFIG_KEY = "viewDefinition"; @@ -28,13 +27,11 @@ public class PrometheusBasedRequestHandler implements RequestHandler { private Optional startTimeAttributeName; private QueryRequestToPromqlConverter requestToPromqlConverter; - PrometheusBasedRequestHandler( - String name, - Config config) { + PrometheusBasedRequestHandler(String name, Config config) { this.name = name; this.processConfig(config); - this.queryRequestEligibilityValidator = new QueryRequestEligibilityValidator( - prometheusViewDefinition); + this.queryRequestEligibilityValidator = + new QueryRequestEligibilityValidator(prometheusViewDefinition); } @Override @@ -56,16 +53,15 @@ private void processConfig(Config config) { String tenantColumnName = config.getString(TENANT_COLUMN_NAME_CONFIG_KEY); this.prometheusViewDefinition = - PrometheusViewDefinition.parse(config.getConfig(VIEW_DEFINITION_CONFIG_KEY), - tenantColumnName); + PrometheusViewDefinition.parse( + config.getConfig(VIEW_DEFINITION_CONFIG_KEY), tenantColumnName); this.startTimeAttributeName = config.hasPath(START_TIME_ATTRIBUTE_NAME_CONFIG_KEY) ? Optional.of(config.getString(START_TIME_ATTRIBUTE_NAME_CONFIG_KEY)) : Optional.empty(); - this.requestToPromqlConverter = - new QueryRequestToPromqlConverter(prometheusViewDefinition); + this.requestToPromqlConverter = new QueryRequestToPromqlConverter(prometheusViewDefinition); } /** @@ -91,4 +87,4 @@ public Observable handleRequest( return null; } -} \ No newline at end of file +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java index c51ac15f..b19c609f 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusModule.java @@ -4,7 +4,6 @@ import com.google.inject.multibindings.Multibinder; import org.hypertrace.core.query.service.RequestHandlerBuilder; import org.hypertrace.core.query.service.RequestHandlerClientConfigRegistry; -import org.hypertrace.core.query.service.pinot.PinotRequestHandlerBuilder; public class PrometheusModule extends AbstractModule { diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java index 8763cea7..c8df6d03 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusRequestHandlerBuilder.java @@ -36,4 +36,4 @@ public RequestHandler build(RequestHandlerConfig config) { return new PrometheusBasedRequestHandler(config.getName(), config.getRequestHandlerInfo()); } -} \ No newline at end of file +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 43e51f9a..f40d87d8 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -8,9 +8,7 @@ import java.util.Map; import java.util.Map.Entry; -/** - * Prometheus metric & attribute mapping for a pinot view - */ +/** Prometheus metric & attribute mapping for a pinot view */ public class PrometheusViewDefinition { private static final String VIEW_NAME_CONFIG_KEY = "viewName"; @@ -74,8 +72,7 @@ public static class MetricConfig { private final String name; private final MetricType metricType; - public MetricConfig(String name, - MetricType metricType) { + public MetricConfig(String name, MetricType metricType) { this.name = name; this.metricType = metricType; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 4d58b294..4a90df65 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -13,15 +13,12 @@ import org.hypertrace.core.query.service.api.Operator; import org.hypertrace.core.query.service.api.QueryRequest; -/** - * Set of rules to check if the given request can be served by prometheus - */ +/** Set of rules to check if the given request can be served by prometheus */ class QueryRequestEligibilityValidator { private final PrometheusViewDefinition prometheusViewDefinition; - public QueryRequestEligibilityValidator( - PrometheusViewDefinition prometheusViewDefinition) { + public QueryRequestEligibilityValidator(PrometheusViewDefinition prometheusViewDefinition) { this.prometheusViewDefinition = prometheusViewDefinition; } @@ -32,8 +29,9 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex } // only aggregation queries are supported - if (queryRequest.getAggregationCount() == 0 || - queryRequest.getGroupByCount() == 0 || queryRequest.getDistinctSelections()) { + if (queryRequest.getAggregationCount() == 0 + || queryRequest.getGroupByCount() == 0 + || queryRequest.getDistinctSelections()) { return QueryCost.UNSUPPORTED; } @@ -51,14 +49,15 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex return QueryCost.UNSUPPORTED; } - if (!selectionAndGroupByOnSameColumn(queryRequest.getSelectionList(), queryRequest.getGroupByList())) { + if (!selectionAndGroupByOnSameColumn( + queryRequest.getSelectionList(), queryRequest.getGroupByList())) { return QueryCost.UNSUPPORTED; } - + if (!analyseFilter(queryRequest.getFilter())) { return QueryCost.UNSUPPORTED; } - + // value 1.0 so that prometheus is preferred over others return new QueryCost(1.0); } @@ -94,7 +93,8 @@ private boolean analyseAggregationColumns(List aggregationList) { return false; } if (prometheusViewDefinition.getMetricConfig( - function.getArgumentsList().get(0).getColumnIdentifier().getColumnName()) == null) { + function.getArgumentsList().get(0).getColumnIdentifier().getColumnName()) + == null) { return false; } // todo check if the function is supported or not @@ -115,12 +115,12 @@ private boolean analyseFilter(Filter filter) { } } - // filter rhs should be literal only + // filter rhs should be literal only if (filter.getRhs().getValueCase() != ValueCase.LITERAL) { return false; } - - // filter lhs should be column + + // filter lhs should be column if (filter.getLhs().getValueCase() != ValueCase.COLUMNIDENTIFIER) { return false; } @@ -128,4 +128,4 @@ private boolean analyseFilter(Filter filter) { // todo check for valid operators here return true; } -} \ No newline at end of file +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index f2509f27..63a311f3 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -122,7 +122,8 @@ private String getFunctionName(Expression functionSelection) { } private MetricConfig getMetricConfigForFunction(Expression functionSelection) { - return prometheusViewDefinition.getMetricConfig(functionSelection.getColumnIdentifier().getColumnName()); + return prometheusViewDefinition.getMetricConfig( + functionSelection.getColumnIdentifier().getColumnName()); } private String convertColumnIdentifierExpression2String(Expression expression) { From a683ce9d16e1dac56b8e09d604c82014eac73eb3 Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 09:41:18 +0530 Subject: [PATCH 06/31] Add test for promql converter --- query-service-impl/config.yml | 2 + .../core/query/service/ExecutionContext.java | 9 +- .../core/query/service/QueryRequestUtil.java | 11 +++ .../PrometheusBasedRequestHandler.java | 4 +- .../prometheus/PrometheusViewDefinition.java | 11 ++- .../query/service/prometheus/PromqlQuery.java | 14 ++- .../QueryRequestEligibilityValidator.java | 14 ++- .../QueryRequestToPromqlConverter.java | 28 ++++-- .../QueryRequestToPromqlConverterTest.java | 94 +++++++++++++++++++ .../resources/prometheus_request_handler.conf | 42 +++++++++ 10 files changed, 202 insertions(+), 27 deletions(-) create mode 100644 query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java create mode 100644 query-service-impl/src/test/resources/prometheus_request_handler.conf diff --git a/query-service-impl/config.yml b/query-service-impl/config.yml index 0d047ff4..9257d0d3 100644 --- a/query-service-impl/config.yml +++ b/query-service-impl/config.yml @@ -1,3 +1,5 @@ + + logging: level: INFO loggers: diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java index b62c667b..264e8996 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java @@ -65,7 +65,7 @@ public ExecutionContext(String tenantId, QueryRequest request) { private Optional calculateTimeSeriesPeriod(QueryRequest request) { if (request.getGroupByCount() > 0) { for (Expression expression : request.getGroupByList()) { - if (isDateTimeFunction(expression)) { + if (QueryRequestUtil.isDateTimeFunction(expression)) { String timeSeriesPeriod = expression .getFunction() @@ -218,7 +218,7 @@ private Optional buildQueryTimeRange(Filter filter, String timeF .findFirst(); if (timeRangeStart.isPresent() && timeRangeEnd.isPresent()) { - return Optional.of(new QueryTimeRange(timeRangeStart.get(), timeRangeEnd.get())); + return Optional.of(new QueryTimeRange(timeRangeEnd.get(), timeRangeStart.get())); } return filter.getChildFilterList().stream() @@ -240,11 +240,6 @@ private Duration parseDuration(String timeSeriesPeriod) { return Duration.of(amount, unit); } - private boolean isDateTimeFunction(Expression expression) { - return expression.getValueCase() == ValueCase.FUNCTION - && expression.getFunction().getFunctionName().equals("dateTimeConvert"); - } - public void setTimeFilterColumn(String timeFilterColumn) { this.timeFilterColumn = timeFilterColumn; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java index 402a8386..977ee28e 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java @@ -2,6 +2,7 @@ import org.hypertrace.core.query.service.api.ColumnIdentifier; import org.hypertrace.core.query.service.api.Expression; +import org.hypertrace.core.query.service.api.Expression.ValueCase; import org.hypertrace.core.query.service.api.LiteralConstant; import org.hypertrace.core.query.service.api.Value; import org.hypertrace.core.query.service.api.ValueType; @@ -68,4 +69,14 @@ public static Expression createNullNumberLiteralExpression() { .setValue(Value.newBuilder().setValueType(ValueType.NULL_NUMBER))) .build(); } + + public static boolean isDateTimeFunction(Expression expression) { + return expression.getValueCase() == ValueCase.FUNCTION + && expression.getFunction().getFunctionName().equals("dateTimeConvert"); + } + + public static boolean isTimeColumn(String columnName) { + return columnName.contains("startTime") || + columnName.contains("endTime"); + } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index a88b1e1a..bb42a02d 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -17,7 +17,7 @@ public class PrometheusBasedRequestHandler implements RequestHandler { private static final Logger LOG = LoggerFactory.getLogger(PinotBasedRequestHandler.class); - public static final String VIEW_DEFINITION_CONFIG_KEY = "viewDefinition"; + public static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition"; private static final String TENANT_COLUMN_NAME_CONFIG_KEY = "tenantAttributeName"; private static final String START_TIME_ATTRIBUTE_NAME_CONFIG_KEY = "startTimeAttributeName"; @@ -87,4 +87,4 @@ public Observable handleRequest( return null; } -} +} \ No newline at end of file diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index f40d87d8..0d508c16 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -1,12 +1,14 @@ package org.hypertrace.core.query.service.prometheus; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import com.typesafe.config.Config; import com.typesafe.config.ConfigUtil; import com.typesafe.config.ConfigValue; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Set; /** Prometheus metric & attribute mapping for a pinot view */ public class PrometheusViewDefinition { @@ -45,11 +47,16 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN final Map metricMap = Maps.newHashMap(); Config metricMapConfig = config.getConfig(METRIC_MAP_CONFIG_KEY); + Set metricLogicalNames = Sets.newHashSet(); for (Entry element : metricMapConfig.entrySet()) { List keys = ConfigUtil.splitPath(element.getKey()); - Config metricDef = metricMapConfig.getConfig(element.getKey()); + metricLogicalNames.add(keys.get(0) + "." + keys.get(1)); + } + + for (String metricLogicalName : metricLogicalNames) { + Config metricDef = metricMapConfig.getConfig(metricLogicalName); metricMap.put( - keys.get(0), + metricLogicalName, new MetricConfig( metricDef.getString(METRIC_NAME_CONFIG_KEY), MetricType.valueOf(metricDef.getString(METRIC_TYPE_CONFIG_KEY)))); diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java index 3acff8f2..36043c6b 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java @@ -4,11 +4,11 @@ public class PromqlQuery { - private List queries; - private boolean isInstantRequest; - private long startTimeMs; - private long endTimeMs; - private long stepMs; + private final List queries; + private final boolean isInstantRequest; + private final long startTimeMs; + private final long endTimeMs; + private final long stepMs; public PromqlQuery( List queries, @@ -22,4 +22,8 @@ public PromqlQuery( this.endTimeMs = endTimeMs; this.stepMs = stepMs; } + + public List getQueries() { + return queries; + } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 4a90df65..19e4e4d1 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -6,6 +6,7 @@ import java.util.Set; import org.hypertrace.core.query.service.ExecutionContext; import org.hypertrace.core.query.service.QueryCost; +import org.hypertrace.core.query.service.QueryRequestUtil; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Expression.ValueCase; import org.hypertrace.core.query.service.api.Filter; @@ -39,7 +40,8 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex Preconditions.checkArgument(!referencedColumns.isEmpty()); // all the columns in the request should have a mapping in the config for (String referencedColumn : referencedColumns) { - if (prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null + if (!QueryRequestUtil.isTimeColumn(referencedColumn) && + prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { return QueryCost.UNSUPPORTED; } @@ -73,6 +75,10 @@ private boolean selectionAndGroupByOnSameColumn( } for (Expression expression : groupByList) { + // skip datetime convert group by + if (QueryRequestUtil.isDateTimeFunction(expression)) { + continue; + } if (expression.getValueCase() != ValueCase.COLUMNIDENTIFIER) { return false; } @@ -86,6 +92,10 @@ private boolean selectionAndGroupByOnSameColumn( private boolean analyseAggregationColumns(List aggregationList) { for (Expression expression : aggregationList) { Function function = expression.getFunction(); + // skip dateTimeConvert function as it is part of the prometheus api call + if (QueryRequestUtil.isDateTimeFunction(expression)) { + continue; + } if (function.getArgumentsCount() > 1) { return false; } @@ -128,4 +138,6 @@ private boolean analyseFilter(Filter filter) { // todo check for valid operators here return true; } + + } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 63a311f3..3dbb224c 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -1,6 +1,7 @@ package org.hypertrace.core.query.service.prometheus; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_AVG; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_COUNT; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MAX; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; @@ -8,11 +9,13 @@ import com.google.common.collect.Lists; import com.google.protobuf.ByteString; import java.time.Duration; +import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; import java.util.stream.Collectors; import org.apache.commons.codec.binary.Hex; import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.QueryRequestUtil; import org.hypertrace.core.query.service.QueryTimeRange; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Expression.ValueCase; @@ -44,8 +47,8 @@ PromqlQuery toPromql( LinkedHashSet allSelections) { String groupByList = getGroupByList(request); - StringBuilder filter = new StringBuilder(); - convertFilter2String(request.getFilter(), filter); + List filterList = new ArrayList<>(); + convertFilter2String(request.getFilter(), filterList); QueryTimeRange queryTimeRange = executionContext @@ -63,7 +66,7 @@ PromqlQuery toPromql( metricConfig.getName(), functionName, groupByList, - filter.toString(), + String.join(", ", filterList), queryTimeRange.getDuration().toMillis()); }) .collect(Collectors.toUnmodifiableList()); @@ -83,7 +86,7 @@ private String getGroupByList(QueryRequest queryRequest) { groupByList.add(convertColumnIdentifierExpression2String(expression)); } } - return String.join(",", groupByList); + return String.join(", ", groupByList); } private long getStep(ExecutionContext executionContext) { @@ -116,6 +119,8 @@ private String getFunctionName(Expression functionSelection) { return "min"; case QUERY_FUNCTION_AVG: return "avg"; + case QUERY_FUNCTION_COUNT: + return "count"; default: throw new RuntimeException(""); } @@ -123,7 +128,7 @@ private String getFunctionName(Expression functionSelection) { private MetricConfig getMetricConfigForFunction(Expression functionSelection) { return prometheusViewDefinition.getMetricConfig( - functionSelection.getColumnIdentifier().getColumnName()); + functionSelection.getFunction().getArgumentsList().get(0).getColumnIdentifier().getColumnName()); } private String convertColumnIdentifierExpression2String(Expression expression) { @@ -136,15 +141,17 @@ private String convertColumnIdentifierExpression2String(Expression expression) { * *

rhs of leaf filter should be literal */ - private void convertFilter2String(Filter filter, StringBuilder builder) { + private void convertFilter2String(Filter filter, List filterList) { if (filter.getChildFilterCount() > 0) { - String delim = ","; for (Filter childFilter : filter.getChildFilterList()) { - builder.append(delim); - convertFilter2String(childFilter, builder); - builder.append(" "); + convertFilter2String(childFilter, filterList); } } else { + if (filter.getLhs().getValueCase() == ValueCase.COLUMNIDENTIFIER && + QueryRequestUtil.isTimeColumn(filter.getLhs().getColumnIdentifier().getColumnName())) { + return; + } + StringBuilder builder = new StringBuilder(); builder.append(convertColumnIdentifierExpression2String(filter.getLhs())); switch (filter.getOperator()) { case IN: @@ -161,6 +168,7 @@ private void convertFilter2String(Filter filter, StringBuilder builder) { // throw exception } builder.append(convertLiteralToString(filter.getRhs().getLiteral())); + filterList.add(builder.toString()); } } diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java new file mode 100644 index 00000000..48074ce7 --- /dev/null +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -0,0 +1,94 @@ +package org.hypertrace.core.query.service.prometheus; + +import static java.util.Objects.requireNonNull; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createColumnExpression; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createFunctionExpression; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createTimeFilter; + +import com.typesafe.config.Config; +import com.typesafe.config.ConfigFactory; +import java.util.LinkedHashSet; +import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.api.Expression; +import org.hypertrace.core.query.service.api.Filter; +import org.hypertrace.core.query.service.api.Operator; +import org.hypertrace.core.query.service.api.QueryRequest; +import org.hypertrace.core.query.service.api.QueryRequest.Builder; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class QueryRequestToPromqlConverterTest { + + private static final String TENANT_COLUMN_NAME = "tenant_id"; + + private static final String TEST_REQUEST_HANDLER_CONFIG_FILE = "prometheus_request_handler.conf"; + + @Test + public void testQueryWithGroupByWithMultipleAggregates() { + QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQuery(); + Builder builder = QueryRequest.newBuilder(orderByQueryRequest); + builder.setLimit(20); + PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); + + QueryRequest queryRequest = builder.build(); + + ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); + executionContext.setTimeFilterColumn("SERVICE.startTime"); + PromqlQuery promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) + .toPromql(executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); + + + String query1 = "count by (service_name, api_name) (count_over_time(error_count{}[100ms]))"; + String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{}[100ms]))"; + + Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); + Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); + } + + private QueryRequest buildMultipleGroupByMultipleAggQuery() { + Builder builder = QueryRequest.newBuilder(); + builder.addAggregation(createFunctionExpression("Count", createColumnExpression("SERVICE.errorCount").build())); + Expression avg = + createFunctionExpression("AVG", createColumnExpression("SERVICE.numCalls").build()); + builder.addAggregation(avg); + + Filter startTimeFilter = + createTimeFilter("SERVICE.startTime", Operator.GT, 100L); + Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200L); + + Filter andFilter = + Filter.newBuilder() + .setOperator(Operator.AND) + .addChildFilter(startTimeFilter) + .addChildFilter(endTimeFilter) + .build(); + builder.setFilter(andFilter); + + builder.addGroupBy(createColumnExpression("SERVICE.name")); + builder.addGroupBy(createColumnExpression("API.name")); + return builder.build(); + } + + + private PrometheusViewDefinition getDefaultPrometheusViewDefinition() { + Config fileConfig = + ConfigFactory.parseURL( + requireNonNull( + QueryRequestToPromqlConverterTest.class + .getClassLoader() + .getResource(TEST_REQUEST_HANDLER_CONFIG_FILE))); + + return PrometheusViewDefinition.parse( + fileConfig.getConfig("requestHandlerInfo.prometheusViewDefinition"), TENANT_COLUMN_NAME); + } + + private LinkedHashSet createSelectionsFromQueryRequest(QueryRequest queryRequest) { + LinkedHashSet selections = new LinkedHashSet<>(); + + selections.addAll(queryRequest.getGroupByList()); + selections.addAll(queryRequest.getSelectionList()); + selections.addAll(queryRequest.getAggregationList()); + + return selections; + } +} \ No newline at end of file diff --git a/query-service-impl/src/test/resources/prometheus_request_handler.conf b/query-service-impl/src/test/resources/prometheus_request_handler.conf new file mode 100644 index 00000000..56c22e38 --- /dev/null +++ b/query-service-impl/src/test/resources/prometheus_request_handler.conf @@ -0,0 +1,42 @@ +{ + name = raw-service-prometheus-handler + type = prometheus + clientConfig = "" + requestHandlerInfo { + tenantAttributeName = tenant_id + prometheusViewDefinition { + viewName = rawServiceView + metricMap { + API { + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + } + } + SERVICE { + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + }, + errorCount { + metricName: "error_count", + metricType: "GAUGE" + } + } + } + fieldMap { + "SERVICE.id": "service_id", + "SERVICE.name": "service_name", + "API.id": "api_id", + "API.name": "api_name", + "EVENT.protocolName": "protocol_name", + "EVENT.status_code": "status_code", + "API_TRACE.status_code": "status_code", + "API.startTime": "start_time_millis", + "API.endTime": "end_time_millis", + "SERVICE.startTime": "start_time_millis", + "SERVICE.endTime": "end_time_millis", + } + } + } +} \ No newline at end of file From 45f3cd4baae0dae70c51c68d5e0c4a6162e497d9 Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 09:41:28 +0530 Subject: [PATCH 07/31] spotless --- .../core/query/service/QueryRequestUtil.java | 3 +-- .../PrometheusBasedRequestHandler.java | 2 +- .../QueryRequestEligibilityValidator.java | 6 ++---- .../QueryRequestToPromqlConverter.java | 11 ++++++++--- .../QueryRequestToPromqlConverterTest.java | 16 ++++++++-------- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java index 977ee28e..cbbc8340 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java @@ -76,7 +76,6 @@ public static boolean isDateTimeFunction(Expression expression) { } public static boolean isTimeColumn(String columnName) { - return columnName.contains("startTime") || - columnName.contains("endTime"); + return columnName.contains("startTime") || columnName.contains("endTime"); } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index bb42a02d..8547000f 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -87,4 +87,4 @@ public Observable handleRequest( return null; } -} \ No newline at end of file +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 19e4e4d1..26e24830 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -40,8 +40,8 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex Preconditions.checkArgument(!referencedColumns.isEmpty()); // all the columns in the request should have a mapping in the config for (String referencedColumn : referencedColumns) { - if (!QueryRequestUtil.isTimeColumn(referencedColumn) && - prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null + if (!QueryRequestUtil.isTimeColumn(referencedColumn) + && prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { return QueryCost.UNSUPPORTED; } @@ -138,6 +138,4 @@ private boolean analyseFilter(Filter filter) { // todo check for valid operators here return true; } - - } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 3dbb224c..e6cccb38 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -128,7 +128,12 @@ private String getFunctionName(Expression functionSelection) { private MetricConfig getMetricConfigForFunction(Expression functionSelection) { return prometheusViewDefinition.getMetricConfig( - functionSelection.getFunction().getArgumentsList().get(0).getColumnIdentifier().getColumnName()); + functionSelection + .getFunction() + .getArgumentsList() + .get(0) + .getColumnIdentifier() + .getColumnName()); } private String convertColumnIdentifierExpression2String(Expression expression) { @@ -147,8 +152,8 @@ private void convertFilter2String(Filter filter, List filterList) { convertFilter2String(childFilter, filterList); } } else { - if (filter.getLhs().getValueCase() == ValueCase.COLUMNIDENTIFIER && - QueryRequestUtil.isTimeColumn(filter.getLhs().getColumnIdentifier().getColumnName())) { + if (filter.getLhs().getValueCase() == ValueCase.COLUMNIDENTIFIER + && QueryRequestUtil.isTimeColumn(filter.getLhs().getColumnIdentifier().getColumnName())) { return; } StringBuilder builder = new StringBuilder(); diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index 48074ce7..9f2384c3 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -34,9 +34,10 @@ public void testQueryWithGroupByWithMultipleAggregates() { ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromqlQuery promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) - .toPromql(executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); - + PromqlQuery promqlQuery = + new QueryRequestToPromqlConverter(prometheusViewDefinition) + .toPromql( + executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); String query1 = "count by (service_name, api_name) (count_over_time(error_count{}[100ms]))"; String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{}[100ms]))"; @@ -47,13 +48,13 @@ public void testQueryWithGroupByWithMultipleAggregates() { private QueryRequest buildMultipleGroupByMultipleAggQuery() { Builder builder = QueryRequest.newBuilder(); - builder.addAggregation(createFunctionExpression("Count", createColumnExpression("SERVICE.errorCount").build())); + builder.addAggregation( + createFunctionExpression("Count", createColumnExpression("SERVICE.errorCount").build())); Expression avg = createFunctionExpression("AVG", createColumnExpression("SERVICE.numCalls").build()); builder.addAggregation(avg); - Filter startTimeFilter = - createTimeFilter("SERVICE.startTime", Operator.GT, 100L); + Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100L); Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200L); Filter andFilter = @@ -69,7 +70,6 @@ private QueryRequest buildMultipleGroupByMultipleAggQuery() { return builder.build(); } - private PrometheusViewDefinition getDefaultPrometheusViewDefinition() { Config fileConfig = ConfigFactory.parseURL( @@ -91,4 +91,4 @@ private LinkedHashSet createSelectionsFromQueryRequest(QueryRequest return selections; } -} \ No newline at end of file +} From 870ed974f35f563afabcf5f26d04986ca5f5cc45 Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 09:43:42 +0530 Subject: [PATCH 08/31] remove prom config from main --- .../resources/configs/common/application.conf | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/query-service/src/main/resources/configs/common/application.conf b/query-service/src/main/resources/configs/common/application.conf index d385455d..0f5a2bf1 100644 --- a/query-service/src/main/resources/configs/common/application.conf +++ b/query-service/src/main/resources/configs/common/application.conf @@ -315,32 +315,6 @@ service.config = { } } } - { - name = raw-service-prometheus-handler - type = prometheus - requestHandlerInfo = { - tenantAttributeName = tenant_id - metricMap = { - "API.numCalls" = { - "metricName": "num_calls", - "metricType": "gauge" - } - "SERVICE.numCalls" = { - "metricName": "num_calls", - "metricType": "gauge" - } - } - fieldMap = { - "SERVICE.id": "service_id", - "SERVICE.name": "service_name", - "API.id": "api_id", - "API.name": "api_name", - "EVENT.protocolName": "protocol_name", - "EVENT.status_code": "status_code", - "API_TRACE.status_code": "status_code" - } - } - } ] } From b608e80ea62b730c3f84458427073d59aae7b59c Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 11:59:44 +0530 Subject: [PATCH 09/31] Add test --- .../query/service/prometheus/PromqlQuery.java | 4 + .../QueryRequestToPromqlConverter.java | 27 ++-- .../QueryRequestToPromqlConverterTest.java | 121 +++++++++++++++++- 3 files changed, 138 insertions(+), 14 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java index 36043c6b..c5ad62b7 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java @@ -26,4 +26,8 @@ public PromqlQuery( public List getQueries() { return queries; } + + public long getStepMs() { + return stepMs; + } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index e6cccb38..78cf7674 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -37,10 +37,6 @@ class QueryRequestToPromqlConverter { this.prometheusViewDefinition = prometheusViewDefinition; } - /** - * 1. selection should be equal to group by list 2. number of function selection is equal to - * number of query fired 3. count(*) type of query can't be served - */ PromqlQuery toPromql( ExecutionContext executionContext, QueryRequest request, @@ -55,9 +51,10 @@ PromqlQuery toPromql( .getQueryTimeRange() .orElseThrow(() -> new RuntimeException("Time Range missing in query")); + // iterate over all the functions in the query except for date time function (which is handled separately and not a part of the query string) List queries = allSelections.stream() - .filter(expression -> expression.getValueCase().equals(ValueCase.FUNCTION)) + .filter(expression -> expression.getValueCase().equals(ValueCase.FUNCTION) && !QueryRequestUtil.isDateTimeFunction(expression)) .map( functionExpression -> { String functionName = getFunctionName(functionExpression); @@ -83,6 +80,10 @@ private String getGroupByList(QueryRequest queryRequest) { List groupByList = Lists.newArrayList(); if (queryRequest.getGroupByCount() > 0) { for (Expression expression : queryRequest.getGroupByList()) { + // skip datetime function in group-by + if (QueryRequestUtil.isDateTimeFunction(expression)) { + continue; + } groupByList.add(convertColumnIdentifierExpression2String(expression)); } } @@ -204,28 +205,28 @@ private String convertLiteralToString(LiteralConstant literal) { ret = builder.toString(); break; case STRING: - ret = value.getString(); + ret = "\"" + value.getString() + "\""; break; case LONG: - ret = String.valueOf(value.getLong()); + ret = "\"" + value.getLong() + "\""; break; case INT: - ret = String.valueOf(value.getInt()); + ret = "\"" + value.getInt() + "\""; break; case FLOAT: - ret = String.valueOf(value.getFloat()); + ret = "\"" + value.getFloat() + "\""; break; case DOUBLE: - ret = String.valueOf(value.getDouble()); + ret = "\"" + value.getDouble() + "\""; break; case BYTES: - ret = Hex.encodeHexString(value.getBytes().toByteArray()); + ret = "\"" + Hex.encodeHexString(value.getBytes().toByteArray()) + "\""; break; case BOOL: - ret = String.valueOf(value.getBoolean()); + ret = "\"" + value.getBoolean() + "\""; break; case TIMESTAMP: - ret = String.valueOf(value.getTimestamp()); + ret = "\"" + value.getTimestamp() + "\""; break; case NULL_NUMBER: ret = "0"; diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index 9f2384c3..939c4890 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -3,11 +3,15 @@ import static java.util.Objects.requireNonNull; import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createColumnExpression; import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createFunctionExpression; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createInFilter; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createStringLiteralValueExpression; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createTimeColumnGroupByExpression; import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createTimeFilter; import com.typesafe.config.Config; import com.typesafe.config.ConfigFactory; import java.util.LinkedHashSet; +import java.util.List; import org.hypertrace.core.query.service.ExecutionContext; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Filter; @@ -24,7 +28,7 @@ public class QueryRequestToPromqlConverterTest { private static final String TEST_REQUEST_HANDLER_CONFIG_FILE = "prometheus_request_handler.conf"; @Test - public void testQueryWithGroupByWithMultipleAggregates() { + public void testInstantQueryWithGroupByWithMultipleAggregates() { QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQuery(); Builder builder = QueryRequest.newBuilder(orderByQueryRequest); builder.setLimit(20); @@ -39,6 +43,7 @@ public void testQueryWithGroupByWithMultipleAggregates() { .toPromql( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); + // time filter is removed from the query String query1 = "count by (service_name, api_name) (count_over_time(error_count{}[100ms]))"; String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{}[100ms]))"; @@ -46,6 +51,55 @@ public void testQueryWithGroupByWithMultipleAggregates() { Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); } + @Test + public void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { + QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQueryWithMultipleFilters(); + Builder builder = QueryRequest.newBuilder(orderByQueryRequest); + builder.setLimit(20); + PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); + + QueryRequest queryRequest = builder.build(); + + ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); + executionContext.setTimeFilterColumn("SERVICE.startTime"); + PromqlQuery promqlQuery = + new QueryRequestToPromqlConverter(prometheusViewDefinition) + .toPromql( + executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); + + // time filter is removed from the query + String query1 = "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + + Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); + Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); + } + + @Test + public void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { + QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime(); + Builder builder = QueryRequest.newBuilder(orderByQueryRequest); + builder.setLimit(20); + PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); + + QueryRequest queryRequest = builder.build(); + + ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); + executionContext.setTimeFilterColumn("SERVICE.startTime"); + PromqlQuery promqlQuery = + new QueryRequestToPromqlConverter(prometheusViewDefinition) + .toPromql( + executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); + + // time filter is removed from the query + String query1 = "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + + Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); + Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); + Assertions.assertEquals(15000, promqlQuery.getStepMs()); + } + private QueryRequest buildMultipleGroupByMultipleAggQuery() { Builder builder = QueryRequest.newBuilder(); builder.addAggregation( @@ -70,6 +124,71 @@ private QueryRequest buildMultipleGroupByMultipleAggQuery() { return builder.build(); } + private QueryRequest buildMultipleGroupByMultipleAggQueryWithMultipleFilters() { + Builder builder = QueryRequest.newBuilder(); + builder.addAggregation( + createFunctionExpression("Count", createColumnExpression("SERVICE.errorCount").build())); + Expression avg = + createFunctionExpression("AVG", createColumnExpression("SERVICE.numCalls").build()); + builder.addAggregation(avg); + + Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100L); + Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200L); + Filter inFilter = createInFilter("SERVICE.id", List.of("1", "2", "3")); + Filter likeFilter = + Filter.newBuilder() + .setOperator(Operator.LIKE) + .setLhs(createColumnExpression("SERVICE.name").build()) + .setRhs(createStringLiteralValueExpression("someregex")) + .build(); + Filter andFilter = + Filter.newBuilder() + .setOperator(Operator.AND) + .addChildFilter(startTimeFilter) + .addChildFilter(endTimeFilter) + .addChildFilter(inFilter) + .addChildFilter(likeFilter) + .build(); + builder.setFilter(andFilter); + + builder.addGroupBy(createColumnExpression("SERVICE.name")); + builder.addGroupBy(createColumnExpression("API.name")); + return builder.build(); + } + + private QueryRequest buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime() { + Builder builder = QueryRequest.newBuilder(); + builder.addAggregation( + createFunctionExpression("Count", createColumnExpression("SERVICE.errorCount").build())); + Expression avg = + createFunctionExpression("AVG", createColumnExpression("SERVICE.numCalls").build()); + builder.addAggregation(avg); + + Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100L); + Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200L); + Filter inFilter = createInFilter("SERVICE.id", List.of("1", "2", "3")); + Filter likeFilter = + Filter.newBuilder() + .setOperator(Operator.LIKE) + .setLhs(createColumnExpression("SERVICE.name").build()) + .setRhs(createStringLiteralValueExpression("someregex")) + .build(); + Filter andFilter = + Filter.newBuilder() + .setOperator(Operator.AND) + .addChildFilter(startTimeFilter) + .addChildFilter(endTimeFilter) + .addChildFilter(inFilter) + .addChildFilter(likeFilter) + .build(); + builder.setFilter(andFilter); + + builder.addGroupBy(createColumnExpression("SERVICE.name")); + builder.addGroupBy(createColumnExpression("API.name")); + builder.addGroupBy(createTimeColumnGroupByExpression("SERVICE.startTime", "15:SECONDS")); + return builder.build(); + } + private PrometheusViewDefinition getDefaultPrometheusViewDefinition() { Config fileConfig = ConfigFactory.parseURL( From d3d5a76b6f8b6026b603f135ab5df2da97df89e9 Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 13:02:10 +0530 Subject: [PATCH 10/31] fix test --- .../QueryRequestToPromqlConverter.java | 8 ++- .../service/pinot/ExecutionContextTest.java | 5 +- .../pinot/PinotBasedRequestHandlerTest.java | 48 ++++++++++++++++-- .../QueryRequestToPromqlConverterTest.java | 15 ++++-- .../src/test/resources/application.conf | 49 ++++++++++++------- 5 files changed, 96 insertions(+), 29 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 78cf7674..13a8b978 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -51,10 +51,14 @@ PromqlQuery toPromql( .getQueryTimeRange() .orElseThrow(() -> new RuntimeException("Time Range missing in query")); - // iterate over all the functions in the query except for date time function (which is handled separately and not a part of the query string) + // iterate over all the functions in the query except for date time function (which is handled + // separately and not a part of the query string) List queries = allSelections.stream() - .filter(expression -> expression.getValueCase().equals(ValueCase.FUNCTION) && !QueryRequestUtil.isDateTimeFunction(expression)) + .filter( + expression -> + expression.getValueCase().equals(ValueCase.FUNCTION) + && !QueryRequestUtil.isDateTimeFunction(expression)) .map( functionExpression -> { String functionName = getFunctionName(functionExpression); diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java index 51cc1880..6a03fe3f 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/ExecutionContextTest.java @@ -15,6 +15,7 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.QueryTimeRange; import org.hypertrace.core.query.service.api.ColumnIdentifier; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Filter; @@ -397,7 +398,9 @@ public void testEmptyGetTimeRangeDuration() { public void testGetTimeRangeDuration(QueryRequest queryRequest) { ExecutionContext context = new ExecutionContext("test", queryRequest); context.setTimeFilterColumn("SERVICE.startTime"); - assertEquals(Optional.of(Duration.ofMinutes(60)), context.getQueryTimeRange()); + assertEquals( + Optional.of(Duration.ofMinutes(60)), + context.getQueryTimeRange().map(QueryTimeRange::getDuration)); } private static Stream provideQueryRequest() { diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java index e42a1f90..f6ff500a 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java @@ -75,6 +75,10 @@ public void setUp() { @Test public void testInit() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); } @@ -95,6 +99,10 @@ public void testInitFailure() { @Test public void testCanHandle() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -123,6 +131,10 @@ public void testCanHandle() { @Test public void testCanHandleNegativeCase() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -147,6 +159,10 @@ public void testCanHandleNegativeCase() { @Test public void testCanHandleWithInViewFilter() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -244,6 +260,10 @@ public void testCanHandleWithInViewFilter() { @Test public void testCanHandleWithEqViewFilter() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -447,6 +467,10 @@ public void testCanHandleWithEqViewFilter() { @Test public void testCanHandleWithMultipleViewFilters() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -702,6 +726,10 @@ public void testCanHandleWithMultipleViewFilters() { @Test public void testCanHandleWithMultipleViewFiltersAndRepeatedQueryFilters() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -896,6 +924,10 @@ public void testCanHandleWithMultipleViewFiltersAndRepeatedQueryFilters() { @Test void testCanHandle_costShouldBeLessThanHalf() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -928,6 +960,9 @@ void testCanHandle_costShouldBeLessThanHalf() { @Test void testCanHandle_costShouldBeMoreThanHalf() { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } PinotBasedRequestHandler handler = new PinotBasedRequestHandler( config.getString("name"), config.getConfig("requestHandlerInfo")); @@ -1166,7 +1201,7 @@ public void testNullTenantIdQueryRequestContextThrowsNPE() { @Test public void testWithMockPinotClient() throws IOException { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { - if (!config.getString("name").equals("trace-view-handler")) { + if (!isPinotConfig(config)) { continue; } @@ -1211,14 +1246,15 @@ public boolean isResultTableResultSetType(ResultSet resultSet) { QueryRequestBuilderUtils.createColumnExpression("Trace.duration_millis")) .build(); ExecutionContext context = new ExecutionContext("__default", request); - verifyResponseRows(handler.handleRequest(request, context), resultTable); + Observable rows = handler.handleRequest(request, context); + verifyResponseRows(rows, resultTable); } } @Test public void testViewColumnFilterRemoval() throws IOException { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { - if (!config.getString("name").equals("span-event-view-handler")) { + if (!isPinotConfig(config)) { continue; } @@ -1298,7 +1334,7 @@ public boolean isResultTableResultSetType(ResultSet resultSet) { @Test public void testViewColumnFilterRemovalComplexCase() throws IOException { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { - if (!config.getString("name").equals("error-entry-span-view-handler")) { + if (!isPinotConfig(config)) { continue; } @@ -1458,4 +1494,8 @@ private void verifyResponseRows(Observable rowObservable, String[][] expect private String stringify(Object obj) throws JsonProcessingException { return objectMapper.writeValueAsString(obj); } + + private boolean isPinotConfig(Config config) { + return config.getString("type").equals("pinot"); + } } diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index 939c4890..fd6ebd8a 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -68,8 +68,10 @@ public void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); // time filter is removed from the query - String query1 = "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; - String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + String query1 = + "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + String query2 = + "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); @@ -77,7 +79,8 @@ public void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters @Test public void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { - QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime(); + QueryRequest orderByQueryRequest = + buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime(); Builder builder = QueryRequest.newBuilder(orderByQueryRequest); builder.setLimit(20); PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); @@ -92,8 +95,10 @@ public void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilt executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); // time filter is removed from the query - String query1 = "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; - String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + String query1 = + "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + String query2 = + "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); diff --git a/query-service-impl/src/test/resources/application.conf b/query-service-impl/src/test/resources/application.conf index c149dc75..8ccd448d 100644 --- a/query-service-impl/src/test/resources/application.conf +++ b/query-service-impl/src/test/resources/application.conf @@ -164,27 +164,42 @@ service.config = { name = raw-service-prometheus-handler type = prometheus clientConfig = "" - requestHandlerInfo = { + requestHandlerInfo { tenantAttributeName = tenant_id - metricMap = { - "API.numCalls" = { - "metricName": "num_calls", - "metricType": "gauge" + prometheusViewDefinition { + viewName = rawServiceView + metricMap { + API { + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + } + } + SERVICE { + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + }, + errorCount { + metricName: "error_count", + metricType: "GAUGE" + } + } } - "SERVICE.numCalls" = { - "metricName": "num_calls", - "metricType": "gauge" + fieldMap { + "SERVICE.id": "service_id", + "SERVICE.name": "service_name", + "API.id": "api_id", + "API.name": "api_name", + "EVENT.protocolName": "protocol_name", + "EVENT.status_code": "status_code", + "API_TRACE.status_code": "status_code", + "API.startTime": "start_time_millis", + "API.endTime": "end_time_millis", + "SERVICE.startTime": "start_time_millis", + "SERVICE.endTime": "end_time_millis", } } - fieldMap = { - "SERVICE.id": "service_id", - "SERVICE.name": "service_name", - "API.id": "api_id", - "API.name": "api_name", - "EVENT.protocolName": "protocol_name", - "EVENT.status_code": "status_code", - "API_TRACE.status_code": "status_code" - } } } ] From ff36a9ed73c55bdf48c4761b3eceafbc257bb99e Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 13:09:17 +0530 Subject: [PATCH 11/31] minor change --- .../core/query/service/pinot/PinotBasedRequestHandlerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java index f6ff500a..2d8d858c 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java @@ -1201,7 +1201,7 @@ public void testNullTenantIdQueryRequestContextThrowsNPE() { @Test public void testWithMockPinotClient() throws IOException { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { - if (!isPinotConfig(config)) { + if (!config.getString("name").equals("trace-view-handler")) { continue; } From a3895ec4770c343174e679d953df0628c74cbdff Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 13:14:23 +0530 Subject: [PATCH 12/31] fixed test --- .../hypertrace/core/query/service/QueryServiceConfigTest.java | 2 +- .../query/service/pinot/PinotBasedRequestHandlerTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java index 721abaa2..4a76d9bf 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java @@ -33,7 +33,7 @@ public void testQueryServiceImplConfigParser() { assertEquals("query-service", appConfig.getString("service.name")); assertEquals(8091, appConfig.getInt("service.admin.port")); assertEquals(8090, appConfig.getInt("service.port")); - assertEquals(4, queryServiceConfig.getQueryRequestHandlersConfigs().size()); + assertEquals(5, queryServiceConfig.getQueryRequestHandlersConfigs().size()); assertEquals(2, queryServiceConfig.getRequestHandlerClientConfigs().size()); RequestHandlerConfig handler0 = queryServiceConfig.getQueryRequestHandlersConfigs().get(0); diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java index 2d8d858c..6ddbfc04 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java @@ -1254,7 +1254,7 @@ public boolean isResultTableResultSetType(ResultSet resultSet) { @Test public void testViewColumnFilterRemoval() throws IOException { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { - if (!isPinotConfig(config)) { + if (!config.getString("name").equals("span-event-view-handler")) { continue; } @@ -1334,7 +1334,7 @@ public boolean isResultTableResultSetType(ResultSet resultSet) { @Test public void testViewColumnFilterRemovalComplexCase() throws IOException { for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { - if (!isPinotConfig(config)) { + if (!config.getString("name").equals("error-entry-span-view-handler")) { continue; } From 05762148aa3835a424fd83e35324c5162fbe1aa1 Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 15:31:01 +0530 Subject: [PATCH 13/31] Update --- .../core/query/service/QueryRequestUtil.java | 12 +++++++ .../QueryRequestToPinotSQLConverter.java | 6 +--- .../QueryRequestEligibilityValidator.java | 33 +++++++++++++------ .../QueryRequestToPromqlConverter.java | 30 +++++++++-------- 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java index cbbc8340..6c7d5956 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java @@ -1,5 +1,7 @@ package org.hypertrace.core.query.service; +import static org.hypertrace.core.query.service.api.Expression.ValueCase.ATTRIBUTE_EXPRESSION; + import org.hypertrace.core.query.service.api.ColumnIdentifier; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Expression.ValueCase; @@ -78,4 +80,14 @@ public static boolean isDateTimeFunction(Expression expression) { public static boolean isTimeColumn(String columnName) { return columnName.contains("startTime") || columnName.contains("endTime"); } + + public static boolean isComplexAttribute(Expression expression) { + return expression.getValueCase().equals(ATTRIBUTE_EXPRESSION) + && expression.getAttributeExpression().hasSubpath(); + } + + public static boolean isSimpleColumnExpression(Expression expression) { + return expression.getValueCase() == ValueCase.COLUMNIDENTIFIER + || (expression.getValueCase() == ATTRIBUTE_EXPRESSION && !isComplexAttribute(expression)); + } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java index 146c8336..1180f6d0 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java @@ -1,5 +1,6 @@ package org.hypertrace.core.query.service.pinot; +import static org.hypertrace.core.query.service.QueryRequestUtil.isComplexAttribute; import static org.hypertrace.core.query.service.api.Expression.ValueCase.ATTRIBUTE_EXPRESSION; import static org.hypertrace.core.query.service.api.Expression.ValueCase.COLUMNIDENTIFIER; import static org.hypertrace.core.query.service.api.Expression.ValueCase.LITERAL; @@ -365,11 +366,6 @@ private LiteralConstant[] convertExpressionToMapLiterals(Expression expression) return literals; } - private boolean isComplexAttribute(Expression expression) { - return expression.getValueCase().equals(ATTRIBUTE_EXPRESSION) - && expression.getAttributeExpression().hasSubpath(); - } - /** TODO:Handle all types */ private String convertLiteralToString(LiteralConstant literal, Params.Builder paramsBuilder) { Value value = literal.getValue(); diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 26e24830..0d4247b8 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -68,10 +68,10 @@ private boolean selectionAndGroupByOnSameColumn( List selectionList, List groupByList) { Set selections = Sets.newHashSet(); for (Expression expression : selectionList) { - if (expression.getValueCase() != ValueCase.COLUMNIDENTIFIER) { + if (!QueryRequestUtil.isSimpleColumnExpression(expression)) { return false; } - selections.add(expression.getColumnIdentifier().getColumnName()); + selections.add(getLogicalColumnName(expression)); } for (Expression expression : groupByList) { @@ -79,16 +79,26 @@ private boolean selectionAndGroupByOnSameColumn( if (QueryRequestUtil.isDateTimeFunction(expression)) { continue; } - if (expression.getValueCase() != ValueCase.COLUMNIDENTIFIER) { + if (!QueryRequestUtil.isSimpleColumnExpression(expression)) { return false; } - if (!selections.remove(expression.getColumnIdentifier().getColumnName())) { + if (!selections.remove(getLogicalColumnName(expression))) { return false; } } return selections.isEmpty(); } + private String getLogicalColumnName(Expression expression) { + String logicalColumnName; + if (expression.getValueCase() == ValueCase.COLUMNIDENTIFIER) { + logicalColumnName = expression.getColumnIdentifier().getColumnName(); + } else { + logicalColumnName = expression.getAttributeExpression().getAttributeId(); + } + return logicalColumnName; + } + private boolean analyseAggregationColumns(List aggregationList) { for (Expression expression : aggregationList) { Function function = expression.getFunction(); @@ -99,12 +109,15 @@ private boolean analyseAggregationColumns(List aggregationList) { if (function.getArgumentsCount() > 1) { return false; } - if (function.getArgumentsList().get(0).getValueCase() != ValueCase.COLUMNIDENTIFIER) { + Expression functionExpression = function.getArgumentsList().get(0); + if (!QueryRequestUtil.isSimpleColumnExpression(functionExpression)) { return false; } - if (prometheusViewDefinition.getMetricConfig( - function.getArgumentsList().get(0).getColumnIdentifier().getColumnName()) - == null) { + String attributeName = + (functionExpression.getValueCase() == ValueCase.COLUMNIDENTIFIER) + ? functionExpression.getColumnIdentifier().getColumnName() + : functionExpression.getAttributeExpression().getAttributeId(); + if (prometheusViewDefinition.getMetricConfig(attributeName) == null) { return false; } // todo check if the function is supported or not @@ -130,8 +143,8 @@ private boolean analyseFilter(Filter filter) { return false; } - // filter lhs should be column - if (filter.getLhs().getValueCase() != ValueCase.COLUMNIDENTIFIER) { + // filter lhs should be column or simple attribute + if (!QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())) { return false; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 13a8b978..89438c73 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -88,7 +88,7 @@ private String getGroupByList(QueryRequest queryRequest) { if (QueryRequestUtil.isDateTimeFunction(expression)) { continue; } - groupByList.add(convertColumnIdentifierExpression2String(expression)); + groupByList.add(convertColumnAttribute2String(expression)); } } return String.join(", ", groupByList); @@ -133,17 +133,21 @@ private String getFunctionName(Expression functionSelection) { private MetricConfig getMetricConfigForFunction(Expression functionSelection) { return prometheusViewDefinition.getMetricConfig( - functionSelection - .getFunction() - .getArgumentsList() - .get(0) - .getColumnIdentifier() - .getColumnName()); + getLogicalColumnName(functionSelection.getFunction().getArgumentsList().get(0))); } - private String convertColumnIdentifierExpression2String(Expression expression) { - String logicalColumnName = expression.getColumnIdentifier().getColumnName(); - return prometheusViewDefinition.getPhysicalColumnName(logicalColumnName); + private String convertColumnAttribute2String(Expression expression) { + return prometheusViewDefinition.getPhysicalColumnName(getLogicalColumnName(expression)); + } + + private String getLogicalColumnName(Expression expression) { + String logicalColumnName; + if (expression.getValueCase() == ValueCase.COLUMNIDENTIFIER) { + logicalColumnName = expression.getColumnIdentifier().getColumnName(); + } else { + logicalColumnName = expression.getAttributeExpression().getAttributeId(); + } + return logicalColumnName; } /** @@ -157,12 +161,12 @@ private void convertFilter2String(Filter filter, List filterList) { convertFilter2String(childFilter, filterList); } } else { - if (filter.getLhs().getValueCase() == ValueCase.COLUMNIDENTIFIER - && QueryRequestUtil.isTimeColumn(filter.getLhs().getColumnIdentifier().getColumnName())) { + if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs()) + && QueryRequestUtil.isTimeColumn(getLogicalColumnName(filter.getLhs()))) { return; } StringBuilder builder = new StringBuilder(); - builder.append(convertColumnIdentifierExpression2String(filter.getLhs())); + builder.append(convertColumnAttribute2String(filter.getLhs())); switch (filter.getOperator()) { case IN: case EQ: From 90d95999dcf9cf853af9d3054f802077c78a5bee Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 15:37:23 +0530 Subject: [PATCH 14/31] Minor change --- query-service-impl/config.yml | 2 -- .../query/service/prometheus/PrometheusBasedRequestHandler.java | 2 -- 2 files changed, 4 deletions(-) diff --git a/query-service-impl/config.yml b/query-service-impl/config.yml index 9257d0d3..0d047ff4 100644 --- a/query-service-impl/config.yml +++ b/query-service-impl/config.yml @@ -1,5 +1,3 @@ - - logging: level: INFO loggers: diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index 8547000f..79482527 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -15,8 +15,6 @@ public class PrometheusBasedRequestHandler implements RequestHandler { - private static final Logger LOG = LoggerFactory.getLogger(PinotBasedRequestHandler.class); - public static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition"; private static final String TENANT_COLUMN_NAME_CONFIG_KEY = "tenantAttributeName"; private static final String START_TIME_ATTRIBUTE_NAME_CONFIG_KEY = "startTimeAttributeName"; From 71978509c38a1670ab6f0633fbea74b9d3bd7e66 Mon Sep 17 00:00:00 2001 From: rish691 Date: Fri, 3 Dec 2021 15:41:42 +0530 Subject: [PATCH 15/31] spotless --- .../service/prometheus/PrometheusBasedRequestHandler.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index 79482527..7c91d075 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -9,9 +9,6 @@ import org.hypertrace.core.query.service.RequestHandler; import org.hypertrace.core.query.service.api.QueryRequest; import org.hypertrace.core.query.service.api.Row; -import org.hypertrace.core.query.service.pinot.PinotBasedRequestHandler; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class PrometheusBasedRequestHandler implements RequestHandler { From a78bdd7c939002e1502d47579449d91fe7943533 Mon Sep 17 00:00:00 2001 From: rish691 Date: Mon, 6 Dec 2021 22:23:52 +0530 Subject: [PATCH 16/31] Changes in request handler conf --- .../prometheus/PrometheusViewDefinition.java | 17 +++-- .../query/service/QueryServiceConfigTest.java | 2 +- .../src/test/resources/application.conf | 64 ++++++++++++------- .../resources/prometheus_request_handler.conf | 27 +++----- 4 files changed, 62 insertions(+), 48 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 0d508c16..3a4c2797 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -14,10 +14,11 @@ public class PrometheusViewDefinition { private static final String VIEW_NAME_CONFIG_KEY = "viewName"; - private static final String FIELD_MAP_CONFIG_KEY = "fieldMap"; + private static final String ATTRIBUTE_MAP_CONFIG_KEY = "attributeMap"; private static final String METRIC_MAP_CONFIG_KEY = "metricMap"; private static final String METRIC_NAME_CONFIG_KEY = "metricName"; private static final String METRIC_TYPE_CONFIG_KEY = "metricType"; + private static final String METRIC_SCOPE_CONFIG_KEY = "metricScope"; private final String viewName; private final String tenantColumnName; @@ -39,7 +40,7 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN String viewName = config.getString(VIEW_NAME_CONFIG_KEY); final Map fieldMap = Maps.newHashMap(); - Config fieldMapConfig = config.getConfig(FIELD_MAP_CONFIG_KEY); + Config fieldMapConfig = config.getConfig(ATTRIBUTE_MAP_CONFIG_KEY); for (Entry element : fieldMapConfig.entrySet()) { List keys = ConfigUtil.splitPath(element.getKey()); fieldMap.put(keys.get(0), fieldMapConfig.getString(element.getKey())); @@ -47,16 +48,18 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN final Map metricMap = Maps.newHashMap(); Config metricMapConfig = config.getConfig(METRIC_MAP_CONFIG_KEY); - Set metricLogicalNames = Sets.newHashSet(); + + Set metricNames = Sets.newHashSet(); for (Entry element : metricMapConfig.entrySet()) { List keys = ConfigUtil.splitPath(element.getKey()); - metricLogicalNames.add(keys.get(0) + "." + keys.get(1)); + metricNames.add(keys.get(0)); } - for (String metricLogicalName : metricLogicalNames) { - Config metricDef = metricMapConfig.getConfig(metricLogicalName); + String metricScope = config.getString(METRIC_SCOPE_CONFIG_KEY); + for (String metricName : metricNames) { + Config metricDef = metricMapConfig.getConfig(metricName); metricMap.put( - metricLogicalName, + metricScope + "." + metricName, new MetricConfig( metricDef.getString(METRIC_NAME_CONFIG_KEY), MetricType.valueOf(metricDef.getString(METRIC_TYPE_CONFIG_KEY)))); diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java index 4a76d9bf..a067c2ca 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java @@ -33,7 +33,7 @@ public void testQueryServiceImplConfigParser() { assertEquals("query-service", appConfig.getString("service.name")); assertEquals(8091, appConfig.getInt("service.admin.port")); assertEquals(8090, appConfig.getInt("service.port")); - assertEquals(5, queryServiceConfig.getQueryRequestHandlersConfigs().size()); + assertEquals(6, queryServiceConfig.getQueryRequestHandlersConfigs().size()); assertEquals(2, queryServiceConfig.getRequestHandlerClientConfigs().size()); RequestHandlerConfig handler0 = queryServiceConfig.getQueryRequestHandlersConfigs().get(0); diff --git a/query-service-impl/src/test/resources/application.conf b/query-service-impl/src/test/resources/application.conf index 8ccd448d..76e50ef2 100644 --- a/query-service-impl/src/test/resources/application.conf +++ b/query-service-impl/src/test/resources/application.conf @@ -161,46 +161,64 @@ service.config = { } } { - name = raw-service-prometheus-handler + name = raw-service-view-service-scope-prometheus-handler type = prometheus clientConfig = "" requestHandlerInfo { tenantAttributeName = tenant_id prometheusViewDefinition { viewName = rawServiceView + metricScope = SERVICE metricMap { - API { - numCalls { - metricName: "num_calls", - metricType: "GAUGE" - } - } - SERVICE { - numCalls { - metricName: "num_calls", - metricType: "GAUGE" - }, - errorCount { - metricName: "error_count", - metricType: "GAUGE" - } + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + }, + errorCount { + metricName: "error_count", + metricType: "GAUGE" } } - fieldMap { + attributeMap { "SERVICE.id": "service_id", "SERVICE.name": "service_name", "API.id": "api_id", "API.name": "api_name", - "EVENT.protocolName": "protocol_name", - "EVENT.status_code": "status_code", - "API_TRACE.status_code": "status_code", "API.startTime": "start_time_millis", - "API.endTime": "end_time_millis", + "API.endTime": "end_time_millis" + } + } + } + } + { + name = raw-service-view-api-scope-prometheus-handler + type = prometheus + clientConfig = "" + requestHandlerInfo { + tenantAttributeName = tenant_id + prometheusViewDefinition { + viewName = rawServiceView + metricScope = API + metricMap { + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + }, + errorCount { + metricName: "error_count", + metricType: "GAUGE" + } + } + attributeMap { + "SERVICE.id": "service_id", + "SERVICE.name": "service_name", + "API.id": "api_id", + "API.name": "api_name", "SERVICE.startTime": "start_time_millis", - "SERVICE.endTime": "end_time_millis", + "SERVICE.endTime": "end_time_millis" } } } } ] -} +} \ No newline at end of file diff --git a/query-service-impl/src/test/resources/prometheus_request_handler.conf b/query-service-impl/src/test/resources/prometheus_request_handler.conf index 56c22e38..c29b492d 100644 --- a/query-service-impl/src/test/resources/prometheus_request_handler.conf +++ b/query-service-impl/src/test/resources/prometheus_request_handler.conf @@ -1,30 +1,23 @@ { - name = raw-service-prometheus-handler + name = raw-service-view-service-prometheus-handler type = prometheus clientConfig = "" requestHandlerInfo { tenantAttributeName = tenant_id prometheusViewDefinition { viewName = rawServiceView + metricScope = SERVICE metricMap { - API { - numCalls { - metricName: "num_calls", - metricType: "GAUGE" - } - } - SERVICE { - numCalls { - metricName: "num_calls", - metricType: "GAUGE" - }, - errorCount { - metricName: "error_count", - metricType: "GAUGE" - } + numCalls { + metricName: "num_calls", + metricType: "GAUGE" + }, + errorCount { + metricName: "error_count", + metricType: "GAUGE" } } - fieldMap { + attributeMap { "SERVICE.id": "service_id", "SERVICE.name": "service_name", "API.id": "api_id", From 0e577129f0681a1c8b83ebc24326f2e38306e94c Mon Sep 17 00:00:00 2001 From: rish691 Date: Tue, 7 Dec 2021 15:39:46 +0530 Subject: [PATCH 17/31] Address review comment --- query-service-impl/build.gradle.kts | 2 + .../core/query/service/ExecutionContext.java | 7 +- .../core/query/service/QueryRequestUtil.java | 11 ++ .../core/query/service/QueryTimeRange.java | 28 +--- .../prometheus/PromQLInstantQuery.java | 18 ++ .../service/prometheus/PromQLRangeQuery.java | 23 +++ .../PrometheusBasedRequestHandler.java | 4 +- .../prometheus/PrometheusViewDefinition.java | 8 + .../query/service/prometheus/PromqlQuery.java | 33 ---- .../QueryRequestEligibilityValidator.java | 87 +++++----- .../QueryRequestToPromqlConverter.java | 156 ++++++++++-------- .../QueryRequestToPromqlConverterTest.java | 14 +- 12 files changed, 211 insertions(+), 180 deletions(-) create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java delete mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java diff --git a/query-service-impl/build.gradle.kts b/query-service-impl/build.gradle.kts index 0de7eaee..75e6852a 100644 --- a/query-service-impl/build.gradle.kts +++ b/query-service-impl/build.gradle.kts @@ -46,6 +46,8 @@ dependencies { implementation("com.google.protobuf:protobuf-java-util:3.15.6") implementation("com.google.guava:guava:30.1.1-jre") implementation("io.reactivex.rxjava3:rxjava:3.0.11") + annotationProcessor("org.projectlombok:lombok:1.18.20") + compileOnly("org.projectlombok:lombok:1.18.20") testImplementation(project(":query-service-api")) testImplementation("org.junit.jupiter:junit-jupiter:5.7.1") diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java index 264e8996..0b07818a 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java @@ -3,6 +3,7 @@ import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import java.time.Duration; +import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Collection; @@ -218,7 +219,11 @@ private Optional buildQueryTimeRange(Filter filter, String timeF .findFirst(); if (timeRangeStart.isPresent() && timeRangeEnd.isPresent()) { - return Optional.of(new QueryTimeRange(timeRangeEnd.get(), timeRangeStart.get())); + return Optional.of( + new QueryTimeRange( + Instant.ofEpochMilli(timeRangeStart.get()), + Instant.ofEpochMilli(timeRangeEnd.get()), + Duration.ofMillis(timeRangeEnd.get() - timeRangeStart.get()))); } return filter.getChildFilterList().stream() diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java index 6c7d5956..f9b034c4 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java @@ -90,4 +90,15 @@ public static boolean isSimpleColumnExpression(Expression expression) { return expression.getValueCase() == ValueCase.COLUMNIDENTIFIER || (expression.getValueCase() == ATTRIBUTE_EXPRESSION && !isComplexAttribute(expression)); } + + public static String getLogicalColumnNameForSimpleColumnExpression(Expression expression) { + if (!isSimpleColumnExpression(expression)) { + throw new RuntimeException("Expecting expression of type COLUMN or ATTRIBUTE"); + } + if (expression.getValueCase() == ValueCase.COLUMNIDENTIFIER) { + return expression.getColumnIdentifier().getColumnName(); + } else { + return expression.getAttributeExpression().getAttributeId(); + } + } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java index 0c7943c8..f4a6fa8b 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java @@ -1,27 +1,15 @@ package org.hypertrace.core.query.service; import java.time.Duration; +import java.time.Instant; +import lombok.AllArgsConstructor; +import lombok.Getter; +@Getter +@AllArgsConstructor public class QueryTimeRange { - private final long startTime; - private final long endTime; - private final Duration duration; - - public QueryTimeRange(long startTime, long endTime) { - this.startTime = startTime; - this.endTime = endTime; - this.duration = Duration.ofMillis(startTime - endTime); - } - - public long getStartTime() { - return startTime; - } - public long getEndTime() { - return endTime; - } - - public Duration getDuration() { - return duration; - } + private final Instant startTime; + private final Instant endTime; + private final Duration duration; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java new file mode 100644 index 00000000..5d363f60 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java @@ -0,0 +1,18 @@ +package org.hypertrace.core.query.service.prometheus; + +import java.time.Instant; +import java.util.List; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Getter; +import lombok.NonNull; +import lombok.Singular; + +@Getter +@Builder +@AllArgsConstructor +class PromQLInstantQuery { + @NonNull @Singular private List queries; + + @NonNull private Instant evalTime; +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java new file mode 100644 index 00000000..a074cff8 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java @@ -0,0 +1,23 @@ +package org.hypertrace.core.query.service.prometheus; + +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Getter; +import lombok.NonNull; +import lombok.Singular; + +@Getter +@Builder +@AllArgsConstructor +class PromQLRangeQuery { + @NonNull @Singular private List queries; + + @NonNull private Instant startTime; + + @NonNull private Instant endTime; + + @NonNull private Duration step; +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index 7c91d075..95c797e5 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -76,9 +76,7 @@ public Observable handleRequest( Preconditions.checkNotNull(executionContext); Preconditions.checkNotNull(executionContext.getTenantId()); - PromqlQuery promqlQuery = - requestToPromqlConverter.toPromql( - executionContext, originalRequest, executionContext.getAllSelections()); + // todo call convert and execute request using client here return null; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 3a4c2797..c49ede3c 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -78,6 +78,14 @@ public MetricConfig getMetricConfig(String logicalMetricName) { return metricMap.get(logicalMetricName); } + public String getViewName() { + return viewName; + } + + public String getTenantColumnName() { + return tenantColumnName; + } + public static class MetricConfig { private final String name; private final MetricType metricType; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java deleted file mode 100644 index c5ad62b7..00000000 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromqlQuery.java +++ /dev/null @@ -1,33 +0,0 @@ -package org.hypertrace.core.query.service.prometheus; - -import java.util.List; - -public class PromqlQuery { - - private final List queries; - private final boolean isInstantRequest; - private final long startTimeMs; - private final long endTimeMs; - private final long stepMs; - - public PromqlQuery( - List queries, - boolean isInstantRequest, - long startTimeMs, - long endTimeMs, - long stepMs) { - this.queries = queries; - this.isInstantRequest = isInstantRequest; - this.startTimeMs = startTimeMs; - this.endTimeMs = endTimeMs; - this.stepMs = stepMs; - } - - public List getQueries() { - return queries; - } - - public long getStepMs() { - return stepMs; - } -} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 0d4247b8..1bfff650 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -1,5 +1,7 @@ package org.hypertrace.core.query.service.prometheus; +import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; + import com.google.common.base.Preconditions; import com.google.common.collect.Sets; import java.util.List; @@ -24,54 +26,57 @@ public QueryRequestEligibilityValidator(PrometheusViewDefinition prometheusViewD } QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContext) { - // orderBy to be supported later - if (queryRequest.getOrderByCount() > 0) { - return QueryCost.UNSUPPORTED; - } - - // only aggregation queries are supported - if (queryRequest.getAggregationCount() == 0 - || queryRequest.getGroupByCount() == 0 - || queryRequest.getDistinctSelections()) { - return QueryCost.UNSUPPORTED; - } + try { + // orderBy to be supported later + if (queryRequest.getOrderByCount() > 0) { + return QueryCost.UNSUPPORTED; + } - Set referencedColumns = executionContext.getReferencedColumns(); - Preconditions.checkArgument(!referencedColumns.isEmpty()); - // all the columns in the request should have a mapping in the config - for (String referencedColumn : referencedColumns) { - if (!QueryRequestUtil.isTimeColumn(referencedColumn) - && prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null - && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { + // only aggregation queries are supported + if (queryRequest.getAggregationCount() == 0 + || queryRequest.getGroupByCount() == 0 + || queryRequest.getDistinctSelections()) { return QueryCost.UNSUPPORTED; } - } - if (!analyseAggregationColumns(queryRequest.getAggregationList())) { - return QueryCost.UNSUPPORTED; - } + Set referencedColumns = executionContext.getReferencedColumns(); + Preconditions.checkArgument(!referencedColumns.isEmpty()); + // all the columns in the request should have a mapping in the config + for (String referencedColumn : referencedColumns) { + if (!QueryRequestUtil.isTimeColumn(referencedColumn) + && prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null + && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { + return QueryCost.UNSUPPORTED; + } + } - if (!selectionAndGroupByOnSameColumn( - queryRequest.getSelectionList(), queryRequest.getGroupByList())) { - return QueryCost.UNSUPPORTED; - } + if (!analyseAggregationColumns(queryRequest.getAggregationList())) { + return QueryCost.UNSUPPORTED; + } + + if (!analyseSelectionAndGroupBy( + queryRequest.getSelectionList(), queryRequest.getGroupByList())) { + return QueryCost.UNSUPPORTED; + } - if (!analyseFilter(queryRequest.getFilter())) { + if (!analyseFilter(queryRequest.getFilter())) { + return QueryCost.UNSUPPORTED; + } + } catch (Exception e) { return QueryCost.UNSUPPORTED; } - // value 1.0 so that prometheus is preferred over others return new QueryCost(1.0); } - private boolean selectionAndGroupByOnSameColumn( + private boolean analyseSelectionAndGroupBy( List selectionList, List groupByList) { Set selections = Sets.newHashSet(); for (Expression expression : selectionList) { if (!QueryRequestUtil.isSimpleColumnExpression(expression)) { return false; } - selections.add(getLogicalColumnName(expression)); + selections.add(getLogicalColumnNameForSimpleColumnExpression(expression)); } for (Expression expression : groupByList) { @@ -82,23 +87,14 @@ private boolean selectionAndGroupByOnSameColumn( if (!QueryRequestUtil.isSimpleColumnExpression(expression)) { return false; } - if (!selections.remove(getLogicalColumnName(expression))) { + if (!selections.remove(getLogicalColumnNameForSimpleColumnExpression(expression))) { return false; } } + // all selection and group by should be on same column return selections.isEmpty(); } - private String getLogicalColumnName(Expression expression) { - String logicalColumnName; - if (expression.getValueCase() == ValueCase.COLUMNIDENTIFIER) { - logicalColumnName = expression.getColumnIdentifier().getColumnName(); - } else { - logicalColumnName = expression.getAttributeExpression().getAttributeId(); - } - return logicalColumnName; - } - private boolean analyseAggregationColumns(List aggregationList) { for (Expression expression : aggregationList) { Function function = expression.getFunction(); @@ -109,14 +105,11 @@ private boolean analyseAggregationColumns(List aggregationList) { if (function.getArgumentsCount() > 1) { return false; } - Expression functionExpression = function.getArgumentsList().get(0); - if (!QueryRequestUtil.isSimpleColumnExpression(functionExpression)) { + Expression functionArgument = function.getArgumentsList().get(0); + if (!QueryRequestUtil.isSimpleColumnExpression(functionArgument)) { return false; } - String attributeName = - (functionExpression.getValueCase() == ValueCase.COLUMNIDENTIFIER) - ? functionExpression.getColumnIdentifier().getColumnName() - : functionExpression.getAttributeExpression().getAttributeId(); + String attributeName = getLogicalColumnNameForSimpleColumnExpression(functionArgument); if (prometheusViewDefinition.getMetricConfig(attributeName) == null) { return false; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 89438c73..023f6d99 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -5,6 +5,7 @@ import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MAX; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; +import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; import com.google.common.collect.Lists; import com.google.protobuf.ByteString; @@ -21,66 +22,88 @@ import org.hypertrace.core.query.service.api.Expression.ValueCase; import org.hypertrace.core.query.service.api.Filter; import org.hypertrace.core.query.service.api.LiteralConstant; +import org.hypertrace.core.query.service.api.Operator; import org.hypertrace.core.query.service.api.QueryRequest; import org.hypertrace.core.query.service.api.Value; import org.hypertrace.core.query.service.prometheus.PrometheusViewDefinition.MetricConfig; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; class QueryRequestToPromqlConverter { - private static final Logger LOG = LoggerFactory.getLogger(QueryRequestToPromqlConverter.class); - private final PrometheusViewDefinition prometheusViewDefinition; QueryRequestToPromqlConverter(PrometheusViewDefinition prometheusViewDefinition) { this.prometheusViewDefinition = prometheusViewDefinition; } - PromqlQuery toPromql( + PromQLInstantQuery convertToPromqlInstantQuery( ExecutionContext executionContext, QueryRequest request, LinkedHashSet allSelections) { - String groupByList = getGroupByList(request); - List filterList = new ArrayList<>(); - convertFilter2String(request.getFilter(), filterList); + QueryTimeRange queryTimeRange = + executionContext + .getQueryTimeRange() + .orElseThrow(() -> new RuntimeException("Time Range missing in query")); + + return new PromQLInstantQuery( + buildPromqlQueries(request, allSelections, queryTimeRange), queryTimeRange.getEndTime()); + } + + PromQLRangeQuery convertToPromqlRangeQuery( + ExecutionContext executionContext, + QueryRequest request, + LinkedHashSet allSelections) { QueryTimeRange queryTimeRange = executionContext .getQueryTimeRange() .orElseThrow(() -> new RuntimeException("Time Range missing in query")); - // iterate over all the functions in the query except for date time function (which is handled - // separately and not a part of the query string) - List queries = - allSelections.stream() - .filter( - expression -> - expression.getValueCase().equals(ValueCase.FUNCTION) - && !QueryRequestUtil.isDateTimeFunction(expression)) - .map( - functionExpression -> { - String functionName = getFunctionName(functionExpression); - MetricConfig metricConfig = getMetricConfigForFunction(functionExpression); - return buildQuery( - metricConfig.getName(), - functionName, - groupByList, - String.join(", ", filterList), - queryTimeRange.getDuration().toMillis()); - }) - .collect(Collectors.toUnmodifiableList()); - - return new PromqlQuery( - queries, - executionContext.getTimeSeriesPeriod().isEmpty(), + return new PromQLRangeQuery( + buildPromqlQueries(request, allSelections, queryTimeRange), queryTimeRange.getStartTime(), queryTimeRange.getEndTime(), - getStep(executionContext)); + getTimeSeriesPeriod(executionContext)); } - private String getGroupByList(QueryRequest queryRequest) { + private List buildPromqlQueries( + QueryRequest request, + LinkedHashSet allSelections, + QueryTimeRange queryTimeRange) { + List groupByList = getGroupByList(request); + + List filterList = new ArrayList<>(); + convertFilterToString(request.getFilter(), filterList); + + // iterate over all the functions in the query except for date time function (which is handled + // separately and not a part of the query string) + return allSelections.stream() + .filter( + expression -> + expression.getValueCase().equals(ValueCase.FUNCTION) + && !QueryRequestUtil.isDateTimeFunction(expression)) + .map( + functionExpression -> + mapToPromqlQuery(functionExpression, groupByList, filterList, queryTimeRange)) + .collect(Collectors.toUnmodifiableList()); + } + + private String mapToPromqlQuery( + Expression functionExpression, + List groupByList, + List filterList, + QueryTimeRange queryTimeRange) { + String functionName = getFunctionName(functionExpression); + MetricConfig metricConfig = getMetricConfigForFunction(functionExpression); + return buildQuery( + metricConfig.getName(), + functionName, + String.join(", ", groupByList), + String.join(", ", filterList), + queryTimeRange.getDuration().toMillis()); + } + + private List getGroupByList(QueryRequest queryRequest) { List groupByList = Lists.newArrayList(); if (queryRequest.getGroupByCount() > 0) { for (Expression expression : queryRequest.getGroupByList()) { @@ -88,14 +111,14 @@ private String getGroupByList(QueryRequest queryRequest) { if (QueryRequestUtil.isDateTimeFunction(expression)) { continue; } - groupByList.add(convertColumnAttribute2String(expression)); + groupByList.add(convertColumnAttributeToString(expression)); } } - return String.join(", ", groupByList); + return groupByList; } - private long getStep(ExecutionContext executionContext) { - return executionContext.getTimeSeriesPeriod().map(Duration::toMillis).orElse(-1L); + private Duration getTimeSeriesPeriod(ExecutionContext executionContext) { + return executionContext.getTimeSeriesPeriod().get(); } private String buildQuery( @@ -133,21 +156,13 @@ private String getFunctionName(Expression functionSelection) { private MetricConfig getMetricConfigForFunction(Expression functionSelection) { return prometheusViewDefinition.getMetricConfig( - getLogicalColumnName(functionSelection.getFunction().getArgumentsList().get(0))); - } - - private String convertColumnAttribute2String(Expression expression) { - return prometheusViewDefinition.getPhysicalColumnName(getLogicalColumnName(expression)); + getLogicalColumnNameForSimpleColumnExpression( + functionSelection.getFunction().getArgumentsList().get(0))); } - private String getLogicalColumnName(Expression expression) { - String logicalColumnName; - if (expression.getValueCase() == ValueCase.COLUMNIDENTIFIER) { - logicalColumnName = expression.getColumnIdentifier().getColumnName(); - } else { - logicalColumnName = expression.getAttributeExpression().getAttributeId(); - } - return logicalColumnName; + private String convertColumnAttributeToString(Expression expression) { + return prometheusViewDefinition.getPhysicalColumnName( + getLogicalColumnNameForSimpleColumnExpression(expression)); } /** @@ -155,37 +170,40 @@ private String getLogicalColumnName(Expression expression) { * *

rhs of leaf filter should be literal */ - private void convertFilter2String(Filter filter, List filterList) { + private void convertFilterToString(Filter filter, List filterList) { if (filter.getChildFilterCount() > 0) { for (Filter childFilter : filter.getChildFilterList()) { - convertFilter2String(childFilter, filterList); + convertFilterToString(childFilter, filterList); } } else { if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs()) - && QueryRequestUtil.isTimeColumn(getLogicalColumnName(filter.getLhs()))) { + && QueryRequestUtil.isTimeColumn( + getLogicalColumnNameForSimpleColumnExpression(filter.getLhs()))) { return; } StringBuilder builder = new StringBuilder(); - builder.append(convertColumnAttribute2String(filter.getLhs())); - switch (filter.getOperator()) { - case IN: - case EQ: - builder.append("="); - break; - case NEQ: - builder.append("!="); - break; - case LIKE: - builder.append("=~"); - break; - default: - // throw exception - } + builder.append(convertColumnAttributeToString(filter.getLhs())); + builder.append(convertOperatorToString(filter.getOperator())); builder.append(convertLiteralToString(filter.getRhs().getLiteral())); filterList.add(builder.toString()); } } + private String convertOperatorToString(Operator operator) { + switch (operator) { + case IN: + case EQ: + return "="; + case NEQ: + return "!="; + case LIKE: + return "=~"; + default: + throw new RuntimeException( + String.format("Equivalent operator %s not supported in promql", operator)); + } + } + private String convertLiteralToString(LiteralConstant literal) { Value value = literal.getValue(); String ret = null; diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index fd6ebd8a..6c7dd140 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -38,9 +38,9 @@ public void testInstantQueryWithGroupByWithMultipleAggregates() { ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromqlQuery promqlQuery = + PromQLInstantQuery promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) - .toPromql( + .convertToPromqlInstantQuery( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); // time filter is removed from the query @@ -62,9 +62,9 @@ public void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromqlQuery promqlQuery = + PromQLInstantQuery promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) - .toPromql( + .convertToPromqlInstantQuery( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); // time filter is removed from the query @@ -89,9 +89,9 @@ public void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilt ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromqlQuery promqlQuery = + PromQLRangeQuery promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) - .toPromql( + .convertToPromqlRangeQuery( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); // time filter is removed from the query @@ -102,7 +102,7 @@ public void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilt Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); - Assertions.assertEquals(15000, promqlQuery.getStepMs()); + Assertions.assertEquals(15000, promqlQuery.getStep().toMillis()); } private QueryRequest buildMultipleGroupByMultipleAggQuery() { From 78464d3bcad2bfb331d4715c6551c70eec20658c Mon Sep 17 00:00:00 2001 From: rish691 Date: Tue, 7 Dec 2021 18:45:48 +0530 Subject: [PATCH 18/31] Add PrometheusFunctionConverter class --- .../PrometheusFunctionConverter.java | 32 +++++++++++++++++++ .../prometheus/PrometheusViewDefinition.java | 2 +- .../QueryRequestToPromqlConverter.java | 27 +++------------- 3 files changed, 37 insertions(+), 24 deletions(-) create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java new file mode 100644 index 00000000..f97a2f9e --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java @@ -0,0 +1,32 @@ +package org.hypertrace.core.query.service.prometheus; + +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_AVG; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_COUNT; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MAX; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; +import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; + +import org.hypertrace.core.query.service.api.Expression; + +class PrometheusFunctionConverter { + + String mapToPrometheusFunctionName(Expression functionSelection) { + String queryFunctionName = functionSelection.getFunction().getFunctionName().toUpperCase(); + switch (queryFunctionName) { + case QUERY_FUNCTION_SUM: + return "sum"; + case QUERY_FUNCTION_MAX: + return "max"; + case QUERY_FUNCTION_MIN: + return "min"; + case QUERY_FUNCTION_AVG: + return "avg"; + case QUERY_FUNCTION_COUNT: + return "count"; + default: + throw new RuntimeException( + String.format( + "Couldn't map query function {} to prometheus function", queryFunctionName)); + } + } +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index c49ede3c..105072a2 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -11,7 +11,7 @@ import java.util.Set; /** Prometheus metric & attribute mapping for a pinot view */ -public class PrometheusViewDefinition { +class PrometheusViewDefinition { private static final String VIEW_NAME_CONFIG_KEY = "viewName"; private static final String ATTRIBUTE_MAP_CONFIG_KEY = "attributeMap"; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 023f6d99..a1880389 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -1,10 +1,5 @@ package org.hypertrace.core.query.service.prometheus; -import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_AVG; -import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_COUNT; -import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MAX; -import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; -import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; import com.google.common.collect.Lists; @@ -30,9 +25,11 @@ class QueryRequestToPromqlConverter { private final PrometheusViewDefinition prometheusViewDefinition; + private final PrometheusFunctionConverter prometheusFunctionConverter; QueryRequestToPromqlConverter(PrometheusViewDefinition prometheusViewDefinition) { this.prometheusViewDefinition = prometheusViewDefinition; + this.prometheusFunctionConverter = new PrometheusFunctionConverter(); } PromQLInstantQuery convertToPromqlInstantQuery( @@ -93,7 +90,8 @@ private String mapToPromqlQuery( List groupByList, List filterList, QueryTimeRange queryTimeRange) { - String functionName = getFunctionName(functionExpression); + String functionName = + prometheusFunctionConverter.mapToPrometheusFunctionName(functionExpression); MetricConfig metricConfig = getMetricConfigForFunction(functionExpression); return buildQuery( metricConfig.getName(), @@ -137,23 +135,6 @@ private String buildQuery( durationMillis); } - private String getFunctionName(Expression functionSelection) { - switch (functionSelection.getFunction().getFunctionName().toUpperCase()) { - case QUERY_FUNCTION_SUM: - return "sum"; - case QUERY_FUNCTION_MAX: - return "max"; - case QUERY_FUNCTION_MIN: - return "min"; - case QUERY_FUNCTION_AVG: - return "avg"; - case QUERY_FUNCTION_COUNT: - return "count"; - default: - throw new RuntimeException(""); - } - } - private MetricConfig getMetricConfigForFunction(Expression functionSelection) { return prometheusViewDefinition.getMetricConfig( getLogicalColumnNameForSimpleColumnExpression( From 8b44ce358dd68af3b00c46a3b33231c37e237f92 Mon Sep 17 00:00:00 2001 From: rish691 Date: Tue, 7 Dec 2021 19:14:35 +0530 Subject: [PATCH 19/31] Check for timeFilter based on config --- .../core/query/service/ExecutionContext.java | 4 ++++ .../core/query/service/QueryRequestUtil.java | 4 ---- .../prometheus/PromQLInstantQuery.java | 2 -- .../service/prometheus/PromQLRangeQuery.java | 2 -- .../prometheus/PrometheusViewDefinition.java | 17 ++++------------- .../QueryRequestEligibilityValidator.java | 3 ++- .../QueryRequestToPromqlConverter.java | 19 ++++++++++++------- .../src/test/resources/application.conf | 10 ++++++---- .../resources/prometheus_request_handler.conf | 1 + 9 files changed, 29 insertions(+), 33 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java index 0b07818a..26559593 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/ExecutionContext.java @@ -280,4 +280,8 @@ public Optional getTimeRangeDuration() { public Optional getQueryTimeRange() { return queryTimeRangeSupplier.get(); } + + public String getTimeFilterColumn() { + return timeFilterColumn; + } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java index f9b034c4..5cb3ba73 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryRequestUtil.java @@ -77,10 +77,6 @@ public static boolean isDateTimeFunction(Expression expression) { && expression.getFunction().getFunctionName().equals("dateTimeConvert"); } - public static boolean isTimeColumn(String columnName) { - return columnName.contains("startTime") || columnName.contains("endTime"); - } - public static boolean isComplexAttribute(Expression expression) { return expression.getValueCase().equals(ATTRIBUTE_EXPRESSION) && expression.getAttributeExpression().hasSubpath(); diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java index 5d363f60..96b0863b 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java @@ -2,7 +2,6 @@ import java.time.Instant; import java.util.List; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Getter; import lombok.NonNull; @@ -10,7 +9,6 @@ @Getter @Builder -@AllArgsConstructor class PromQLInstantQuery { @NonNull @Singular private List queries; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java index a074cff8..b94992e9 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java @@ -3,7 +3,6 @@ import java.time.Duration; import java.time.Instant; import java.util.List; -import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Getter; import lombok.NonNull; @@ -11,7 +10,6 @@ @Getter @Builder -@AllArgsConstructor class PromQLRangeQuery { @NonNull @Singular private List queries; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 105072a2..cd6a22ca 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -9,6 +9,8 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import lombok.AllArgsConstructor; +import lombok.Getter; /** Prometheus metric & attribute mapping for a pinot view */ class PrometheusViewDefinition { @@ -86,22 +88,11 @@ public String getTenantColumnName() { return tenantColumnName; } + @Getter + @AllArgsConstructor public static class MetricConfig { private final String name; private final MetricType metricType; - - public MetricConfig(String name, MetricType metricType) { - this.name = name; - this.metricType = metricType; - } - - public String getName() { - return name; - } - - public MetricType getMetricType() { - return metricType; - } } public enum MetricType { diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 1bfff650..ec4f132a 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -41,9 +41,10 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex Set referencedColumns = executionContext.getReferencedColumns(); Preconditions.checkArgument(!referencedColumns.isEmpty()); + String timeFilterColumn = executionContext.getTimeFilterColumn(); // all the columns in the request should have a mapping in the config for (String referencedColumn : referencedColumns) { - if (!QueryRequestUtil.isTimeColumn(referencedColumn) + if (!timeFilterColumn.equals(referencedColumn) && prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { return QueryCost.UNSUPPORTED; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index a1880389..87631e90 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -43,7 +43,9 @@ PromQLInstantQuery convertToPromqlInstantQuery( .orElseThrow(() -> new RuntimeException("Time Range missing in query")); return new PromQLInstantQuery( - buildPromqlQueries(request, allSelections, queryTimeRange), queryTimeRange.getEndTime()); + buildPromqlQueries( + request, allSelections, queryTimeRange, executionContext.getTimeFilterColumn()), + queryTimeRange.getEndTime()); } PromQLRangeQuery convertToPromqlRangeQuery( @@ -57,7 +59,8 @@ PromQLRangeQuery convertToPromqlRangeQuery( .orElseThrow(() -> new RuntimeException("Time Range missing in query")); return new PromQLRangeQuery( - buildPromqlQueries(request, allSelections, queryTimeRange), + buildPromqlQueries( + request, allSelections, queryTimeRange, executionContext.getTimeFilterColumn()), queryTimeRange.getStartTime(), queryTimeRange.getEndTime(), getTimeSeriesPeriod(executionContext)); @@ -66,11 +69,12 @@ PromQLRangeQuery convertToPromqlRangeQuery( private List buildPromqlQueries( QueryRequest request, LinkedHashSet allSelections, - QueryTimeRange queryTimeRange) { + QueryTimeRange queryTimeRange, + String timeFilterColumn) { List groupByList = getGroupByList(request); List filterList = new ArrayList<>(); - convertFilterToString(request.getFilter(), filterList); + convertFilterToString(request.getFilter(), filterList, timeFilterColumn); // iterate over all the functions in the query except for date time function (which is handled // separately and not a part of the query string) @@ -151,14 +155,15 @@ private String convertColumnAttributeToString(Expression expression) { * *

rhs of leaf filter should be literal */ - private void convertFilterToString(Filter filter, List filterList) { + private void convertFilterToString( + Filter filter, List filterList, String timeFilterColumn) { if (filter.getChildFilterCount() > 0) { for (Filter childFilter : filter.getChildFilterList()) { - convertFilterToString(childFilter, filterList); + convertFilterToString(childFilter, filterList, timeFilterColumn); } } else { if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs()) - && QueryRequestUtil.isTimeColumn( + && timeFilterColumn.equals( getLogicalColumnNameForSimpleColumnExpression(filter.getLhs()))) { return; } diff --git a/query-service-impl/src/test/resources/application.conf b/query-service-impl/src/test/resources/application.conf index 76e50ef2..6e306083 100644 --- a/query-service-impl/src/test/resources/application.conf +++ b/query-service-impl/src/test/resources/application.conf @@ -166,6 +166,7 @@ service.config = { clientConfig = "" requestHandlerInfo { tenantAttributeName = tenant_id + startTimeAttributeName = "SERVICE.startTime" prometheusViewDefinition { viewName = rawServiceView metricScope = SERVICE @@ -184,8 +185,8 @@ service.config = { "SERVICE.name": "service_name", "API.id": "api_id", "API.name": "api_name", - "API.startTime": "start_time_millis", - "API.endTime": "end_time_millis" + "SERVICE.startTime": "start_time_millis", + "SERVICE.endTime": "end_time_millis" } } } @@ -196,6 +197,7 @@ service.config = { clientConfig = "" requestHandlerInfo { tenantAttributeName = tenant_id + startTimeAttributeName = "API.startTime" prometheusViewDefinition { viewName = rawServiceView metricScope = API @@ -214,8 +216,8 @@ service.config = { "SERVICE.name": "service_name", "API.id": "api_id", "API.name": "api_name", - "SERVICE.startTime": "start_time_millis", - "SERVICE.endTime": "end_time_millis" + "API.startTime": "start_time_millis", + "API.endTime": "end_time_millis" } } } diff --git a/query-service-impl/src/test/resources/prometheus_request_handler.conf b/query-service-impl/src/test/resources/prometheus_request_handler.conf index c29b492d..be9c1e2e 100644 --- a/query-service-impl/src/test/resources/prometheus_request_handler.conf +++ b/query-service-impl/src/test/resources/prometheus_request_handler.conf @@ -4,6 +4,7 @@ clientConfig = "" requestHandlerInfo { tenantAttributeName = tenant_id + startTimeAttributeName = "SERVICE.startTime" prometheusViewDefinition { viewName = rawServiceView metricScope = SERVICE From 9184afd782e272b214799b6b50bc8bd585aba4a0 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 18:15:52 +0530 Subject: [PATCH 20/31] Add test for eligibilityValidator --- .../PrometheusBasedRequestHandler.java | 2 +- .../PrometheusFunctionConverter.java | 11 +- .../QueryRequestEligibilityValidator.java | 108 ++++++++--------- .../QueryRequestEligibilityValidatorTest.java | 113 ++++++++++++++++++ .../QueryRequestToPromqlConverterTest.java | 21 ++-- 5 files changed, 184 insertions(+), 71 deletions(-) create mode 100644 query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidatorTest.java diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index 95c797e5..ee7d337a 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -65,7 +65,7 @@ private void processConfig(Config config) { */ @Override public QueryCost canHandle(QueryRequest request, ExecutionContext executionContext) { - return queryRequestEligibilityValidator.isEligible(request, executionContext); + return queryRequestEligibilityValidator.calculateCost(request, executionContext); } @Override diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java index f97a2f9e..2bf59269 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java @@ -6,10 +6,19 @@ import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_MIN; import static org.hypertrace.core.query.service.QueryFunctionConstants.QUERY_FUNCTION_SUM; +import java.util.List; import org.hypertrace.core.query.service.api.Expression; class PrometheusFunctionConverter { + static final List supportedFunctions = + List.of( + QUERY_FUNCTION_SUM, + QUERY_FUNCTION_MAX, + QUERY_FUNCTION_MIN, + QUERY_FUNCTION_AVG, + QUERY_FUNCTION_COUNT); + String mapToPrometheusFunctionName(Expression functionSelection) { String queryFunctionName = functionSelection.getFunction().getFunctionName().toUpperCase(); switch (queryFunctionName) { @@ -26,7 +35,7 @@ String mapToPrometheusFunctionName(Expression functionSelection) { default: throw new RuntimeException( String.format( - "Couldn't map query function {} to prometheus function", queryFunctionName)); + "Couldn't map query function %s to prometheus function", queryFunctionName)); } } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index ec4f132a..c6b5890d 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -3,9 +3,10 @@ import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; import com.google.common.base.Preconditions; -import com.google.common.collect.Sets; import java.util.List; import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.Collectors; import org.hypertrace.core.query.service.ExecutionContext; import org.hypertrace.core.query.service.QueryCost; import org.hypertrace.core.query.service.QueryRequestUtil; @@ -25,7 +26,7 @@ public QueryRequestEligibilityValidator(PrometheusViewDefinition prometheusViewD this.prometheusViewDefinition = prometheusViewDefinition; } - QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContext) { + QueryCost calculateCost(QueryRequest queryRequest, ExecutionContext executionContext) { try { // orderBy to be supported later if (queryRequest.getOrderByCount() > 0) { @@ -39,28 +40,33 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex return QueryCost.UNSUPPORTED; } + // all selection including group by and aggregations should be either on column or attribute + if (executionContext.getAllSelections().stream() + .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) + .anyMatch(Predicate.not(QueryRequestUtil::isSimpleColumnExpression))) { + return QueryCost.UNSUPPORTED; + } + Set referencedColumns = executionContext.getReferencedColumns(); Preconditions.checkArgument(!referencedColumns.isEmpty()); - String timeFilterColumn = executionContext.getTimeFilterColumn(); // all the columns in the request should have a mapping in the config for (String referencedColumn : referencedColumns) { - if (!timeFilterColumn.equals(referencedColumn) - && prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null + if (prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { return QueryCost.UNSUPPORTED; } } - if (!analyseAggregationColumns(queryRequest.getAggregationList())) { + if (areAggregationsNotSupported(queryRequest.getAggregationList())) { return QueryCost.UNSUPPORTED; } - if (!analyseSelectionAndGroupBy( + if (selectionAndGroupByOnDifferentColumn( queryRequest.getSelectionList(), queryRequest.getGroupByList())) { return QueryCost.UNSUPPORTED; } - if (!analyseFilter(queryRequest.getFilter())) { + if (isFilterNotSupported(queryRequest.getFilter())) { return QueryCost.UNSUPPORTED; } } catch (Exception e) { @@ -70,79 +76,65 @@ QueryCost isEligible(QueryRequest queryRequest, ExecutionContext executionContex return new QueryCost(1.0); } - private boolean analyseSelectionAndGroupBy( + private boolean selectionAndGroupByOnDifferentColumn( List selectionList, List groupByList) { - Set selections = Sets.newHashSet(); - for (Expression expression : selectionList) { - if (!QueryRequestUtil.isSimpleColumnExpression(expression)) { - return false; - } - selections.add(getLogicalColumnNameForSimpleColumnExpression(expression)); - } - for (Expression expression : groupByList) { - // skip datetime convert group by - if (QueryRequestUtil.isDateTimeFunction(expression)) { - continue; - } - if (!QueryRequestUtil.isSimpleColumnExpression(expression)) { - return false; - } - if (!selections.remove(getLogicalColumnNameForSimpleColumnExpression(expression))) { - return false; - } - } + Set selections = + selectionList.stream() + .map(QueryRequestUtil::getLogicalColumnNameForSimpleColumnExpression) + .collect(Collectors.toSet()); + + Set groupBys = + groupByList.stream() + .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) + .map(QueryRequestUtil::getLogicalColumnNameForSimpleColumnExpression) + .collect(Collectors.toSet()); // all selection and group by should be on same column - return selections.isEmpty(); + return !selections.equals(groupBys); } - private boolean analyseAggregationColumns(List aggregationList) { - for (Expression expression : aggregationList) { - Function function = expression.getFunction(); - // skip dateTimeConvert function as it is part of the prometheus api call - if (QueryRequestUtil.isDateTimeFunction(expression)) { - continue; - } - if (function.getArgumentsCount() > 1) { - return false; - } - Expression functionArgument = function.getArgumentsList().get(0); - if (!QueryRequestUtil.isSimpleColumnExpression(functionArgument)) { - return false; - } - String attributeName = getLogicalColumnNameForSimpleColumnExpression(functionArgument); - if (prometheusViewDefinition.getMetricConfig(attributeName) == null) { - return false; - } - // todo check if the function is supported or not - } - - return true; + private boolean areAggregationsNotSupported(List aggregationList) { + return aggregationList.stream() + .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) + .anyMatch( + expression -> { + Function function = expression.getFunction(); + if (function.getArgumentsCount() > 1) { + return true; + } + Expression functionArgument = function.getArgumentsList().get(0); + String attributeId = getLogicalColumnNameForSimpleColumnExpression(functionArgument); + if (!PrometheusFunctionConverter.supportedFunctions.contains( + function.getFunctionName())) { + return true; + } + return prometheusViewDefinition.getMetricConfig(attributeId) == null; + }); } - private boolean analyseFilter(Filter filter) { + private boolean isFilterNotSupported(Filter filter) { if (filter.getChildFilterCount() > 0) { if (filter.getOperator() != Operator.AND) { - return false; + return true; } for (Filter childFilter : filter.getChildFilterList()) { - if (!analyseFilter(childFilter)) { - return false; + if (!isFilterNotSupported(childFilter)) { + return true; } } } // filter rhs should be literal only if (filter.getRhs().getValueCase() != ValueCase.LITERAL) { - return false; + return true; } // filter lhs should be column or simple attribute if (!QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())) { - return false; + return true; } // todo check for valid operators here - return true; + return false; } } diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidatorTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidatorTest.java new file mode 100644 index 00000000..686bf390 --- /dev/null +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidatorTest.java @@ -0,0 +1,113 @@ +package org.hypertrace.core.query.service.prometheus; + +import static java.util.Objects.requireNonNull; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createColumnExpression; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createFunctionExpression; +import static org.hypertrace.core.query.service.QueryRequestBuilderUtils.createOrderByExpression; + +import com.typesafe.config.Config; +import com.typesafe.config.ConfigFactory; +import org.hypertrace.core.query.service.ExecutionContext; +import org.hypertrace.core.query.service.QueryCost; +import org.hypertrace.core.query.service.api.Expression; +import org.hypertrace.core.query.service.api.QueryRequest; +import org.hypertrace.core.query.service.api.QueryRequest.Builder; +import org.hypertrace.core.query.service.api.SortOrder; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class QueryRequestEligibilityValidatorTest { + + private static final String TENANT_COLUMN_NAME = "tenant_id"; + private static final String TEST_REQUEST_HANDLER_CONFIG_FILE = "prometheus_request_handler.conf"; + + private QueryRequestEligibilityValidator queryRequestEligibilityValidator; + + @BeforeEach + public void setup() { + queryRequestEligibilityValidator = + new QueryRequestEligibilityValidator(getDefaultPrometheusViewDefinition()); + } + + @Test + void testCalculateCost_orderBy() { + QueryRequest queryRequest = buildOrderByQuery(); + + ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); + executionContext.setTimeFilterColumn("SERVICE.startTime"); + + Assertions.assertEquals( + QueryCost.UNSUPPORTED, + queryRequestEligibilityValidator.calculateCost(queryRequest, executionContext)); + } + + @Test + void testCalculateCost_groupByAndSelectionOnDifferentColumn() { + Builder builder = QueryRequest.newBuilder(); + Expression startTimeColumn = createColumnExpression("SERVICE.startTime").build(); + + builder.addSelection(createColumnExpression("SERVICE.id")); + builder.addSelection(startTimeColumn); + builder.addGroupBy(createColumnExpression("SERVICE.name")); + + QueryRequest queryRequest = builder.build(); + + ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); + executionContext.setTimeFilterColumn("SERVICE.startTime"); + + Assertions.assertEquals( + QueryCost.UNSUPPORTED, + queryRequestEligibilityValidator.calculateCost(queryRequest, executionContext)); + } + + @Test + void testCalculateCost_aggregationNotSupported() { + Builder builder = QueryRequest.newBuilder(); + builder.addAggregation( + createFunctionExpression("Count", createColumnExpression("SERVICE.name").build())); + + Expression startTimeColumn = createColumnExpression("SERVICE.startTime").build(); + + builder.addSelection(createColumnExpression("SERVICE.id")); + builder.addSelection(startTimeColumn); + builder.addGroupBy(createColumnExpression("SERVICE.name")); + + QueryRequest queryRequest = builder.build(); + + ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); + executionContext.setTimeFilterColumn("SERVICE.startTime"); + + Assertions.assertEquals( + QueryCost.UNSUPPORTED, + queryRequestEligibilityValidator.calculateCost(queryRequest, executionContext)); + } + + private PrometheusViewDefinition getDefaultPrometheusViewDefinition() { + Config fileConfig = + ConfigFactory.parseURL( + requireNonNull( + QueryRequestToPromqlConverterTest.class + .getClassLoader() + .getResource(TEST_REQUEST_HANDLER_CONFIG_FILE))); + + return PrometheusViewDefinition.parse( + fileConfig.getConfig("requestHandlerInfo.prometheusViewDefinition"), TENANT_COLUMN_NAME); + } + + private QueryRequest buildOrderByQuery() { + Builder builder = QueryRequest.newBuilder(); + Expression startTimeColumn = createColumnExpression("SERVICE.startTime").build(); + Expression endTimeColumn = createColumnExpression("SERVICE.endTime").build(); + + builder.addSelection(createColumnExpression("SERVICE.id")); + builder.addSelection(startTimeColumn); + builder.addSelection(endTimeColumn); + + builder.addOrderBy(createOrderByExpression(startTimeColumn.toBuilder(), SortOrder.DESC)); + builder.addOrderBy(createOrderByExpression(endTimeColumn.toBuilder(), SortOrder.ASC)); + + builder.setLimit(100); + return builder.build(); + } +} diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index 6c7dd140..5cbecda2 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -21,16 +21,16 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -public class QueryRequestToPromqlConverterTest { +class QueryRequestToPromqlConverterTest { private static final String TENANT_COLUMN_NAME = "tenant_id"; private static final String TEST_REQUEST_HANDLER_CONFIG_FILE = "prometheus_request_handler.conf"; @Test - public void testInstantQueryWithGroupByWithMultipleAggregates() { - QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQuery(); - Builder builder = QueryRequest.newBuilder(orderByQueryRequest); + void testInstantQueryWithGroupByWithMultipleAggregates() { + QueryRequest query = buildMultipleGroupByMultipleAggQuery(); + Builder builder = QueryRequest.newBuilder(query); builder.setLimit(20); PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); @@ -52,9 +52,9 @@ public void testInstantQueryWithGroupByWithMultipleAggregates() { } @Test - public void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { - QueryRequest orderByQueryRequest = buildMultipleGroupByMultipleAggQueryWithMultipleFilters(); - Builder builder = QueryRequest.newBuilder(orderByQueryRequest); + void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { + QueryRequest query = buildMultipleGroupByMultipleAggQueryWithMultipleFilters(); + Builder builder = QueryRequest.newBuilder(query); builder.setLimit(20); PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); @@ -78,10 +78,9 @@ public void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters } @Test - public void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { - QueryRequest orderByQueryRequest = - buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime(); - Builder builder = QueryRequest.newBuilder(orderByQueryRequest); + void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { + QueryRequest query = buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime(); + Builder builder = QueryRequest.newBuilder(query); builder.setLimit(20); PrometheusViewDefinition prometheusViewDefinition = getDefaultPrometheusViewDefinition(); From 93771dbc051cf60d8f39335366a37cedf0eb8582 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 18:19:09 +0530 Subject: [PATCH 21/31] Use Value lombok annotation --- .../hypertrace/core/query/service/QueryTimeRange.java | 10 +++++----- .../service/prometheus/PrometheusViewDefinition.java | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java index f4a6fa8b..eb4d11f9 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java @@ -3,13 +3,13 @@ import java.time.Duration; import java.time.Instant; import lombok.AllArgsConstructor; -import lombok.Getter; +import lombok.Value; -@Getter +@Value @AllArgsConstructor public class QueryTimeRange { - private final Instant startTime; - private final Instant endTime; - private final Duration duration; + Instant startTime; + Instant endTime; + Duration duration; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index cd6a22ca..79bfe706 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -10,7 +10,7 @@ import java.util.Map.Entry; import java.util.Set; import lombok.AllArgsConstructor; -import lombok.Getter; +import lombok.Value; /** Prometheus metric & attribute mapping for a pinot view */ class PrometheusViewDefinition { @@ -88,11 +88,11 @@ public String getTenantColumnName() { return tenantColumnName; } - @Getter + @Value @AllArgsConstructor public static class MetricConfig { - private final String name; - private final MetricType metricType; + String name; + MetricType metricType; } public enum MetricType { From 2bd4bc0f477c82148db04c5fc2e8291c00e1d6e2 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 18:19:48 +0530 Subject: [PATCH 22/31] Minor change --- .../java/org/hypertrace/core/query/service/QueryTimeRange.java | 1 - 1 file changed, 1 deletion(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java index eb4d11f9..77b11049 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/QueryTimeRange.java @@ -8,7 +8,6 @@ @Value @AllArgsConstructor public class QueryTimeRange { - Instant startTime; Instant endTime; Duration duration; From 21f3d5849834b9bd9a902dfba5ca0481445a2ec4 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 18:27:13 +0530 Subject: [PATCH 23/31] Minor changes here and der --- .../prometheus/PrometheusViewDefinition.java | 4 ++-- .../QueryRequestEligibilityValidator.java | 9 +++++--- .../QueryRequestToPromqlConverter.java | 21 +++++++------------ 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 79bfe706..54a062d1 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -72,11 +72,11 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN metricMap, fieldMap); } - public String getPhysicalColumnName(String logicalColumnName) { + public String getPhysicalColumnNameForLogicalColumnName(String logicalColumnName) { return columnMap.get(logicalColumnName); } - public MetricConfig getMetricConfig(String logicalMetricName) { + public MetricConfig getMetricConfigForLogicalMetricName(String logicalMetricName) { return metricMap.get(logicalMetricName); } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index c6b5890d..5285b8f6 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -51,8 +51,10 @@ QueryCost calculateCost(QueryRequest queryRequest, ExecutionContext executionCon Preconditions.checkArgument(!referencedColumns.isEmpty()); // all the columns in the request should have a mapping in the config for (String referencedColumn : referencedColumns) { - if (prometheusViewDefinition.getPhysicalColumnName(referencedColumn) == null - && prometheusViewDefinition.getMetricConfig(referencedColumn) == null) { + if (prometheusViewDefinition.getPhysicalColumnNameForLogicalColumnName(referencedColumn) + == null + && prometheusViewDefinition.getMetricConfigForLogicalMetricName(referencedColumn) + == null) { return QueryCost.UNSUPPORTED; } } @@ -108,7 +110,8 @@ private boolean areAggregationsNotSupported(List aggregationList) { function.getFunctionName())) { return true; } - return prometheusViewDefinition.getMetricConfig(attributeId) == null; + return prometheusViewDefinition.getMetricConfigForLogicalMetricName(attributeId) + == null; }); } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 87631e90..b220f811 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -2,12 +2,12 @@ import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; -import com.google.common.collect.Lists; import com.google.protobuf.ByteString; import java.time.Duration; import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; +import java.util.function.Predicate; import java.util.stream.Collectors; import org.apache.commons.codec.binary.Hex; import org.hypertrace.core.query.service.ExecutionContext; @@ -106,17 +106,10 @@ private String mapToPromqlQuery( } private List getGroupByList(QueryRequest queryRequest) { - List groupByList = Lists.newArrayList(); - if (queryRequest.getGroupByCount() > 0) { - for (Expression expression : queryRequest.getGroupByList()) { - // skip datetime function in group-by - if (QueryRequestUtil.isDateTimeFunction(expression)) { - continue; - } - groupByList.add(convertColumnAttributeToString(expression)); - } - } - return groupByList; + return queryRequest.getGroupByList().stream() + .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) + .map(this::convertColumnAttributeToString) + .collect(Collectors.toUnmodifiableList()); } private Duration getTimeSeriesPeriod(ExecutionContext executionContext) { @@ -140,13 +133,13 @@ private String buildQuery( } private MetricConfig getMetricConfigForFunction(Expression functionSelection) { - return prometheusViewDefinition.getMetricConfig( + return prometheusViewDefinition.getMetricConfigForLogicalMetricName( getLogicalColumnNameForSimpleColumnExpression( functionSelection.getFunction().getArgumentsList().get(0))); } private String convertColumnAttributeToString(Expression expression) { - return prometheusViewDefinition.getPhysicalColumnName( + return prometheusViewDefinition.getPhysicalColumnNameForLogicalColumnName( getLogicalColumnNameForSimpleColumnExpression(expression)); } From feab5ba30c5110ab6e858943f40ffde128f3138f Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 18:28:51 +0530 Subject: [PATCH 24/31] Minor change --- .../query/service/prometheus/PrometheusFunctionConverter.java | 2 +- .../query/service/prometheus/QueryRequestToPromqlConverter.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java index 2bf59269..7100c057 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusFunctionConverter.java @@ -35,7 +35,7 @@ String mapToPrometheusFunctionName(Expression functionSelection) { default: throw new RuntimeException( String.format( - "Couldn't map query function %s to prometheus function", queryFunctionName)); + "Couldn't map query function [%s] to prometheus function", queryFunctionName)); } } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index b220f811..820a8361 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -179,7 +179,7 @@ private String convertOperatorToString(Operator operator) { return "=~"; default: throw new RuntimeException( - String.format("Equivalent operator %s not supported in promql", operator)); + String.format("Equivalent %s operator not supported in promql", operator)); } } From d05d02d061086e88075a4c5cd4f037830b357f56 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 19:46:48 +0530 Subject: [PATCH 25/31] Move filter converter to separate class --- .../prometheus/FilterToPromqlConverter.java | 128 ++++++++++++++++++ .../QueryRequestToPromqlConverter.java | 118 +--------------- 2 files changed, 132 insertions(+), 114 deletions(-) create mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java new file mode 100644 index 00000000..445e8890 --- /dev/null +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java @@ -0,0 +1,128 @@ +package org.hypertrace.core.query.service.prometheus; + +import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; + +import com.google.protobuf.ByteString; +import java.util.List; +import java.util.function.Function; +import org.apache.commons.codec.binary.Hex; +import org.hypertrace.core.query.service.QueryRequestUtil; +import org.hypertrace.core.query.service.api.Expression; +import org.hypertrace.core.query.service.api.Filter; +import org.hypertrace.core.query.service.api.LiteralConstant; +import org.hypertrace.core.query.service.api.Operator; +import org.hypertrace.core.query.service.api.Value; + +class FilterToPromqlConverter { + + /** + * only `AND` operator in filter is allowed + * + *

rhs of leaf filter should be literal + */ + void convertFilterToString( + Filter filter, + List filterList, + String timeFilterColumn, + Function expressionToColumnConverter) { + if (filter.getChildFilterCount() > 0) { + for (Filter childFilter : filter.getChildFilterList()) { + convertFilterToString( + childFilter, filterList, timeFilterColumn, expressionToColumnConverter); + } + } else { + if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs()) + && timeFilterColumn.equals( + getLogicalColumnNameForSimpleColumnExpression(filter.getLhs()))) { + return; + } + StringBuilder builder = new StringBuilder(); + builder.append(expressionToColumnConverter.apply(filter.getLhs())); + builder.append(convertOperatorToString(filter.getOperator())); + builder.append(convertLiteralToString(filter.getRhs().getLiteral())); + filterList.add(builder.toString()); + } + } + + private String convertOperatorToString(Operator operator) { + switch (operator) { + case IN: + case EQ: + return "="; + case NEQ: + return "!="; + case LIKE: + return "=~"; + default: + throw new RuntimeException( + String.format("Equivalent %s operator not supported in promql", operator)); + } + } + + private String convertLiteralToString(LiteralConstant literal) { + Value value = literal.getValue(); + String ret = null; + switch (value.getValueType()) { + case STRING_ARRAY: + StringBuilder builder = new StringBuilder("\""); + for (String item : value.getStringArrayList()) { + if (builder.length() > 1) { + builder.append("|"); + } + builder.append(item); + } + builder.append("\""); + ret = builder.toString(); + break; + case BYTES_ARRAY: + builder = new StringBuilder("\""); + for (ByteString item : value.getBytesArrayList()) { + if (builder.length() > 1) { + builder.append("|"); + } + builder.append(Hex.encodeHexString(item.toByteArray())); + } + builder.append("\""); + ret = builder.toString(); + break; + case STRING: + ret = "\"" + value.getString() + "\""; + break; + case LONG: + ret = "\"" + value.getLong() + "\""; + break; + case INT: + ret = "\"" + value.getInt() + "\""; + break; + case FLOAT: + ret = "\"" + value.getFloat() + "\""; + break; + case DOUBLE: + ret = "\"" + value.getDouble() + "\""; + break; + case BYTES: + ret = "\"" + Hex.encodeHexString(value.getBytes().toByteArray()) + "\""; + break; + case BOOL: + ret = "\"" + value.getBoolean() + "\""; + break; + case TIMESTAMP: + ret = "\"" + value.getTimestamp() + "\""; + break; + case NULL_NUMBER: + ret = "0"; + break; + case NULL_STRING: + ret = "null"; + break; + case LONG_ARRAY: + case INT_ARRAY: + case FLOAT_ARRAY: + case DOUBLE_ARRAY: + case BOOLEAN_ARRAY: + case UNRECOGNIZED: + break; + } + return ret; + } +} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 820a8361..e5fa2f1d 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -2,34 +2,30 @@ import static org.hypertrace.core.query.service.QueryRequestUtil.getLogicalColumnNameForSimpleColumnExpression; -import com.google.protobuf.ByteString; import java.time.Duration; import java.util.ArrayList; import java.util.LinkedHashSet; import java.util.List; import java.util.function.Predicate; import java.util.stream.Collectors; -import org.apache.commons.codec.binary.Hex; import org.hypertrace.core.query.service.ExecutionContext; import org.hypertrace.core.query.service.QueryRequestUtil; import org.hypertrace.core.query.service.QueryTimeRange; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Expression.ValueCase; -import org.hypertrace.core.query.service.api.Filter; -import org.hypertrace.core.query.service.api.LiteralConstant; -import org.hypertrace.core.query.service.api.Operator; import org.hypertrace.core.query.service.api.QueryRequest; -import org.hypertrace.core.query.service.api.Value; import org.hypertrace.core.query.service.prometheus.PrometheusViewDefinition.MetricConfig; class QueryRequestToPromqlConverter { private final PrometheusViewDefinition prometheusViewDefinition; private final PrometheusFunctionConverter prometheusFunctionConverter; + private final FilterToPromqlConverter filterToPromqlConverter; QueryRequestToPromqlConverter(PrometheusViewDefinition prometheusViewDefinition) { this.prometheusViewDefinition = prometheusViewDefinition; this.prometheusFunctionConverter = new PrometheusFunctionConverter(); + this.filterToPromqlConverter = new FilterToPromqlConverter(); } PromQLInstantQuery convertToPromqlInstantQuery( @@ -74,7 +70,8 @@ private List buildPromqlQueries( List groupByList = getGroupByList(request); List filterList = new ArrayList<>(); - convertFilterToString(request.getFilter(), filterList, timeFilterColumn); + filterToPromqlConverter.convertFilterToString( + request.getFilter(), filterList, timeFilterColumn, this::convertColumnAttributeToString); // iterate over all the functions in the query except for date time function (which is handled // separately and not a part of the query string) @@ -142,111 +139,4 @@ private String convertColumnAttributeToString(Expression expression) { return prometheusViewDefinition.getPhysicalColumnNameForLogicalColumnName( getLogicalColumnNameForSimpleColumnExpression(expression)); } - - /** - * only `AND` operator in filter is allowed - * - *

rhs of leaf filter should be literal - */ - private void convertFilterToString( - Filter filter, List filterList, String timeFilterColumn) { - if (filter.getChildFilterCount() > 0) { - for (Filter childFilter : filter.getChildFilterList()) { - convertFilterToString(childFilter, filterList, timeFilterColumn); - } - } else { - if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs()) - && timeFilterColumn.equals( - getLogicalColumnNameForSimpleColumnExpression(filter.getLhs()))) { - return; - } - StringBuilder builder = new StringBuilder(); - builder.append(convertColumnAttributeToString(filter.getLhs())); - builder.append(convertOperatorToString(filter.getOperator())); - builder.append(convertLiteralToString(filter.getRhs().getLiteral())); - filterList.add(builder.toString()); - } - } - - private String convertOperatorToString(Operator operator) { - switch (operator) { - case IN: - case EQ: - return "="; - case NEQ: - return "!="; - case LIKE: - return "=~"; - default: - throw new RuntimeException( - String.format("Equivalent %s operator not supported in promql", operator)); - } - } - - private String convertLiteralToString(LiteralConstant literal) { - Value value = literal.getValue(); - String ret = null; - switch (value.getValueType()) { - case STRING_ARRAY: - StringBuilder builder = new StringBuilder("\""); - for (String item : value.getStringArrayList()) { - if (builder.length() > 1) { - builder.append("|"); - } - builder.append(item); - } - builder.append("\""); - ret = builder.toString(); - break; - case BYTES_ARRAY: - builder = new StringBuilder("\""); - for (ByteString item : value.getBytesArrayList()) { - if (builder.length() > 1) { - builder.append("|"); - } - builder.append(Hex.encodeHexString(item.toByteArray())); - } - builder.append("\""); - ret = builder.toString(); - break; - case STRING: - ret = "\"" + value.getString() + "\""; - break; - case LONG: - ret = "\"" + value.getLong() + "\""; - break; - case INT: - ret = "\"" + value.getInt() + "\""; - break; - case FLOAT: - ret = "\"" + value.getFloat() + "\""; - break; - case DOUBLE: - ret = "\"" + value.getDouble() + "\""; - break; - case BYTES: - ret = "\"" + Hex.encodeHexString(value.getBytes().toByteArray()) + "\""; - break; - case BOOL: - ret = "\"" + value.getBoolean() + "\""; - break; - case TIMESTAMP: - ret = "\"" + value.getTimestamp() + "\""; - break; - case NULL_NUMBER: - ret = "0"; - break; - case NULL_STRING: - ret = "null"; - break; - case LONG_ARRAY: - case INT_ARRAY: - case FLOAT_ARRAY: - case DOUBLE_ARRAY: - case BOOLEAN_ARRAY: - case UNRECOGNIZED: - break; - } - return ret; - } } From 55d2bb29215f84b5bdbefd70d29eed74426f9896 Mon Sep 17 00:00:00 2001 From: rish691 Date: Wed, 8 Dec 2021 20:14:30 +0530 Subject: [PATCH 26/31] delete unused class --- .../prometheus/PromQLInstantQuery.java | 16 -------------- .../service/prometheus/PromQLRangeQuery.java | 21 ------------------- .../QueryRequestToPromqlConverter.java | 8 +++---- .../QueryRequestToPromqlConverterTest.java | 8 +++---- 4 files changed, 8 insertions(+), 45 deletions(-) delete mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java delete mode 100644 query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java deleted file mode 100644 index 96b0863b..00000000 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLInstantQuery.java +++ /dev/null @@ -1,16 +0,0 @@ -package org.hypertrace.core.query.service.prometheus; - -import java.time.Instant; -import java.util.List; -import lombok.Builder; -import lombok.Getter; -import lombok.NonNull; -import lombok.Singular; - -@Getter -@Builder -class PromQLInstantQuery { - @NonNull @Singular private List queries; - - @NonNull private Instant evalTime; -} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java deleted file mode 100644 index b94992e9..00000000 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PromQLRangeQuery.java +++ /dev/null @@ -1,21 +0,0 @@ -package org.hypertrace.core.query.service.prometheus; - -import java.time.Duration; -import java.time.Instant; -import java.util.List; -import lombok.Builder; -import lombok.Getter; -import lombok.NonNull; -import lombok.Singular; - -@Getter -@Builder -class PromQLRangeQuery { - @NonNull @Singular private List queries; - - @NonNull private Instant startTime; - - @NonNull private Instant endTime; - - @NonNull private Duration step; -} diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index e5fa2f1d..8b241459 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -28,7 +28,7 @@ class QueryRequestToPromqlConverter { this.filterToPromqlConverter = new FilterToPromqlConverter(); } - PromQLInstantQuery convertToPromqlInstantQuery( + PromQLInstantQueries convertToPromqlInstantQuery( ExecutionContext executionContext, QueryRequest request, LinkedHashSet allSelections) { @@ -38,13 +38,13 @@ PromQLInstantQuery convertToPromqlInstantQuery( .getQueryTimeRange() .orElseThrow(() -> new RuntimeException("Time Range missing in query")); - return new PromQLInstantQuery( + return new PromQLInstantQueries( buildPromqlQueries( request, allSelections, queryTimeRange, executionContext.getTimeFilterColumn()), queryTimeRange.getEndTime()); } - PromQLRangeQuery convertToPromqlRangeQuery( + PromQLRangeQueries convertToPromqlRangeQuery( ExecutionContext executionContext, QueryRequest request, LinkedHashSet allSelections) { @@ -54,7 +54,7 @@ PromQLRangeQuery convertToPromqlRangeQuery( .getQueryTimeRange() .orElseThrow(() -> new RuntimeException("Time Range missing in query")); - return new PromQLRangeQuery( + return new PromQLRangeQueries( buildPromqlQueries( request, allSelections, queryTimeRange, executionContext.getTimeFilterColumn()), queryTimeRange.getStartTime(), diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index 5cbecda2..2d723cb3 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -38,7 +38,7 @@ void testInstantQueryWithGroupByWithMultipleAggregates() { ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromQLInstantQuery promqlQuery = + PromQLInstantQueries promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) .convertToPromqlInstantQuery( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); @@ -62,7 +62,7 @@ void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromQLInstantQuery promqlQuery = + PromQLInstantQueries promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) .convertToPromqlInstantQuery( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); @@ -88,7 +88,7 @@ void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { ExecutionContext executionContext = new ExecutionContext("__default", queryRequest); executionContext.setTimeFilterColumn("SERVICE.startTime"); - PromQLRangeQuery promqlQuery = + PromQLRangeQueries promqlQuery = new QueryRequestToPromqlConverter(prometheusViewDefinition) .convertToPromqlRangeQuery( executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); @@ -101,7 +101,7 @@ void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); - Assertions.assertEquals(15000, promqlQuery.getStep().toMillis()); + Assertions.assertEquals(15000, promqlQuery.getPeriod().toMillis()); } private QueryRequest buildMultipleGroupByMultipleAggQuery() { From 90e91037283a99f4811eee36d01b9c401e4adabf Mon Sep 17 00:00:00 2001 From: rish691 Date: Thu, 9 Dec 2021 12:57:36 +0530 Subject: [PATCH 27/31] Minor changes --- .../prometheus/QueryRequestEligibilityValidator.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 5285b8f6..67a7392d 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -35,8 +35,11 @@ QueryCost calculateCost(QueryRequest queryRequest, ExecutionContext executionCon // only aggregation queries are supported if (queryRequest.getAggregationCount() == 0 - || queryRequest.getGroupByCount() == 0 - || queryRequest.getDistinctSelections()) { + || queryRequest.getGroupByCount() == 0) { + return QueryCost.UNSUPPORTED; + } + + if (queryRequest.getDistinctSelections()) { return QueryCost.UNSUPPORTED; } @@ -63,6 +66,7 @@ QueryCost calculateCost(QueryRequest queryRequest, ExecutionContext executionCon return QueryCost.UNSUPPORTED; } + // if selection and groupBy should be on same column or simple attribute if (selectionAndGroupByOnDifferentColumn( queryRequest.getSelectionList(), queryRequest.getGroupByList())) { return QueryCost.UNSUPPORTED; @@ -91,7 +95,6 @@ private boolean selectionAndGroupByOnDifferentColumn( .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) .map(QueryRequestUtil::getLogicalColumnNameForSimpleColumnExpression) .collect(Collectors.toSet()); - // all selection and group by should be on same column return !selections.equals(groupBys); } From eee7e1a575d9c80998eeb5ac1111be9b37bc16e6 Mon Sep 17 00:00:00 2001 From: rish691 Date: Thu, 9 Dec 2021 12:58:23 +0530 Subject: [PATCH 28/31] spotless --- .../prometheus/FilterToPromqlConverter.java | 41 +++++++------------ .../prometheus/PrometheusViewDefinition.java | 2 +- .../QueryRequestEligibilityValidator.java | 3 +- 3 files changed, 16 insertions(+), 30 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java index 445e8890..30ec2f7c 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java @@ -61,7 +61,6 @@ private String convertOperatorToString(Operator operator) { private String convertLiteralToString(LiteralConstant literal) { Value value = literal.getValue(); - String ret = null; switch (value.getValueType()) { case STRING_ARRAY: StringBuilder builder = new StringBuilder("\""); @@ -72,8 +71,7 @@ private String convertLiteralToString(LiteralConstant literal) { builder.append(item); } builder.append("\""); - ret = builder.toString(); - break; + return builder.toString(); case BYTES_ARRAY: builder = new StringBuilder("\""); for (ByteString item : value.getBytesArrayList()) { @@ -83,46 +81,35 @@ private String convertLiteralToString(LiteralConstant literal) { builder.append(Hex.encodeHexString(item.toByteArray())); } builder.append("\""); - ret = builder.toString(); - break; + return builder.toString(); case STRING: - ret = "\"" + value.getString() + "\""; - break; + return "\"" + value.getString() + "\""; case LONG: - ret = "\"" + value.getLong() + "\""; - break; + return "\"" + value.getLong() + "\""; case INT: - ret = "\"" + value.getInt() + "\""; - break; + return "\"" + value.getInt() + "\""; case FLOAT: - ret = "\"" + value.getFloat() + "\""; - break; + return "\"" + value.getFloat() + "\""; case DOUBLE: - ret = "\"" + value.getDouble() + "\""; - break; + return "\"" + value.getDouble() + "\""; case BYTES: - ret = "\"" + Hex.encodeHexString(value.getBytes().toByteArray()) + "\""; - break; + return "\"" + Hex.encodeHexString(value.getBytes().toByteArray()) + "\""; case BOOL: - ret = "\"" + value.getBoolean() + "\""; - break; + return "\"" + value.getBoolean() + "\""; case TIMESTAMP: - ret = "\"" + value.getTimestamp() + "\""; - break; + return "\"" + value.getTimestamp() + "\""; case NULL_NUMBER: - ret = "0"; - break; + return "0"; case NULL_STRING: - ret = "null"; - break; + return "null"; case LONG_ARRAY: case INT_ARRAY: case FLOAT_ARRAY: case DOUBLE_ARRAY: case BOOLEAN_ARRAY: case UNRECOGNIZED: - break; + default: + return null; } - return ret; } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 54a062d1..91f603ba 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -48,7 +48,6 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN fieldMap.put(keys.get(0), fieldMapConfig.getString(element.getKey())); } - final Map metricMap = Maps.newHashMap(); Config metricMapConfig = config.getConfig(METRIC_MAP_CONFIG_KEY); Set metricNames = Sets.newHashSet(); @@ -57,6 +56,7 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN metricNames.add(keys.get(0)); } + final Map metricMap = Maps.newHashMap(); String metricScope = config.getString(METRIC_SCOPE_CONFIG_KEY); for (String metricName : metricNames) { Config metricDef = metricMapConfig.getConfig(metricName); diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 67a7392d..e89f55f0 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -34,8 +34,7 @@ QueryCost calculateCost(QueryRequest queryRequest, ExecutionContext executionCon } // only aggregation queries are supported - if (queryRequest.getAggregationCount() == 0 - || queryRequest.getGroupByCount() == 0) { + if (queryRequest.getAggregationCount() == 0 || queryRequest.getGroupByCount() == 0) { return QueryCost.UNSUPPORTED; } From 1400d7325a1ad189a60e2f33f0d1ce971eec55af Mon Sep 17 00:00:00 2001 From: rish691 Date: Thu, 9 Dec 2021 18:19:02 +0530 Subject: [PATCH 29/31] Address review comment --- .../prometheus/FilterToPromqlConverter.java | 15 +++--- .../PrometheusBasedRequestHandler.java | 2 +- .../prometheus/PrometheusViewDefinition.java | 16 +++--- .../QueryRequestEligibilityValidator.java | 32 +++++++----- .../QueryRequestToPromqlConverter.java | 52 +++++++++++-------- .../QueryRequestToPromqlConverterTest.java | 14 ++--- .../resources/prometheus_request_handler.conf | 5 -- 7 files changed, 72 insertions(+), 64 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java index 30ec2f7c..84e6060a 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java @@ -15,20 +15,16 @@ class FilterToPromqlConverter { - /** - * only `AND` operator in filter is allowed - * - *

rhs of leaf filter should be literal - */ + /** only `AND` operator in filter is allowed rhs of leaf filter should be literal */ void convertFilterToString( Filter filter, - List filterList, String timeFilterColumn, - Function expressionToColumnConverter) { + Function expressionToColumnConverter, + List filterList) { if (filter.getChildFilterCount() > 0) { for (Filter childFilter : filter.getChildFilterList()) { convertFilterToString( - childFilter, filterList, timeFilterColumn, expressionToColumnConverter); + childFilter, timeFilterColumn, expressionToColumnConverter, filterList); } } else { if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs()) @@ -109,7 +105,8 @@ private String convertLiteralToString(LiteralConstant literal) { case BOOLEAN_ARRAY: case UNRECOGNIZED: default: - return null; + throw new RuntimeException( + String.format("Literal type %s not supported", value.getValueType())); } } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index ee7d337a..80edb2c4 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -12,7 +12,7 @@ public class PrometheusBasedRequestHandler implements RequestHandler { - public static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition"; + private static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition"; private static final String TENANT_COLUMN_NAME_CONFIG_KEY = "tenantAttributeName"; private static final String START_TIME_ATTRIBUTE_NAME_CONFIG_KEY = "startTimeAttributeName"; diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 91f603ba..620d0922 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -23,9 +23,9 @@ class PrometheusViewDefinition { private static final String METRIC_SCOPE_CONFIG_KEY = "metricScope"; private final String viewName; - private final String tenantColumnName; + private final String tenantAttributeName; private final Map metricMap; - private final Map columnMap; + private final Map attributeMap; public PrometheusViewDefinition( String viewName, @@ -33,9 +33,9 @@ public PrometheusViewDefinition( Map metricMap, Map columnMap) { this.viewName = viewName; - this.tenantColumnName = tenantColumnName; // tenantAttributeName + this.tenantAttributeName = tenantColumnName; this.metricMap = metricMap; - this.columnMap = columnMap; + this.attributeMap = columnMap; } public static PrometheusViewDefinition parse(Config config, String tenantColumnName) { @@ -73,7 +73,7 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN } public String getPhysicalColumnNameForLogicalColumnName(String logicalColumnName) { - return columnMap.get(logicalColumnName); + return attributeMap.get(logicalColumnName); } public MetricConfig getMetricConfigForLogicalMetricName(String logicalMetricName) { @@ -84,14 +84,14 @@ public String getViewName() { return viewName; } - public String getTenantColumnName() { - return tenantColumnName; + public String getTenantAttributeName() { + return tenantAttributeName; } @Value @AllArgsConstructor public static class MetricConfig { - String name; + String metricName; MetricType metricType; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index e89f55f0..95825146 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -16,6 +16,8 @@ import org.hypertrace.core.query.service.api.Function; import org.hypertrace.core.query.service.api.Operator; import org.hypertrace.core.query.service.api.QueryRequest; +import org.hypertrace.core.query.service.prometheus.PrometheusViewDefinition.MetricConfig; +import org.hypertrace.core.query.service.prometheus.PrometheusViewDefinition.MetricType; /** Set of rules to check if the given request can be served by prometheus */ class QueryRequestEligibilityValidator { @@ -98,6 +100,10 @@ private boolean selectionAndGroupByOnDifferentColumn( } private boolean areAggregationsNotSupported(List aggregationList) { + // supported aggregation must have single argument (except for dateTimeConvert) + // and prometheusViewDef must have mapping for the metric in the argument + // the function type must be supported + // right now only GAUGE type of metric is supported return aggregationList.stream() .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) .anyMatch( @@ -112,13 +118,16 @@ private boolean areAggregationsNotSupported(List aggregationList) { function.getFunctionName())) { return true; } - return prometheusViewDefinition.getMetricConfigForLogicalMetricName(attributeId) - == null; + MetricConfig metricConfig = + prometheusViewDefinition.getMetricConfigForLogicalMetricName(attributeId); + return null == metricConfig || metricConfig.getMetricType() != MetricType.GAUGE; }); } private boolean isFilterNotSupported(Filter filter) { if (filter.getChildFilterCount() > 0) { + // AND high level operator is supported + // later OR operator can be supported for same column if (filter.getOperator() != Operator.AND) { return true; } @@ -127,19 +136,18 @@ private boolean isFilterNotSupported(Filter filter) { return true; } } - } - - // filter rhs should be literal only - if (filter.getRhs().getValueCase() != ValueCase.LITERAL) { - return true; - } + } else { + // filter rhs should be literal only + if (filter.getRhs().getValueCase() != ValueCase.LITERAL) { + return true; + } - // filter lhs should be column or simple attribute - if (!QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())) { - return true; + // filter lhs should be column or simple attribute + if (!QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())) { + return true; + } } - // todo check for valid operators here return false; } } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 8b241459..18c153bf 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -32,15 +32,14 @@ PromQLInstantQueries convertToPromqlInstantQuery( ExecutionContext executionContext, QueryRequest request, LinkedHashSet allSelections) { - - QueryTimeRange queryTimeRange = - executionContext - .getQueryTimeRange() - .orElseThrow(() -> new RuntimeException("Time Range missing in query")); - + QueryTimeRange queryTimeRange = getQueryTimeRange(executionContext); return new PromQLInstantQueries( buildPromqlQueries( - request, allSelections, queryTimeRange, executionContext.getTimeFilterColumn()), + executionContext.getTenantId(), + request, + allSelections, + queryTimeRange.getDuration(), + executionContext.getTimeFilterColumn()), queryTimeRange.getEndTime()); } @@ -48,30 +47,38 @@ PromQLRangeQueries convertToPromqlRangeQuery( ExecutionContext executionContext, QueryRequest request, LinkedHashSet allSelections) { - - QueryTimeRange queryTimeRange = - executionContext - .getQueryTimeRange() - .orElseThrow(() -> new RuntimeException("Time Range missing in query")); - + QueryTimeRange queryTimeRange = getQueryTimeRange(executionContext); return new PromQLRangeQueries( buildPromqlQueries( - request, allSelections, queryTimeRange, executionContext.getTimeFilterColumn()), + executionContext.getTenantId(), + request, + allSelections, + executionContext.getTimeSeriesPeriod().get(), + executionContext.getTimeFilterColumn()), queryTimeRange.getStartTime(), queryTimeRange.getEndTime(), getTimeSeriesPeriod(executionContext)); } + private QueryTimeRange getQueryTimeRange(ExecutionContext executionContext) { + return executionContext + .getQueryTimeRange() + .orElseThrow(() -> new RuntimeException("Time Range missing in query")); + } + private List buildPromqlQueries( + String tenantId, QueryRequest request, LinkedHashSet allSelections, - QueryTimeRange queryTimeRange, + Duration queryDuration, String timeFilterColumn) { List groupByList = getGroupByList(request); List filterList = new ArrayList<>(); + filterList.add( + String.format("%s=\"%s\"", prometheusViewDefinition.getTenantAttributeName(), tenantId)); filterToPromqlConverter.convertFilterToString( - request.getFilter(), filterList, timeFilterColumn, this::convertColumnAttributeToString); + request.getFilter(), timeFilterColumn, this::convertColumnAttributeToString, filterList); // iterate over all the functions in the query except for date time function (which is handled // separately and not a part of the query string) @@ -82,7 +89,7 @@ private List buildPromqlQueries( && !QueryRequestUtil.isDateTimeFunction(expression)) .map( functionExpression -> - mapToPromqlQuery(functionExpression, groupByList, filterList, queryTimeRange)) + mapToPromqlQuery(functionExpression, groupByList, filterList, queryDuration)) .collect(Collectors.toUnmodifiableList()); } @@ -90,16 +97,16 @@ private String mapToPromqlQuery( Expression functionExpression, List groupByList, List filterList, - QueryTimeRange queryTimeRange) { + Duration queryDuration) { String functionName = prometheusFunctionConverter.mapToPrometheusFunctionName(functionExpression); MetricConfig metricConfig = getMetricConfigForFunction(functionExpression); return buildQuery( - metricConfig.getName(), + metricConfig.getMetricName(), functionName, String.join(", ", groupByList), String.join(", ", filterList), - queryTimeRange.getDuration().toMillis()); + queryDuration.toMillis()); } private List getGroupByList(QueryRequest queryRequest) { @@ -115,9 +122,8 @@ private Duration getTimeSeriesPeriod(ExecutionContext executionContext) { private String buildQuery( String metricName, String function, String groupByList, String filter, long durationMillis) { - String template = - "%s by (%s) (%s(%s{%s}[%sms]))"; // sum by (a1, a2) (sum_over_time(num_calls{a4="..", - // a5=".."}[xms])) + // sum by (a1, a2) (sum_over_time(num_calls{a4="..", a5=".."}[xms])) + String template = "%s by (%s) (%s(%s{%s}[%sms]))"; return String.format( template, diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index 2d723cb3..c7704628 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -44,8 +44,10 @@ void testInstantQueryWithGroupByWithMultipleAggregates() { executionContext, builder.build(), createSelectionsFromQueryRequest(queryRequest)); // time filter is removed from the query - String query1 = "count by (service_name, api_name) (count_over_time(error_count{}[100ms]))"; - String query2 = "avg by (service_name, api_name) (avg_over_time(num_calls{}[100ms]))"; + String query1 = + "count by (service_name, api_name) (count_over_time(error_count{tenant_id=\"__default\"}[100ms]))"; + String query2 = + "avg by (service_name, api_name) (avg_over_time(num_calls{tenant_id=\"__default\"}[100ms]))"; Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); @@ -69,9 +71,9 @@ void testInstantQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { // time filter is removed from the query String query1 = - "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + "count by (service_name, api_name) (count_over_time(error_count{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; String query2 = - "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + "avg by (service_name, api_name) (avg_over_time(num_calls{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); @@ -95,9 +97,9 @@ void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { // time filter is removed from the query String query1 = - "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + "count by (service_name, api_name) (count_over_time(error_count{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[15000ms]))"; String query2 = - "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"; + "avg by (service_name, api_name) (avg_over_time(num_calls{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[15000ms]))"; Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); diff --git a/query-service-impl/src/test/resources/prometheus_request_handler.conf b/query-service-impl/src/test/resources/prometheus_request_handler.conf index be9c1e2e..76a31dac 100644 --- a/query-service-impl/src/test/resources/prometheus_request_handler.conf +++ b/query-service-impl/src/test/resources/prometheus_request_handler.conf @@ -23,11 +23,6 @@ "SERVICE.name": "service_name", "API.id": "api_id", "API.name": "api_name", - "EVENT.protocolName": "protocol_name", - "EVENT.status_code": "status_code", - "API_TRACE.status_code": "status_code", - "API.startTime": "start_time_millis", - "API.endTime": "end_time_millis", "SERVICE.startTime": "start_time_millis", "SERVICE.endTime": "end_time_millis", } From c108ae5f28259aa4212bf6dd945f26b68047a6dc Mon Sep 17 00:00:00 2001 From: rish691 Date: Thu, 9 Dec 2021 18:55:19 +0530 Subject: [PATCH 30/31] update test --- .../PrometheusBasedRequestHandler.java | 13 ++++++++----- .../prometheus/PrometheusViewDefinition.java | 18 +++++++++--------- .../QueryRequestEligibilityValidator.java | 10 +++++----- .../QueryRequestToPromqlConverter.java | 13 ++++++++----- .../QueryRequestToPromqlConverterTest.java | 12 ++++++------ 5 files changed, 36 insertions(+), 30 deletions(-) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java index 80edb2c4..d4c11191 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java @@ -13,7 +13,7 @@ public class PrometheusBasedRequestHandler implements RequestHandler { private static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition"; - private static final String TENANT_COLUMN_NAME_CONFIG_KEY = "tenantAttributeName"; + private static final String TENANT_ATTRIBUTE_NAME_CONFIG_KEY = "tenantAttributeName"; private static final String START_TIME_ATTRIBUTE_NAME_CONFIG_KEY = "startTimeAttributeName"; private final QueryRequestEligibilityValidator queryRequestEligibilityValidator; @@ -41,15 +41,18 @@ public Optional getTimeFilterColumn() { private void processConfig(Config config) { - if (!config.hasPath(TENANT_COLUMN_NAME_CONFIG_KEY)) { + if (!config.hasPath(TENANT_ATTRIBUTE_NAME_CONFIG_KEY)) { throw new RuntimeException( - TENANT_COLUMN_NAME_CONFIG_KEY + " is not defined in the " + name + " request handler."); + TENANT_ATTRIBUTE_NAME_CONFIG_KEY + + " is not defined in the " + + name + + " request handler."); } - String tenantColumnName = config.getString(TENANT_COLUMN_NAME_CONFIG_KEY); + String tenantAttributeName = config.getString(TENANT_ATTRIBUTE_NAME_CONFIG_KEY); this.prometheusViewDefinition = PrometheusViewDefinition.parse( - config.getConfig(VIEW_DEFINITION_CONFIG_KEY), tenantColumnName); + config.getConfig(VIEW_DEFINITION_CONFIG_KEY), tenantAttributeName); this.startTimeAttributeName = config.hasPath(START_TIME_ATTRIBUTE_NAME_CONFIG_KEY) diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java index 620d0922..8f202374 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java @@ -29,23 +29,23 @@ class PrometheusViewDefinition { public PrometheusViewDefinition( String viewName, - String tenantColumnName, + String tenantAttributeName, Map metricMap, - Map columnMap) { + Map attributeMap) { this.viewName = viewName; - this.tenantAttributeName = tenantColumnName; + this.tenantAttributeName = tenantAttributeName; this.metricMap = metricMap; - this.attributeMap = columnMap; + this.attributeMap = attributeMap; } - public static PrometheusViewDefinition parse(Config config, String tenantColumnName) { + public static PrometheusViewDefinition parse(Config config, String tenantAttributeName) { String viewName = config.getString(VIEW_NAME_CONFIG_KEY); - final Map fieldMap = Maps.newHashMap(); + final Map attributeMap = Maps.newHashMap(); Config fieldMapConfig = config.getConfig(ATTRIBUTE_MAP_CONFIG_KEY); for (Entry element : fieldMapConfig.entrySet()) { List keys = ConfigUtil.splitPath(element.getKey()); - fieldMap.put(keys.get(0), fieldMapConfig.getString(element.getKey())); + attributeMap.put(keys.get(0), fieldMapConfig.getString(element.getKey())); } Config metricMapConfig = config.getConfig(METRIC_MAP_CONFIG_KEY); @@ -68,8 +68,8 @@ public static PrometheusViewDefinition parse(Config config, String tenantColumnN } return new PrometheusViewDefinition( - viewName, tenantColumnName, - metricMap, fieldMap); + viewName, tenantAttributeName, + metricMap, attributeMap); } public String getPhysicalColumnNameForLogicalColumnName(String logicalColumnName) { diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java index 95825146..4b271810 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java @@ -101,8 +101,8 @@ private boolean selectionAndGroupByOnDifferentColumn( private boolean areAggregationsNotSupported(List aggregationList) { // supported aggregation must have single argument (except for dateTimeConvert) - // and prometheusViewDef must have mapping for the metric in the argument - // the function type must be supported + // prometheusViewDef must have mapping for the metric + // function type must be supported // right now only GAUGE type of metric is supported return aggregationList.stream() .filter(Predicate.not(QueryRequestUtil::isDateTimeFunction)) @@ -126,7 +126,7 @@ private boolean areAggregationsNotSupported(List aggregationList) { private boolean isFilterNotSupported(Filter filter) { if (filter.getChildFilterCount() > 0) { - // AND high level operator is supported + // Currently, `AND` high level composite operator is supported // later OR operator can be supported for same column if (filter.getOperator() != Operator.AND) { return true; @@ -137,12 +137,12 @@ private boolean isFilterNotSupported(Filter filter) { } } } else { - // filter rhs should be literal only + // rhs condition of filter should be literal only if (filter.getRhs().getValueCase() != ValueCase.LITERAL) { return true; } - // filter lhs should be column or simple attribute + // lhs condition of filter should be column or simple attribute if (!QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())) { return true; } diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java index 18c153bf..5f54311b 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java @@ -70,7 +70,7 @@ private List buildPromqlQueries( String tenantId, QueryRequest request, LinkedHashSet allSelections, - Duration queryDuration, + Duration duration, String timeFilterColumn) { List groupByList = getGroupByList(request); @@ -89,7 +89,7 @@ private List buildPromqlQueries( && !QueryRequestUtil.isDateTimeFunction(expression)) .map( functionExpression -> - mapToPromqlQuery(functionExpression, groupByList, filterList, queryDuration)) + mapToPromqlQuery(functionExpression, groupByList, filterList, duration)) .collect(Collectors.toUnmodifiableList()); } @@ -97,7 +97,7 @@ private String mapToPromqlQuery( Expression functionExpression, List groupByList, List filterList, - Duration queryDuration) { + Duration duration) { String functionName = prometheusFunctionConverter.mapToPrometheusFunctionName(functionExpression); MetricConfig metricConfig = getMetricConfigForFunction(functionExpression); @@ -106,7 +106,7 @@ private String mapToPromqlQuery( functionName, String.join(", ", groupByList), String.join(", ", filterList), - queryDuration.toMillis()); + duration.toMillis()); } private List getGroupByList(QueryRequest queryRequest) { @@ -120,9 +120,12 @@ private Duration getTimeSeriesPeriod(ExecutionContext executionContext) { return executionContext.getTimeSeriesPeriod().get(); } + /** + * Builds a promql query. example query `sum by (a1, a2) (sum_over_time(num_calls{a4="..", + * a5=".."}[xms]))` + */ private String buildQuery( String metricName, String function, String groupByList, String filter, long durationMillis) { - // sum by (a1, a2) (sum_over_time(num_calls{a4="..", a5=".."}[xms])) String template = "%s by (%s) (%s(%s{%s}[%sms]))"; return String.format( diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index c7704628..fd521188 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -97,13 +97,13 @@ void testTimeSeriesQueryWithGroupByWithMultipleAggregatesWithMultipleFilters() { // time filter is removed from the query String query1 = - "count by (service_name, api_name) (count_over_time(error_count{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[15000ms]))"; + "count by (service_name, api_name) (count_over_time(error_count{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[10ms]))"; String query2 = - "avg by (service_name, api_name) (avg_over_time(num_calls{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[15000ms]))"; + "avg by (service_name, api_name) (avg_over_time(num_calls{tenant_id=\"__default\", service_id=\"1|2|3\", service_name=~\"someregex\"}[10ms]))"; Assertions.assertTrue(promqlQuery.getQueries().contains(query1)); Assertions.assertTrue(promqlQuery.getQueries().contains(query2)); - Assertions.assertEquals(15000, promqlQuery.getPeriod().toMillis()); + Assertions.assertEquals(10, promqlQuery.getPeriod().toMillis()); } private QueryRequest buildMultipleGroupByMultipleAggQuery() { @@ -138,8 +138,8 @@ private QueryRequest buildMultipleGroupByMultipleAggQueryWithMultipleFilters() { createFunctionExpression("AVG", createColumnExpression("SERVICE.numCalls").build()); builder.addAggregation(avg); - Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100L); - Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200L); + Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100000L); + Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200000L); Filter inFilter = createInFilter("SERVICE.id", List.of("1", "2", "3")); Filter likeFilter = Filter.newBuilder() @@ -191,7 +191,7 @@ private QueryRequest buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndD builder.addGroupBy(createColumnExpression("SERVICE.name")); builder.addGroupBy(createColumnExpression("API.name")); - builder.addGroupBy(createTimeColumnGroupByExpression("SERVICE.startTime", "15:SECONDS")); + builder.addGroupBy(createTimeColumnGroupByExpression("SERVICE.startTime", "10:MILLISECONDS")); return builder.build(); } From 9b102f039176d7982e2cc16d556b0d869e230798 Mon Sep 17 00:00:00 2001 From: rish691 Date: Thu, 9 Dec 2021 18:57:55 +0530 Subject: [PATCH 31/31] update test --- .../service/prometheus/QueryRequestToPromqlConverterTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java index fd521188..c5e1ce1f 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java @@ -138,8 +138,8 @@ private QueryRequest buildMultipleGroupByMultipleAggQueryWithMultipleFilters() { createFunctionExpression("AVG", createColumnExpression("SERVICE.numCalls").build()); builder.addAggregation(avg); - Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100000L); - Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200000L); + Filter startTimeFilter = createTimeFilter("SERVICE.startTime", Operator.GT, 100L); + Filter endTimeFilter = createTimeFilter("SERVICE.startTime", Operator.LT, 200L); Filter inFilter = createInFilter("SERVICE.id", List.of("1", "2", "3")); Filter likeFilter = Filter.newBuilder()