Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: export promql service in server #924

Merged
merged 12 commits into from
Feb 3, 2023

Conversation

waynexia
Copy link
Member

@waynexia waynexia commented Feb 1, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Add a promql service to serve PromQL queries. Currently only query and range_query are defined, and only range_query returns mock results.

This PR also includes some minor changes in a separated commit 26d0918

For the name of this new service, I would prefer to rename it to prometheus as it's a subset of prometheus HTTP API, and can be extended to implement other methods that are not included in this PR. So I propose to rename the current prometheus service to prom_store, which implements Prometheus' remote storage APIs, and rename the new service from promql to prometheus. What do you think about this @killme2008

The code structure may need some adjustments. Specificly for PromqlHandler and PromqlServer. Do I need to put them under the HTTP server or other places? cc @MichaelScofield PTAL at this and looking forward to hearing your advice.

An important missing part of this functionality is the codec of query results. We are using table model all around, it looks impossible to return multi-value columns in prometheus' format. Maybe we need to introduce some hack like built-in tag __value__. And beside this, converting should also consider the semantic type in the result's schema. This needs to change the planner and maybe the executor, so I plan to implement codec in the follow-up PR(s).

I'm also wondering if it's beneficial to provide other ways to perform promql queries, like via SDK or special SQL.

Things left to do in this PR:

  • split PromqlServer into serval sub-modules.
  • add test-integration case for this new service.

Some screenshots

  • data
    mysql> desc table demo;
    +-----------+----------------------+------+---------+---------------+
    | Field     | Type                 | Null | Default | Semantic Type |
    +-----------+----------------------+------+---------+---------------+
    | timestamp | TimestampMillisecond | NO   |         | TIME INDEX    |
    | host      | String               | YES  |         | PRIMARY KEY   |
    | value     | Float64              | YES  |         | VALUE         |
    +-----------+----------------------+------+---------+---------------+
    mysql> select * from demo;
    +-------+---------+---------------------+
    | host  | value   | timestamp           |
    +-------+---------+---------------------+
    | host1 |    66.6 | 1970-01-01 00:00:00 |
    | host1 |    66.6 | 1970-01-01 00:00:02 |
    | host1 |    66.6 | 1970-01-01 00:00:05 |
    | host1 |    43.1 | 1970-01-01 00:00:07 |
    | host1 |    19.1 | 1970-01-01 00:00:09 |
    | host1 |    99.1 | 1970-01-01 00:00:10 |
    | host1 |   999.9 | 1970-01-01 00:00:21 |
    | host1 |    31.9 | 1970-01-01 00:00:22 |
    | host1 |    95.4 | 1970-01-01 00:00:32 |
    | host1 | 12423.1 | 1970-01-01 00:00:49 |
    | host1 |       0 | 1970-01-01 00:01:20 |
    | host1 |      49 | 1970-01-01 00:01:39 |
    +-------+---------+---------------------+
  • query result:
    2023-02-01T09:06:07.477917Z  INFO servers::promql: row: [Timestamp(Timestamp { value: 0, unit: Millisecond }), Float64(OrderedFloat(67.0))]
    2023-02-01T09:06:07.477960Z  INFO servers::promql: row: [Timestamp(Timestamp { value: 5000, unit: Millisecond }), Float64(OrderedFloat(67.0))]
    2023-02-01T09:06:07.477988Z  INFO servers::promql: row: [Timestamp(Timestamp { value: 10000, unit: Millisecond }), Float64(OrderedFloat(100.0))]
    2023-02-01T09:06:07.478016Z  INFO servers::promql: row: [Timestamp(Timestamp { value: 50000, unit: Millisecond }), Float64(OrderedFloat(12424.0))]
    2023-02-01T09:06:07.478044Z  INFO servers::promql: row: [Timestamp(Timestamp { value: 80000, unit: Millisecond }), Float64(OrderedFloat(0.0))]
    2023-02-01T09:06:07.478070Z  INFO servers::promql: row: [Timestamp(Timestamp { value: 100000, unit: Millisecond }), Float64(OrderedFloat(49.0))]
    
  • mocked return value
    image

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#596

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia waynexia mentioned this pull request Feb 1, 2023
26 tasks
@waynexia
Copy link
Member Author

waynexia commented Feb 1, 2023

I'm also wondering if it's beneficial to provide other ways to perform promql queries, like via SDK or special SQL.

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #924 (2fc0fd9) into develop (af93567) will decrease coverage by 0.30%.
The diff coverage is 44.06%.

❗ Current head 2fc0fd9 differs from pull request most recent head c67d85a. Consider uploading reports for the commit c67d85a to get more accurate results

@@             Coverage Diff             @@
##           develop     #924      +/-   ##
===========================================
- Coverage    86.04%   85.74%   -0.30%     
===========================================
  Files          439      441       +2     
  Lines        58653    58941     +288     
===========================================
+ Hits         50469    50541      +72     
- Misses        8184     8400     +216     
Flag Coverage Δ
rust 85.74% <44.06%> (-0.30%) ⬇️

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

Impacted Files Coverage Δ
src/frontend/src/error.rs 11.32% <0.00%> (-0.22%) ⬇️
src/frontend/src/instance/distributed.rs 89.33% <0.00%> (-1.11%) ⬇️
src/frontend/src/instance/standalone.rs 69.04% <0.00%> (-13.81%) ⬇️
src/frontend/src/lib.rs 100.00% <ø> (ø)
src/frontend/src/server.rs 0.00% <0.00%> (ø)
src/servers/src/lib.rs 100.00% <ø> (ø)
src/servers/src/query_handler/sql.rs 41.30% <0.00%> (-19.99%) ⬇️
src/servers/src/http/handler.rs 47.82% <4.76%> (-18.85%) ⬇️
src/frontend/src/instance.rs 69.61% <26.08%> (-1.93%) ⬇️
src/datanode/src/instance/sql.rs 76.06% <37.50%> (-2.84%) ⬇️
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@killme2008
Copy link
Contributor

For the name of this new service, I would prefer to rename it to prometheus as it's a subset of [prometheus HTTP API]> > > (https://prometheus.io/docs/prometheus/latest/querying/api/), and can be extended to implement other methods that are > > not included in this PR. So I propose to rename the current prometheus service to prom_store, which implements > > > Prometheus' remote storage APIs, and rename the new service from promql to prometheus. What do you think about this @killme2008

What about /v1/prometheus/query?I think all prometheus related APIs must be under /prometheus path.

@killme2008
Copy link
Contributor

I'm also wondering if it's beneficial to provide other ways to perform promql queries, like via SDK or special SQL.

Of course, it's better to provide other ways to perform PromQL queries, especially SQL and our gRPC SDK.

@MichaelScofield
Copy link
Collaborator

Seems promql is only executed via http? In that case, I suggest not using PromqlServer at all, integrate PromqlHandler into struct HttpServer, just like other protocol handlers.

By the way, I think we can get rid of PromqlHandler as well, make executing promql as a method of SqlQueryHandler, then rename SqlQueryHandler to QueryLangHandler (maybe other better names). Because promql is translated to logical plan to be actually executed, I think we can unify sql and promql execution path somehow. I've came up with similar thoughts in #766 , if we could make standalone and distribute instance both execute logical plan directly. In that way, promql could be executed in distribute mode, too.

@waynexia
Copy link
Member Author

waynexia commented Feb 2, 2023

After discussing with @killme2008, we think it's better to provide promql query through two ways:

  • via prometheus' official HTTP API, which is partially implemented in this PR
  • via our HTTP services, using a new path /v1/promql, similar to /v1/sql

Implement prometheus' official API can make GreptimeDB a drop-in replacement for Prometheus. Existing systems like grafana only need to change the server address/port to migrate. While/v1/promql is providing the promql ability to query GreptimeDB, which is more lightweight to use and flexible to implement.

src/frontend/src/server.rs Show resolved Hide resolved
src/servers/src/promql.rs Show resolved Hide resolved
src/servers/src/promql.rs Outdated Show resolved Hide resolved
src/servers/src/promql.rs Outdated Show resolved Hide resolved
@waynexia
Copy link
Member Author

waynexia commented Feb 2, 2023

By the way, I think we can get rid of PromqlHandler as well, make executing promql as a method of SqlQueryHandler, then rename SqlQueryHandler to QueryLangHandler (maybe other better names). Because promql is translated to logical plan to be actually executed, I think we can unify sql and promql execution path somehow. I've came up with similar thoughts in #766 , if we could make standalone and distribute instance both execute logical plan directly. In that way, promql could be executed in distribute mode, too.

Looking great 👍

Seems promql is only executed via http? In that case, I suggest not using PromqlServer at all, integrate PromqlHandler into struct HttpServer, just like other protocol handlers.

Consider implementing the prometheus' HTTP API (described in #924 (comment)), is it still necessary to add PromqlServer to implement prometheus' API?

@MichaelScofield
Copy link
Collaborator

@waynexia If we are to make our db as a drop in replacement for prometheus, a dedicated server(then a dedicated port) is better. So that prometheus queries do not mix-up with other protocols API pathes.

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia
Copy link
Member Author

waynexia commented Feb 2, 2023

I'd file two follow-up PRs for (1) rename prometheus service to prom_store and the new dedicated service promql to prometheus (I found too many existing references to include in this PR), and (2) refactor SqlQueryHandler trait and error types between servers mod and query mod.

src/frontend/src/instance.rs Outdated Show resolved Hide resolved
src/frontend/src/promql.rs Outdated Show resolved Hide resolved
src/servers/src/promql.rs Show resolved Hide resolved
src/servers/src/promql.rs Outdated Show resolved Hide resolved
@MichaelScofield
Copy link
Collaborator

There're conflicts. Also would you kind enough to create an issue and assign to me, that our gRPC SDK needs to support promql? thx

@waynexia
Copy link
Member Author

waynexia commented Feb 3, 2023

There're conflicts. Also would you kind enough to create an issue and assign to me, that our gRPC SDK needs to support promql? thx

Opened #936 for this

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

LGTM

@waynexia waynexia enabled auto-merge (squash) February 3, 2023 07:43
@killme2008
Copy link
Contributor

@waynexia Test failure integration_http_file_test::test_promql_api

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia
Copy link
Member Author

waynexia commented Feb 3, 2023

@waynexia Test failure integration_http_file_test::test_promql_api

Fixed in c67d85a. I only checked /v1/query with postman ....

@waynexia waynexia merged commit fc9276c into GreptimeTeam:develop Feb 3, 2023
@waynexia waynexia deleted the test-prom-range branch February 3, 2023 08:45
@waynexia waynexia mentioned this pull request Feb 7, 2023
2 tasks
@waynexia waynexia added this to the v0.1 milestone Feb 15, 2023
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
* chore: some tiny typo/style fix

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* feat: add promql server

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* works for mocked query

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* clean up

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* integration test case

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* resolve CR comments

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* expose promql api to our http server

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* resolve CR comments

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

* adjust router structure

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>

---------

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
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.

3 participants