-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Metricbeat] Add 'query' metricset for prometheus module #15177
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
1 similar comment
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Thank you for opening this @jabbukka!! 🎉 . Could you please explain how would you use this besides the given examples? I try to understand the general use case. Also I wonder, would it make sense to store only the last value? How do you plan to query this data? |
@exekias Sorry for the lack of explanation. 😥 However, it was difficult to calculate metrics with Prometheus raw data in ElasticSearch using ES Query. Because Prometheus data model are based on time series. For examples, Histogram data model uses
the last value is representative for the last query result. i. e) cpu usage rate for last 1 minute.
I hope this made things a bit more clear. |
// Vector [ <unix_timestamp>, "<query_result>" ] is not acceptable for Elasticsearch. | ||
// Because there are two types in one array. | ||
// So change Vector to Object { unixtimestamp: "<unix_timestamp", value: "query_result" } | ||
if res.Data.ResultType == "vector" { |
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.
It would be nice to come up with a good conversion for all result types. I think we can strip down much of the info we get in the response to store only timestamp + results. What do you think about doing something like this?:
- For
scalar
:- Store timestamp under event tiemstamp (
mb.Event.Timestamp
) (this will be reported as@timestamp
in the final event) - Store value under
prometheus.query.<query_name>
(ie:prometheus.query.http_request
in your example)
- Store timestamp under event tiemstamp (
- For
string
:- Store timestamp under event tiemstamp (
mb.Event.Timestamp
) - Store value under
prometheus.query.<query_name>
- Store timestamp under event tiemstamp (
- For
vector
:- Create an event for each metric
- Store timestamp under event timestamp (
mb.Event.Timestamp
) - Store value under
prometheus.query.<query_name>
- Store labels under
prometheus.labels
- For
matrix
(range vector):- Create an event for each metric and (timestamp, vlue) pairs
- Store timestamp under event timestamp (
mb.Event.Timestamp
) - Store value under
prometheus.query.<query_name>
- Store labels under
prometheus.labels
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.
Looks good! I'll try to apply this.
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'm applying the suggested changes on code. Please review commit #c511e95faf
@jabbukka apologies for having it stalled all that time. Could you rebase it on top of the latest master so as to resolve the conflicts and see what is missing? It would be nice to have it in soon.
Also all imports should be defined with |
@@ -0,0 +1,32 @@ | |||
[ |
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.
All files under tesdata
directory are used for testing. In this a docs.plain
file is needed which then will be used as a testing input. See for example https://github.com/elastic/beats/blob/a174425cb45c406a748b290dd2ef892c731cdac7/metricbeat/module/prometheus/collector/_meta/testdata/docs.plain.
) | ||
|
||
func TestData(t *testing.T) { | ||
mbtest.TestDataFiles(t, "prometheus", "query") |
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 actually doing anything? As mentioned in a previous comment a docs.plain
is required to be used as input in the tests.
Vectors [][]interface{} `json:"values"` | ||
} | ||
|
||
func (m *MetricSet) parseResponse(body []byte, pathConfig PathConfig) ([]mb.Event, error) { |
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 there any special reason for parseResponse
to be a method of Metricset? If not the following should be fine:
func (m *MetricSet) parseResponse(body []byte, pathConfig PathConfig) ([]mb.Event, error) { | |
func parseResponse(body []byte, pathConfig PathConfig) ([]mb.Event, error) { |
type PathConfig struct { | ||
Path string `config:"path"` | ||
Fields common.MapStr `config:"fields"` | ||
Name string `config:"name"` |
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.
Maybe it would be better to call it MetricName
QueryName
(and metric_name
query_name
) so as to be more specific.
// Validate for Prometheus "query" metricset config | ||
func (p PathConfig) Validate() error { | ||
if p.Name == "" { | ||
return errors.New("`namespace` can not be empty in path configuration") |
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.
Namespace can be confusing here. We had better call it metric_name
query_name
.
if err := json.Unmarshal(body, &arrayBody); err != nil { | ||
return nil, "", errors.Wrap(err, "Failed to parse api response") | ||
} | ||
|
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.
Shouldn't we check if the query was sucessful?
Sth like:
if arrayBody.Status == "error" {
return nil, "", errors.Errorf("Failed to query")
}
|
||
// Config for "query" metricset | ||
type Config struct { | ||
Paths []PathConfig `config:"paths"` |
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.
Maybe a name like Queries
would be better here. wdyt?
|
||
// PathConfig is used to make a API request. | ||
type PathConfig struct { | ||
Path string `config:"path"` |
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.
Maybe Query
here?
// PathConfig is used to make a API request. | ||
type PathConfig struct { | ||
Path string `config:"path"` | ||
Fields common.MapStr `config:"fields"` |
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.
Maybe QueryParams
?
|
||
events, parseErr := m.parseResponse(body, pathConfig) | ||
if parseErr != nil { | ||
return err |
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.
Since we are in a loop I would say that we don't have to return but to log the error and continue to next query, sth like:
if parseErr != nil {
m.Logger().Debug("error parsing response for ", pathConfig.Name, ": ", parseErr)
reporter.Error(errors.Wrap(err, "error parsing response"))
continue
}
I would do the same if FetchResponse()
return error too.
* Remove array from response body. Original response body has an array (Vector type). It makes difficult to query with Elasticsearch QL. New data schema: "prometheus": { "query": { "mem_usage": { "status": "success", "data": { "resultType": "vector", "result": [ { "metric": {}, "reconciledValue": { "unixtimestamp": 1.576751116531e+09, "value": "2947656593408" } } ] } } } }
* Prometheus API returns "string" type for query result. But actual result type is a number. * Make it easy to use ES SQL
import ( | ||
"errors" | ||
|
||
"github.com/elastic/beats/libbeat/common" |
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.
"github.com/elastic/beats/libbeat/common" | |
"github.com/elastic/beats/v7/libbeat/common" |
"strconv" | ||
"time" | ||
|
||
"github.com/elastic/beats/libbeat/common" |
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.
"github.com/elastic/beats/libbeat/common" | |
"github.com/elastic/beats/v7/libbeat/common" |
"time" | ||
|
||
"github.com/elastic/beats/libbeat/common" | ||
"github.com/elastic/beats/metricbeat/mb" |
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.
"github.com/elastic/beats/metricbeat/mb" | |
"github.com/elastic/beats/v7/metricbeat/mb" |
import ( | ||
"io/ioutil" | ||
|
||
"github.com/elastic/beats/libbeat/common" |
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.
"github.com/elastic/beats/libbeat/common" | |
"github.com/elastic/beats/v7/libbeat/common" |
"io/ioutil" | ||
|
||
"github.com/elastic/beats/libbeat/common" | ||
"github.com/elastic/beats/metricbeat/helper" |
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.
"github.com/elastic/beats/metricbeat/helper" | |
"github.com/elastic/beats/v7/metricbeat/helper" |
|
||
"github.com/elastic/beats/libbeat/common" | ||
"github.com/elastic/beats/metricbeat/helper" | ||
"github.com/elastic/beats/metricbeat/mb" |
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.
"github.com/elastic/beats/metricbeat/mb" | |
"github.com/elastic/beats/v7/metricbeat/mb" |
|
||
func (m *MetricSet) getURL(path string, queryMap common.MapStr) string { | ||
queryStr := mb.QueryParams(queryMap).String() | ||
return "http://" + m.BaseMetricSet.Host() + path + "?" + queryStr |
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.
return "http://" + m.BaseMetricSet.Host() + path + "?" + queryStr | |
return m.http.GetURI() + path + "?" + queryStr |
In order to have the URI already set properly on module initialisation you will need to initialise Metricset like:
const (
defaultScheme = "http"
)
var (
hostParser = parse.URLHostParserBuilder{
DefaultScheme: defaultScheme,
}.Build()
)
func init() {
mb.Registry.MustAddMetricSet("prometheus", "query", New,
mb.WithHostParser(hostParser),
)
}
@jabbukka I think that the requested changes might need a decent amount of work and testing and since we are moving forward to code freeze would you mind if I do the changes-cleanup for you on a different PR while keeping your commits ( in this, in the merge commit you will be the co-author) Let me know! |
@ChrsMark Sure! No problem. :) |
Hey @jabbukka ! This feature has been merged! Here is the commit on master: 7c82034. Thank you for proposing and implementing this feature! |
Pinging @elastic/integrations-platforms (Team:Platforms) |
@ChrsMark I'm so glad to contribute it! :) |
Motivation
There is no way to use Prometheus Query using Prometheus collect API (/metrics, /federate)
If Metricbeat can use Prometheus query API, it would be very powerful to collect only what I need.
Refer to the following documentation: https://prometheus.io/docs/prometheus/latest/querying/api/
Purpose
Prometheus HTTP query API
How to use
Please review this and give me feedback. :)