Add default Prometheus query range route for Grafana integration #877
Conversation
Codecov Report
@@ Coverage Diff @@
## master #877 +/- ##
==========================================
+ Coverage 78.37% 78.55% +0.17%
==========================================
Files 393 393
Lines 33391 33405 +14
==========================================
+ Hits 26171 26240 +69
+ Misses 5415 5365 -50
+ Partials 1805 1800 -5
Continue to review full report at Codecov.
|
…/m3 into r/register-default-prom-query-route
There was a problem hiding this comment.
Might need to update this script as well since endpoint as well as output formatting is changing:
https://github.com/m3db/m3/blob/master/src/cmd/services/m3query/scripts/prom-m3-diff.sh
| if firstErr != nil { | ||
| // Try parsing as an integer value specifying seconds, the Prometheus default | ||
| if seconds, err := strconv.ParseInt(d, 10, 64); err == nil { | ||
| value = time.Duration(seconds) * time.Second |
There was a problem hiding this comment.
nit: return time.Duration(seconds) * time.Second, nil?
There was a problem hiding this comment.
Good call, also refactored the initial form value if/else too.
| if !ok || len(targetQueries) == 0 || targetQueries[0] == "" { | ||
| func parseQuery(r *http.Request) (string, error) { | ||
| queries, ok := r.URL.Query()[queryParam] | ||
| if !ok || len(queries) == 0 || queries[0] == "" { | ||
| return "", errors.ErrNoTargetFound |
There was a problem hiding this comment.
nit: rename ErrNoTargetFound to ErrNoQueryFound?
There was a problem hiding this comment.
Sounds good.
| `) | ||
|
|
||
| actual := mustPrettyJSON(t, buffer.String()) | ||
|
|
| @@ -42,7 +42,7 @@ import ( | |||
| const ( | |||
| endParam = "end" | |||
| startParam = "start" | |||
| targetParam = "target" | |||
| queryParam = "query" | |||
There was a problem hiding this comment.
Is this going to work fine for the prometheus remote read end point?
There was a problem hiding this comment.
Oh wait, this is the native end point, not the remote read one. Never mind!
|
There's a couple of places which still use |
| echo $m3url | ||
| echo $promurl | ||
| curl -G $m3url > m3out | ||
| curl -G $promurl > promout | ||
| jq ".[]|.tags,.datapoints" m3out > m3result |
There was a problem hiding this comment.
This might have to be values rather than datapoints, and metric instead of tags
src/query/ts/values.go
Outdated
| @@ -107,17 +107,17 @@ func (b *fixedResolutionValues) StartTime() time.Time { | |||
|
|
|||
| // Resolution returns resolution per step | |||
| func (b *fixedResolutionValues) Resolution() time.Duration { | |||
| return b.MillisPerStep() | |||
| return b.MillisPerStep() * time.Millisecond | |||
There was a problem hiding this comment.
Could this screw up things down the line?
There was a problem hiding this comment.
Looks like semaphore's failing a few unit tests, this seems like the likely culprit
| dp := vals.DatapointAt(i) | ||
| // Skip points before the query boundary. Ideal place to adjust these would be at the result node but that would make it inefficient | ||
| // since we would need to create another block just for the sake of restricting the bounds. | ||
| // Each series have the same start time so we just need to calculate the correct startIdx once | ||
| // NB(r): Removing the optimization of computing startIdx once just in case our assumptions are wrong, |
There was a problem hiding this comment.
This is a bit odd that you're seeing more points after deleting this. Might bear further investigation to see where our assumptions go bad
| curl -G $m3command > m3out | ||
| curl -G $promcommand > promout | ||
| jq ".[]|.tags,.datapoints" m3out > m3result | ||
| queryurl="/api/v1/query_range?start=$start&end=$end&step=$step --data-urlencode query=$target" |
| @@ -126,6 +126,8 @@ echo "Sleep for 30 seconds to let the remote write endpoint generate some data" | |||
|
|
|||
| sleep 30 | |||
|
|
|||
| # Ensure Prometheus can proxy a Prometheus query | |||
There was a problem hiding this comment.
do you also want to test m3query directly ?
There was a problem hiding this comment.
I do, I'm going to do this in a follow up change if that's ok - its not trivial.
src/query/api/v1/httpd/handler.go
Outdated
| // Prometheus native query endpoints, registered under both the default | ||
| // Prometheus server route and the Prometheus native read endpoint route | ||
| promNativeRead := logged(native.NewPromReadHandler(h.engine)).ServeHTTP | ||
| h.Router.HandleFunc(prometheus.DefaultQueryRangeURL, promNativeRead).Methods(prometheus.DefaultQueryRangeMethod) |
There was a problem hiding this comment.
does it make sense to have both or shall we just update native.PromReadURL to point to /api/v1/query_range ?
No description provided.