feat: Add promql query converter and request handler #111

findingrish · 2021-11-30T05:07:49Z

This pr adds support for translating ht query request to promql, defines a prometheus view definition and request handler

Not every query can be served by prometheus, there are set of conditions under which a query maybe served by prometheus.

First a note on how data is stored in prometheus.

Currently num_calls metric time series is stored in prometheus with following attributes
num_calls(tenant_id, service_id, service_name, api_id, api_name)
This is a subset of the data from rawServiceView table
In prometheus every metric will be an individual time series and there are some attributes on it for aggregations

The PrometheusViewDefinition (or PrometheusMetricDefinition) defines the attributeMapping and the metricMapping.

There are two types of prometheus query, instant and timeseries, any ht query request with dateTimeConvert function is timeseries request
Conditions for a query to be served by prometheus:

Aggregation selections: It must be an aggregation query, the function selection must be on metric column and those metric must be present in prometheus (and thus the view definition).
Normal selection and groupby should be on same columns (groupby on dateTimeConvert can be skipped)
Filter must be on simple columns (column or attribute only) and it should only have and operator (prometheus only allows simple filter applied anded, https://prometheus.io/docs/prometheus/latest/querying/basics/#instant-vector-selectors)
Time filter are part of the api request to prometheus are skipped from the query string
orderby is not supported now

Conversion example:

Select service_name, api_name, COUNT(errorCount), AVG(num_calls) FROM ServiceView WHERE tenant_id = '__default' AND ( start_time_millis > 100 AND end_time_millis < 200 AND service_id IN ('1', '2', '3') AND REGEXP_LIKE(service_name,'someregex') ) GROUP BY service_name, span_name

translates to following prometheus queries

1. "count by (service_name, api_name) (count_over_time(error_count{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"
2. "avg by (service_name, api_name) (avg_over_time(num_calls{service_id=\"1|2|3\", service_name=~\"someregex\"}[100ms]))"

Rules for conversion:

Each aggregation function results in a query, so number of queries equals number of aggregation function requested
the column in the function expression gives the metric series to be queried
the function name is translated to the prometheus function
the group by list makes the by clause of the query
all the filters are put together comma separated inside curly brace
the duration of the query is appended in square brackets

hypertrace/hypertrace#324, hypertrace/hypertrace#320

...pl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java

codecov · 2021-12-03T07:46:13Z

Codecov Report

Merging #111 (9b102f0) into main (2e9b4ec) will decrease coverage by 3.91%.
The diff coverage is 55.78%.

@@             Coverage Diff              @@
##               main     #111      +/-   ##
============================================
- Coverage     82.51%   78.60%   -3.92%     
- Complexity      456      508      +52     
============================================
  Files            41       50       +9     
  Lines          1716     2000     +284     
  Branches        180      216      +36     
============================================
+ Hits           1416     1572     +156     
- Misses          215      333     +118     
- Partials         85       95      +10

Flag	Coverage Δ
integration	`78.60% <55.78%> (-3.92%)`	⬇️
unit	`76.53% <54.42%> (-3.95%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...service/pinot/QueryRequestToPinotSQLConverter.java	`86.80% <ø> (+0.68%)`	⬆️
...vice/prometheus/PrometheusBasedRequestHandler.java	`0.00% <0.00%> (ø)`
...e/prometheus/QueryRequestEligibilityValidator.java	`13.43% <13.43%> (ø)`
...ce/prometheus/PrometheusRequestHandlerBuilder.java	`30.00% <30.00%> (ø)`
...ypertrace/core/query/service/QueryRequestUtil.java	`52.08% <36.36%> (-4.68%)`	⬇️
...ervice/prometheus/PrometheusFunctionConverter.java	`50.00% <50.00%> (ø)`
...ry/service/prometheus/FilterToPromqlConverter.java	`50.98% <50.98%> (ø)`
.../hypertrace/core/query/service/QueryTimeRange.java	`80.00% <80.00%> (ø)`
...y/service/prometheus/PrometheusViewDefinition.java	`92.30% <92.30%> (ø)`
...ypertrace/core/query/service/ExecutionContext.java	`94.36% <100.00%> (+0.12%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e9b4ec...9b102f0. Read the comment docs.

query-service-impl/src/test/resources/prometheus_request_handler.conf

kotharironak · 2021-12-03T11:07:59Z

query-service-impl/src/test/resources/prometheus_request_handler.conf

+    tenantAttributeName = tenant_id
+    prometheusViewDefinition {
+      viewName = rawServiceView
+      metricMap {


each metric map entry, can we define something like

attribute: API.numCalls metric : { metricName: "num_calls", metricType: "GAUGE" }

Can you log a ticket for this so we can change this structure to more readable and align with SERVICE.numCall attribute pattern?

aaron-steinfeld

I thought the idea was to introduce a new API? Has that been done? Why did we choose to build from the bottom up - it makes it far easier to have to rework things when issues come up on integration at higher layers.

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java

sarthak77 · 2021-12-09T07:21:59Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java

+    return executionContext.getTimeSeriesPeriod().get();
+  }
+
+  private String buildQuery(


Do we need this function as it is just returning the string from the arguments ?

Some logic need to go in here (in the upcoming changes)

sarthak77 · 2021-12-09T07:28:02Z

.../java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidatorTest.java

+  @Test
+  void testCalculateCost_aggregationNotSupported() {
+    Builder builder = QueryRequest.newBuilder();
+    builder.addAggregation(


Use createCountByColumnSelection method ?

sarthak77 · 2021-12-09T07:53:33Z

...est/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java

+  private QueryRequest buildMultipleGroupByMultipleAggQueryWithMultipleFiltersAndDateTime() {
+    Builder builder = QueryRequest.newBuilder();
+    builder.addAggregation(
+        createFunctionExpression("Count", createColumnExpression("SERVICE.errorCount").build()));


There is a separate function for count

kotharironak · 2021-12-09T08:30:14Z

query-service-impl/src/test/resources/prometheus_request_handler.conf

+{
+  name = raw-service-view-service-prometheus-handler
+  type = prometheus
+  clientConfig = ""


We need to hook host and port property for this in follow-up hook PR.

kotharironak · 2021-12-09T08:33:56Z

query-service-impl/src/test/resources/prometheus_request_handler.conf

+    tenantAttributeName = tenant_id
+    prometheusViewDefinition {
+      viewName = rawServiceView
+      metricMap {


Can you log a ticket for this so we can change this structure to more readable and align with SERVICE.numCall attribute pattern?

kotharironak · 2021-12-09T08:34:51Z

query-service-impl/src/test/resources/prometheus_request_handler.conf

+          metricName: "num_calls",
+          metricType: "GAUGE"
+        },
+        errorCount {


we only have num_calls emitter for now. So, can you remove this? We will add it later when we add error_count emitter.

kotharironak · 2021-12-09T08:35:46Z

query-service-impl/src/test/resources/prometheus_request_handler.conf

+        "EVENT.protocolName": "protocol_name",
+        "EVENT.status_code": "status_code",
+        "API_TRACE.status_code": "status_code",
+        "API.startTime": "start_time_millis",


Can we remove this "API.startTime": "start_time_millis", "API.endTime": "end_time_millis",?

kotharironak · 2021-12-09T08:42:08Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/PrometheusBasedRequestHandler.java

+
+public class PrometheusBasedRequestHandler implements RequestHandler {
+
+  public static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition";


public -> private

kotharironak · 2021-12-09T08:47:51Z

...mpl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java

+  private final String viewName;
+  private final String tenantColumnName;
+  private final Map<String, MetricConfig> metricMap;
+  private final Map<String, String> columnMap;


Can we rename this to attributeMap?

kotharironak · 2021-12-09T08:48:43Z

...mpl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java

+      Map<String, MetricConfig> metricMap,
+      Map<String, String> columnMap) {
+    this.viewName = viewName;
+    this.tenantColumnName = tenantColumnName; // tenantAttributeName


tenantColumnName -> tenantAttributeName ?

kotharironak · 2021-12-09T08:51:06Z

...mpl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java

+  @Value
+  @AllArgsConstructor
+  public static class MetricConfig {
+    String name;


name -> metricName ?

kotharironak · 2021-12-09T08:52:58Z

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java

+
+  QueryCost calculateCost(QueryRequest queryRequest, ExecutionContext executionContext) {
+    try {
+      // orderBy to be supported later


Can you create a ticket for this in same epic?

kotharironak · 2021-12-09T09:03:28Z

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java

+                return true;
+              }
+              return prometheusViewDefinition.getMetricConfigForLogicalMetricName(attributeId)
+                  == null;


Can we put a comment as general (e.g only single arugment functions are supported on metrics columns only)?

kotharironak · 2021-12-09T09:07:55Z

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java

+    }
+
+    // filter lhs should be column or simple attribute
+    if (!QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())) {


Can e check this as leaf filter condition?

kotharironak · 2021-12-09T09:08:20Z

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java

+      return true;
+    }
+
+    // todo check for valid operators here


Can you remove this comment?

kotharironak · 2021-12-09T09:11:19Z

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java

+                  == null;
+            });
+  }
+


Lets have a comment here that currently only AND filters supported. Later this can be extended for OR filter on same column (attributeId) expression.

kotharironak · 2021-12-09T10:19:26Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java

+      LinkedHashSet<Expression> allSelections,
+      QueryTimeRange queryTimeRange,
+      String timeFilterColumn) {
+    List<String> groupByList = getGroupByList(request);


You will have to add tenant filter as default filter? Hopefully, tenant filter is already present in QueryReqeust?

yes added tenant filter, updated test

kotharironak · 2021-12-09T10:23:37Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java

+
+    // 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()


allSelection = groupBy selection list + function selection list, right?

kotharironak · 2021-12-09T10:27:11Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java

+        template,
+        function,
+        groupByList,
+        function + "_over_time", // assuming gauge type of metric


You have metricConfig with type (metricName, metricType), can we through the exception while preparing metricConfig itself that only supported type is Gauge for now?

kotharironak · 2021-12-09T10:27:13Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java

+        "%s by (%s) (%s(%s{%s}[%sms]))"; // sum by (a1, a2) (sum_over_time(num_calls{a4="..",
+    // a5=".."}[xms]))


Can you put this example // sum by (a1, a2) (sum_over_time(num_calls{a4="..", // a5=".."}[xms])) above function?

kotharironak · 2021-12-09T10:28:30Z

...rc/main/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverter.java

+  }
+
+  private MetricConfig getMetricConfigForFunction(Expression functionSelection) {
+    return prometheusViewDefinition.getMetricConfigForLogicalMetricName(


If, you want here also we can check the type of metrics for gauge

Checking the type in QueryRequestEligibilityValidator

kotharironak · 2021-12-09T10:34:49Z

...impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java

+   */
+  void convertFilterToString(
+      Filter filter,
+      List<String> filterList,


Looks like filterList is output? nit : can this be last argument? Also can we put comments for args?

yes, made it last argument

kotharironak · 2021-12-09T10:35:55Z

...impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java

+            childFilter, filterList, timeFilterColumn, expressionToColumnConverter);
+      }
+    } else {
+      if (QueryRequestUtil.isSimpleColumnExpression(filter.getLhs())


isSimpleColumnExpression do we need to check again? Isn't this already check in selection process?

kotharironak · 2021-12-09T10:48:55Z

...impl/src/main/java/org/hypertrace/core/query/service/prometheus/FilterToPromqlConverter.java

+      StringBuilder builder = new StringBuilder();
+      builder.append(expressionToColumnConverter.apply(filter.getLhs()));
+      builder.append(convertOperatorToString(filter.getOperator()));
+      builder.append(convertLiteralToString(filter.getRhs().getLiteral()));


What will the null value convertLiteralToString mean here? I haven't try such query? Instead of null, would it make to have "" filter or no filter at all?

Throwing exception from convertLiteralToString

kotharironak · 2021-12-09T10:56:58Z

...est/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java

+            .build();
+    builder.setFilter(andFilter);
+
+    builder.addGroupBy(createColumnExpression("SERVICE.name"));


Shouldn't the selection expression required?

kotharironak · 2021-12-09T10:59:18Z

...est/java/org/hypertrace/core/query/service/prometheus/QueryRequestToPromqlConverterTest.java

+
+    // 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]))";


I think, the value 100ms of expression [], we may have to change to value of time series period. we can talk on this.

kotharironak

overall lgtm.

kotharironak · 2021-12-09T13:35:18Z

...mpl/src/main/java/org/hypertrace/core/query/service/prometheus/PrometheusViewDefinition.java

+
+  public enum MetricType {
+    GAUGE,
+    COUNTER


In follow up remove the COUNTER for now.

github-actions · 2021-12-09T13:42:23Z

Unit Test Results

  28 files +2   28 suites +2 8s ⏱️ -2s
152 tests +6 152 ✔️ +6 0 💤 ±0 0 ❌ ±0

Results for commit 74a83c6. ± Comparison against base commit 2e9b4ec.

findingrish · 2021-12-09T13:56:29Z

Filed following backlogs: #112, #114, #115, #116, #117, #118

Add promql query converter

f99e764

kotharironak reviewed Nov 30, 2021

View reviewed changes

...pl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java Outdated Show resolved Hide resolved

kotharironak reviewed Nov 30, 2021

View reviewed changes

...pl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java Outdated Show resolved Hide resolved

kotharironak reviewed Nov 30, 2021

View reviewed changes

...pl/src/main/java/org/hypertrace/core/query/service/promql/QueryRequestToPromqlConverter.java Outdated Show resolved Hide resolved

rish691 added 11 commits November 30, 2021 13:08

Update package name

d697090

spotless

330c893

Add handler and prometheus view def

97ae470

Spotless

69c5ea7

Add test for promql converter

a683ce9

spotless

45f3cd4

remove prom config from main

870ed97

Add test

b608e80

fix test

d3d5a76

minor change

ff36a9e

fixed test

a3895ec

This comment has been minimized.

Sign in to view

Update

0576214

findingrish changed the title ~~feat: Add promql query converter~~ feat: Add promql query converter and request handler Dec 3, 2021

This comment has been minimized.

Sign in to view

Minor change

90d9599

This comment has been minimized.

Sign in to view

spotless

7197850

This comment has been minimized.

Sign in to view

kotharironak reviewed Dec 3, 2021

View reviewed changes

findingrish marked this pull request as ready for review December 3, 2021 18:44

findingrish requested review from a team, sarthak77 and aaron-steinfeld December 3, 2021 18:44

aaron-steinfeld reviewed Dec 3, 2021

View reviewed changes

sarthak77 reviewed Dec 9, 2021

View reviewed changes

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java Outdated Show resolved Hide resolved

sarthak77 reviewed Dec 9, 2021

View reviewed changes

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java Show resolved Hide resolved

sarthak77 reviewed Dec 9, 2021

View reviewed changes

...main/java/org/hypertrace/core/query/service/prometheus/QueryRequestEligibilityValidator.java Show resolved Hide resolved

sarthak77 reviewed Dec 9, 2021

View reviewed changes

Minor changes

90e9103

sarthak77 reviewed Dec 9, 2021

View reviewed changes

spotless

eee7e1a

This comment has been minimized.

Sign in to view

sarthak77 reviewed Dec 9, 2021

View reviewed changes

kotharironak reviewed Dec 9, 2021

View reviewed changes

Address review comment

1400d73

This comment has been minimized.

Sign in to view

rish691 added 2 commits December 9, 2021 18:55

update test

c108ae5

update test

9b102f0

This comment has been minimized.

Sign in to view

kotharironak approved these changes Dec 9, 2021

View reviewed changes

findingrish merged commit 74a83c6 into main Dec 9, 2021

findingrish deleted the metrics-dev branch December 9, 2021 13:38

This was referenced Dec 9, 2021

Update metricMap config in Prometheus View Definition #116

Open

Conditional support for composite filter with OR operator in Prometheus Request Handler #117

Open


		public class PrometheusBasedRequestHandler implements RequestHandler {

		public static final String VIEW_DEFINITION_CONFIG_KEY = "prometheusViewDefinition";

		"%s by (%s) (%s(%s{%s}[%sms]))"; // sum by (a1, a2) (sum_over_time(num_calls{a4="..",
		// a5=".."}[xms]))

feat: Add promql query converter and request handler #111

feat: Add promql query converter and request handler #111

Conversation

findingrish commented Nov 30, 2021 • edited Loading

codecov bot commented Dec 3, 2021 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

kotharironak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 9, 2021

Unit Test Results

findingrish commented Dec 9, 2021 • edited Loading

findingrish commented Nov 30, 2021 •

edited

Loading

codecov bot commented Dec 3, 2021 •

edited

Loading

findingrish commented Dec 9, 2021 •

edited

Loading