-
Notifications
You must be signed in to change notification settings - Fork 180
Add direct-query-core module for prometheus integration #3440
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
Add direct-query-core module for prometheus integration #3440
Conversation
...t-query-core/src/main/java/org/opensearch/sql/datasource/client/DataSourceClientFactory.java
Outdated
Show resolved
Hide resolved
...t-query-core/src/main/java/org/opensearch/sql/datasource/client/DataSourceClientFactory.java
Outdated
Show resolved
Hide resolved
...t-query-core/src/main/java/org/opensearch/sql/datasource/client/DataSourceClientFactory.java
Outdated
Show resolved
Hide resolved
direct-query-core/src/main/java/org/opensearch/sql/directquery/rest/model/QueryEntityType.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public ExecuteDirectQueryResponse executeDirectQuery(ExecuteDirectQueryRequest request) { | ||
| // TODO: Replace with the data source query id. | ||
| String queryId = UUID.randomUUID().toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
| try { | ||
| return handler.executeQuery(client, request); | ||
| } catch (IOException e) { | ||
| return "{\"error\": \"Error executing query: " + e.getMessage() + "\"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if prometheus returns an error, should the SQL request fail with non-200 response?
direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java
Outdated
Show resolved
Hide resolved
| Response response = this.okHttpClient.newCall(request).execute(); | ||
|
|
||
| logger.info("Received Prometheus response for instant query: code={}", response); | ||
| // Return the full response object, not just the data field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the comment mean?
direct-query-core/src/main/java/org/opensearch/sql/prometheus/client/PrometheusClientImpl.java
Outdated
Show resolved
Hide resolved
| package org.opensearch.sql.prometheus.exceptions; | ||
| package org.opensearch.sql.prometheus.exception; | ||
|
|
||
| import org.opensearch.sql.datasources.exceptions.DataSourceClientException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be package org.opensearch.sql.datasource.client.exceptions.DataSourceClientException;? i think we should remove one of the two DataSourceClientException
any reason we need the datasource package under direct-query-core? or can we move it to datasource module?
Co-authored-by: Megha Goyal <[email protected]> Signed-off-by: Joshua Li <[email protected]>
ac6059f to
a64eefe
Compare
|
This PR is stalled because it has been open for 30 days with no activity. |
|
Hi @joshuali925 , do we still need this? |
|
we do, but currently no bandwidth |
@lezzago and I will take over this and take it further. Thanks. |
|
This PR is stalled because it has been open for 30 days with no activity. |
| // Optional fields | ||
| private Integer maxResults; // Optional: limit for Prometheus, maxDataPoints for CW | ||
| private Integer timeout; // Optional: number of seconds | ||
| private DataSourceOptions options; // Optional: Source specific arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we want a separate queryOptions or is the idea to have this contain query options as those are in a way specific to a datasource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't remember the details, will leave it to query API owner @goyamegh
currently this is the only usage
sql/direct-query-core/src/main/java/org/opensearch/sql/prometheus/model/PrometheusOptions.java
Lines 15 to 21 in ac6059f
| public class PrometheusOptions implements DataSourceOptions { | |
| private PrometheusQueryType queryType; | |
| private String step; // Duration string in seconds | |
| private String time; // ISO timestamp for instant queries | |
| private String start; // ISO timestamp for range queries | |
| private String end; // ISO timestamp for range queries | |
| } |
does look like query options
| public class ExecuteDirectQueryResponse { | ||
| private String queryId; | ||
| private String result; | ||
| private String sessionId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed if we have the queryId? What benefit is there to add another tracker if we can just use queryId everywhere instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i remember session id is for EMR job control, and different queries might reuse the same EMR job. I might be wrong but i agree direct (sync) query shouldn't have session concept.
will leave it to query API owner @goyamegh
…-query-core Signed-off-by: Joshua Li <[email protected]>
0ee85a3
into
opensearch-project:feature/direct-query-prometheus
Description
This PR adds direct-query-core module to SQL. It implements the following changes
ExecuteDirectQueryandGetDirectQueryResourcesactionsprometheusThis PR does not expose the actions in SQL plugin at runtime. The follow up #3441 for
direct-querymodule will add the rest and transport actions to SQLQuery API owner: @goyamegh
Resources API owner: @joshuali925
It is currently in draft since some tests are not fixed yet
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--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.