Skip to content

feat: adds support for building converter for query service response builder from PromQL response#113

Merged
kotharironak merged 39 commits intomainfrom
handle-resource-builder
Dec 13, 2021
Merged

feat: adds support for building converter for query service response builder from PromQL response#113
kotharironak merged 39 commits intomainfrom
handle-resource-builder

Conversation

@kotharironak
Copy link
Copy Markdown
Contributor

@kotharironak kotharironak commented Dec 9, 2021

This PR is for handling tickets - hypertrace/hypertrace#326

This PR converts the PormQL response structure of instant query and range query to Query
Ref:
Instant Query Response: https://prometheus.io/docs/prometheus/latest/querying/api/#instant-queries
Range Query Response: https://prometheus.io/docs/prometheus/latest/querying/api/#range-queries

QS Response: https://github.com/hypertrace/query-service/blob/main/query-service-api/src/main/proto/response.proto#L22

In this PR, Prepare the Row structure of Query Service Response.

As for a single QS request, it can result in multiple PromQL metric query requests. However, the response of each PromQL query request only differs on Metric name and value, the rest of the argument for attributes are the same.
So, the approach here is that we prepare the rest of the attributes (for QS Row) from one of the PromQL responses, and for metric values columns, we create columns from the individual response.

As an example:

filter {
  childFilter {
    childFilter {
      lhs {
        columnIdentifier {
          columnName: "BACKEND.startTime"
        }
      }
      operator: GE
      rhs {
        literal {
          value {
            valueType: LONG
            long: 1631177908474
          }
        }
      }
    }
    childFilter {
      lhs {
        columnIdentifier {
          columnName: "BACKEND.startTime"
        }
      }
      operator: LT
      rhs {
        literal {
          value {
            valueType: LONG
            long: 1631181508474
          }
        }
      }
    }
  }
}
selection {
  columnIdentifier {
    columnName: "BACKEND.id"
  }
}
selection {
  function {
    functionName: "SUM"
    arguments {
      columnIdentifier {
        columnName: "BACKEND.errorCount"
      }
    }
    alias: "SUM_BACKEND.errorCount_[]"
  }
}
selection {
  function {
    functionName: "SUM"
    arguments {
      columnIdentifier {
        columnName: "BACKEND.numCalls"
      }
    }
    alias: "SUM_BACKEND.numCalls_[]"
  }
}
groupBy {
  columnIdentifier {
    columnName: "BACKEND.id"
  }
}
limit: 10000

Corresponding PromQL queries:

sum by (backend_id) 
(sum_over_time(num_calls{tenant_id="__default"}[3600000ms])) 
@ 1631181508474

sum by (backend_id) 
(sum_over_time(error_count{tenant_id="__default"'}[3600000ms])) 
@ 1631181508474

So, for the above example, while preparing row,

  • backend_id information is collected form one of the response
  • and num_call and error_count for the matching backend_id will be collected from the individual responses.

kotharironak and others added 28 commits November 24, 2021 20:05
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 9, 2021

Codecov Report

Merging #113 (1d17c26) into main (73dc9aa) will increase coverage by 0.32%.
The diff coverage is 89.06%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #113      +/-   ##
============================================
+ Coverage     78.60%   78.92%   +0.32%     
- Complexity      508      524      +16     
============================================
  Files            50       51       +1     
  Lines          2000     2064      +64     
  Branches        216      221       +5     
============================================
+ Hits           1572     1629      +57     
- Misses          333      337       +4     
- Partials         95       98       +3     
Flag Coverage Δ
integration 78.92% <89.06%> (+0.32%) ⬆️
unit 76.93% <89.06%> (+0.40%) ⬆️

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

Impacted Files Coverage Δ
...ice/prometheus/PrometheusBasedResponseBuilder.java 89.06% <89.06%> (ø)

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 73dc9aa...1d17c26. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kotharironak
Copy link
Copy Markdown
Contributor Author

@rish691 added the test. It returns the result both for instant query and range query. It is also able to merge multiple metrics. However, need to optimize, but you can work on top of it.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kotharironak kotharironak marked this pull request as ready for review December 13, 2021 05:13
@kotharironak kotharironak requested a review from a team December 13, 2021 05:13
* */
static List<Row> buildResponse(
Map<Request, PromQLMetricResponse> promQLMetricResponseMap,
Map<String, String> columnNameToAttributeMap,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we rename the param names?

Map<String, String> logicalAttributeNameToMetricAttributeMap,
Map<String, String> logicalAttributeNameToMetricQueryMap,
List<String> columnSelectionSet,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. All remove some of the unused code.

@kotharironak
Copy link
Copy Markdown
Contributor Author

Integration tests pass locally but fail on GHA.
Screenshot 2021-12-13 at 4 42 03 PM

@github-actions

This comment has been minimized.


kafkaZk =
new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka"))
new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka:7.0.0"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lets add a todo here to use latest version again

@kotharironak kotharironak merged commit cc6be15 into main Dec 13, 2021
@kotharironak kotharironak deleted the handle-resource-builder branch December 13, 2021 11:26
@github-actions
Copy link
Copy Markdown

Unit Test Results

  29 files  +1    29 suites  +1   9s ⏱️ ±0s
156 tests +4  156 ✔️ +4  0 💤 ±0  0 ❌ ±0 

Results for commit cc6be15. ± Comparison against base commit 73dc9aa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants