Change query range response structure #1867

vmmusings · 2023-07-13T23:50:14Z

Description

This PR is for changing the response structure of query_range table function in Prometheus Connector.

BWC failures are unrelated. WIll get fixed with the below PRs
#1876
#1868

Request::
PUT http://localhost:9200/_plugins/_ppl

{
    "query" : "source = my_prometheus.query_range('prometheus_http_requests_total',1689281439,1689291439,14)"
}

Sample Old Response

{
    "schema": [
        {
            "name": "@timestamp",
            "type": "timestamp"
        },
        {
            "name": "@value",
            "type": "double"
        },
        {
            "name": "handler",
            "type": "string"
        },
        {
            "name": "code",
            "type": "string"
        },
        {
            "name": "instance",
            "type": "string"
        },
        {
            "name": "__name__",
            "type": "string"
        },
        {
            "name": "job",
            "type": "string"
        }
    ],
    "datarows": [
        [
            "2023-07-17 23:57:07",
            5.0,
            "/api/v1/query_range",
            "200",
            "localhost:9090",
            "prometheus_http_requests_total",
            "prometheus"
        ],
        [
            "2023-07-17 23:49:39",
            1.0,
            "/metrics",
            "200",
            "localhost:9090",
            "prometheus_http_requests_total",
            "prometheus"
        ]
    "total": 34,
    "size": 34
}

There is no segregation of data among different timeseries in a metric.

In the new response below...each timeseries of a metric is a row individually.

Response::

{
    "schema": [
        {
            "name": "@timestamp",
            "type": "array"
        },
        {
            "name": "@value",
            "type": "array"
        },
        {
            "name": "@labels",
            "type": "struct"
        }
    ],
    "datarows": [
        [
            {
                "handler": "/api/v1/query_range",
                "code": "200",
                "instance": "localhost:9090",
                "__name__": "prometheus_http_requests_total",
                "job": "prometheus"
            },
            [
                "2023-07-13 23:37:15"
            ],
            [
                3.0
            ]
        ],
        [
            {
                "handler": "/metrics",
                "code": "200",
                "instance": "localhost:9090",
                "__name__": "prometheus_http_requests_total",
                "job": "prometheus"
            },
            [
                "2023-07-13 23:33:17",
                "2023-07-13 23:33:31",
                "2023-07-13 23:33:45",
                "2023-07-13 23:33:59",
                "2023-07-13 23:34:13",
                "2023-07-13 23:34:27",
                "2023-07-13 23:34:41",
                "2023-07-13 23:34:55",
                "2023-07-13 23:35:09",
                "2023-07-13 23:35:23",
                "2023-07-13 23:35:37",
                "2023-07-13 23:35:51",
                "2023-07-13 23:36:05",
                "2023-07-13 23:36:19",
                "2023-07-13 23:36:33",
                "2023-07-13 23:36:47",
                "2023-07-13 23:37:01",
                "2023-07-13 23:37:15"
            ],
            [
                1.0,
                2.0,
                3.0,
                4.0,
                5.0,
                6.0,
                7.0,
                7.0,
                8.0,
                9.0,
                10.0,
                11.0,
                12.0,
                13.0,
                14.0,
                15.0,
                16.0,
                17.0
            ]
        ]
    ],
    "total": 2,
    "size": 2
}

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2023-07-13T23:56:07Z

Codecov Report

Merging #1867 (b1e2687) into main (8c8e08c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1867      +/-   ##
============================================
- Coverage     97.39%   97.39%   -0.01%     
+ Complexity     4608     4603       -5     
============================================
  Files           401      401              
  Lines         11408    11397      -11     
  Branches        843      835       -8     
============================================
- Hits          11111    11100      -11     
  Misses          290      290              
  Partials          7        7

Flag	Coverage Δ
sql-engine	`97.39% <100.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
.../sql/prometheus/storage/PrometheusMetricTable.java	`100.00% <ø> (ø)`
...ponse/DefaultQueryRangeFunctionResponseHandle.java	`100.00% <100.00%> (ø)`
...ch/sql/prometheus/response/PrometheusResponse.java	`100.00% <100.00%> (ø)`
...h/sql/prometheus/storage/PrometheusMetricScan.java	`100.00% <100.00%> (ø)`
...rage/implementor/PrometheusDefaultImplementor.java	`100.00% <100.00%> (ø)`

dai-chen · 2023-07-17T23:27:26Z

There is only IT for explain? Could you add to your PR description the response format before/after your changes?

vmmusings · 2023-07-18T00:01:56Z

There is only IT for explain? Could you add to your PR description the response format before/after your changes?
@dai-chen Added old response. Its hard to verify the row response..but will add IT for atleast checking schema output.

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>

dai-chen · 2023-07-18T21:27:23Z

prometheus/src/main/java/org/opensearch/sql/prometheus/storage/PrometheusMetricTable.java

@@ -97,10 +97,6 @@ public Map<String, ExprType> getFieldTypes() {
  public PhysicalPlan implement(LogicalPlan plan) {
    PrometheusMetricScan metricScan =
        new PrometheusMetricScan(prometheusClient);
-    if (prometheusQueryRequest != null) {
-      metricScan.setRequest(prometheusQueryRequest);


I see request field is still in use in metric scan. So where we set it now?

It is set in https://github.com/opensearch-project/sql/blob/main/prometheus/src/main/java/org/opensearch/sql/prometheus/storage/implementor/PrometheusDefaultImplementor.java#L109 here

Implementation of query_range and ppl.prometheus_http_requests_total got coupled with weird logic. Need to refactor in another PR.

query_range...we directly set the prometheusQueryRequest.
In case of source = my_prometheus.prometheus_http_requests_total we set these parameters in PrometheusDefaultImplementator class.

dai-chen · 2023-07-18T21:31:17Z

...ain/java/org/opensearch/sql/prometheus/storage/implementor/PrometheusDefaultImplementor.java

@@ -94,14 +94,12 @@ public PhysicalPlan visitIndexAggregation(PrometheusLogicalMetricAgg node,
  public PhysicalPlan visitRelation(LogicalRelation node,
                                    PrometheusMetricScan context) {
    PrometheusMetricTable prometheusMetricTable = (PrometheusMetricTable) node.getTable();
-    if (prometheusMetricTable.getMetricName() != null) {


Just my feel that it's a little hard to follow why several if check removed in this PR. it's fine if it's covered in UT.

In earlier implementation, query_range function implementation got coupled with normal PPL queries source = my_prometheus.proemtheus_http_requests_total implementation. This PR at least decouples few of the things.

Need to refactor the existing implementation. I tried to refactor in this PR, the changes are becoming huge. Will cover them in another PR.

With the new implementation. Visit Relation will always have metricName.

I see. Please add TODO comment or put all refactor items in Github issue. Thanks!

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58)

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58)

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58) Co-authored-by: Vamsi Manohar <reddyvam@amazon.com>

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> Signed-off-by: Mitchell Gale <Mitchell.Gale@improving.com>

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

vmmusings force-pushed the change-query-range-response branch 4 times, most recently from 77b6bb4 to d3eb1a2 Compare July 17, 2023 20:35

vmmusings self-assigned this Jul 17, 2023

vmmusings added the backport 2.x label Jul 17, 2023

vmmusings marked this pull request as ready for review July 17, 2023 22:38

vmmusings requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners July 17, 2023 22:38

pjfitzgibbons previously approved these changes Jul 17, 2023

View reviewed changes

vmmusings dismissed pjfitzgibbons’s stale review via 716df8e July 18, 2023 00:14

vmmusings force-pushed the change-query-range-response branch 2 times, most recently from 716df8e to 8f04562 Compare July 18, 2023 00:41

Change query range response structure

b1e2687

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>

vmmusings force-pushed the change-query-range-response branch from 8f04562 to b1e2687 Compare July 18, 2023 00:41

dai-chen reviewed Jul 18, 2023

View reviewed changes

dai-chen approved these changes Jul 19, 2023

View reviewed changes

rupal-bq approved these changes Jul 19, 2023

View reviewed changes

vmmusings merged commit 4102b58 into opensearch-project:main Jul 20, 2023

opensearch-trigger-bot bot mentioned this pull request Jul 20, 2023

[Backport 2.x] Change query range response structure #1890

Merged

opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 20, 2023

Change query range response structure (#1867)

ee760aa

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58)

matthewryanwells pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jul 20, 2023

Change query range response structure (opensearch-project#1867)

051120a

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com>

vmmusings added a commit that referenced this pull request Jul 26, 2023

Change query range response structure (#1867)

0f57f30

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58)

vmmusings added a commit that referenced this pull request Jul 27, 2023

Change query range response structure (#1867) (#1890)

4d7a02c

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> (cherry picked from commit 4102b58) Co-authored-by: Vamsi Manohar <reddyvam@amazon.com>

MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jul 28, 2023

Change query range response structure (opensearch-project#1867)

c403776

Signed-off-by: Vamsi Manohar <reddyvam@amazon.com> Signed-off-by: Mitchell Gale <Mitchell.Gale@improving.com>

vmmusings added the backport 2.9-enhanced label Sep 27, 2023

opensearch-trigger-bot bot mentioned this pull request Sep 27, 2023

[Backport 2.9-enhanced] Change query range response structure #2164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change query range response structure #1867

Change query range response structure #1867

vmmusings commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading

dai-chen commented Jul 17, 2023

vmmusings commented Jul 18, 2023

dai-chen Jul 18, 2023

vmmusings Jul 18, 2023

vmmusings Jul 18, 2023

dai-chen Jul 18, 2023

vmmusings Jul 18, 2023

vmmusings Jul 18, 2023

dai-chen Jul 19, 2023

vmmusings Jul 19, 2023

Change query range response structure #1867

Change query range response structure #1867

Conversation

vmmusings commented Jul 13, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Jul 13, 2023 • edited Loading

Codecov Report

dai-chen commented Jul 17, 2023

vmmusings commented Jul 18, 2023

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

vmmusings commented Jul 13, 2023 •

edited

Loading

codecov bot commented Jul 13, 2023 •

edited

Loading