Added Prometheus Handler #9654
Added Prometheus Handler #9654
Conversation
panoskyriakis
had a problem deploying
to
manual-approval
GitHub Actions
Error
panoskyriakis
force-pushed
the
prometheus_integration
branch
from
026ffc5 to
316561c
Compare
panoskyriakis
had a problem deploying
to
manual-approval
GitHub Actions
Error
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
panoskyriakis
had a problem deploying
to
manual-approval
GitHub Actions
Error
|
@ZoranPandovski could you have a look at this and the failed checks? |
hamishfagg
had a problem deploying
to
manual-approval
GitHub Actions
Error
There was a problem hiding this comment.
Hey @panoskyriakis,
Overall, this looks good to me. I left a few minor comments to be addressed in the docs.
Quick question; I am not to familiar with Prometheus, but do you think it would be better to implement this as an APIHandler?
| To setup a connection to Prometheus host, run the following SQL in the MindsDB interface: | ||
|
|
||
| ```sql | ||
| CREATE DATABASE prometheus_sourxe |
There was a problem hiding this comment.
There's a small type here in the name of the data source. It should be prometheus_source.
panoskyriakis
had a problem deploying
to
manual-approval
GitHub Actions
Error
panoskyriakis
had a problem deploying
to
manual-approval
GitHub Actions
Error
@MinuraPunchihewa I think Prometheus is much closer to a database than an API. I used the ElasticSearch handler as a prototype which is quite similar to this one and is implemented as a DBHandler. Hope that works :) |
MinuraPunchihewa
had a problem deploying
to
manual-approval
GitHub Actions
Failure
There was a problem hiding this comment.
Hey @panoskyriakis,
Apologies for the delay in taking a look at this. I left some minor comments. I think we should be ready to merge once these are done.
| if not isinstance(query, Select): | ||
| raise ValueError("Only select queries are supported") | ||
|
|
||
| select_query = cast(Select, query) |
There was a problem hiding this comment.
Is this necessary? Based on your condition above, only SELECT queries will be allowed to get to this point, yes?
| raise ValueError("Only select queries are supported") | ||
|
|
||
| select_query = cast(Select, query) | ||
| from_table = select_query.from_table |
There was a problem hiding this comment.
Nit: The from_table variable does not seem to be used; should we maybe use in the subsequent lines rather than calling select_query.from_table again?
| conditions = extract_comparison_conditions(select_query.where) | ||
| conditions = convert_conditions_to_promql(conditions) | ||
|
|
||
| select_statement_parser = SELECTQueryParser( | ||
| query, | ||
| select_query.from_table, | ||
| self.get_columns(select_query.from_table) | ||
| ) | ||
|
|
||
| # TODO: use the orderbys returned by this parser | ||
| _, _, _, result_limit = select_statement_parser.parse_query() |
There was a problem hiding this comment.
It is possible to retrieve the conditions of the query when calling parse_query() of the SELECTQueryParser. I think it would be cleaner to do so, rather than calling extract_comparison_conditions again.
| Before proceeding, ensure the following prerequisites are met: | ||
|
|
||
| 1. Install MindsDB locally via [Docker](/setup/self-hosted/docker) or [Docker Desktop](/setup/self-hosted/docker-desktop). | ||
| 2. Install ```prometheus-api-client``` the only requirement of the Prometheus handler: |
There was a problem hiding this comment.
Can we replace the second step here with a sentence similar to what is given here? I think it would be best to point out all of the options available to install the dependencies for the integration.
Description
Added a handler to fetch data from Prometheus server. Per discussion here.
Fixes #issue_number
Type of change
Verification Process
The README contains instructions on how to run unit and e2e tests.
Checklist: