api: reorganize API package and add label values endpoint implementation #291

andrestc · 2017-03-31T22:56:06Z

This adds a client side implementation for fetching label values from prometheus api.

Would love to get some review on this as this is my first contribution here. Some design questions i may have oversimplified:

return a more descriptive value instead of []string? Maybe model.LabelValues?
create a separated interface for metadata api(following the separation on the docs)?

Related to #290.

This commit adds a client side implementation for fetching label values from prometheus api.

juliusv

Thanks, looks good besides comments!

juliusv · 2017-03-31T23:04:09Z

api/prometheus/api.go

 	QueryRange(ctx context.Context, query string, r Range) (model.Value, error)
+	// QueryLabelValues performs a query for the values of the given label.
+	QueryLabelValues(ctx context.Context, label string) ([]string, error)


To be more consistent with naming we have elsewhere, maybe omit the Query and just call this LabelValues.

And yeah, this should be model.LabelValues, since we're using the other model types in here as well.

juliusv · 2017-03-31T23:08:13Z

api/prometheus/api.go

+
+func (h *httpQueryAPI) QueryLabelValues(ctx context.Context, label string) ([]string, error) {
+	u := h.client.url(epLabelValues, map[string]string{"name": label})
+	req, _ := http.NewRequest(http.MethodGet, u.String(), nil)


Never ignore returned errors.

andrestc · 2017-04-01T00:32:59Z

@juliusv, addressed your comments. Thanks for the quick review!

grobie · 2017-04-02T19:40:33Z

api/prometheus/api.go

 	QueryRange(ctx context.Context, query string, r Range) (model.Value, error)
+	// LabelValues performs a query for the values of the given label.
+	LabelValues(ctx context.Context, label string) (model.LabelValues, error)


So, I always understood this interface to only include /api/v1/query* endpoints. With that, we'd end up with QueryAPI, LabelsAPI, AlertmanagerAPI, etc. interfaces.

I'd say we should either do that, or rename this interface to something like APIv1 (or a better name). Adding all current v1 functions in a QueryAPI interface doesn't make sense though, as not all are about querying.

Valid concern, and one that we are violating by the page menu hierarchy under which https://prometheus.io/docs/querying/api/ is embedded as well.

I don't like the idea of QueryAPI, LabelsAPI, etc. much, but if we also want to add Alertmanager API functionality here eventually, then maybe Prometheus and Alertmanager? I'm not even sure whether we should keep the ...API suffix. Or alternatively, ...Client instead of ...API.

I guess Prometheus and Alertmanager makes sense since there aren't that many endpoints to justify doing QueryAPI, LabelsAPI etc.

grobie · 2017-04-03T19:55:20Z

Yet? ;) Alertmanager is difficult though, as there are alertmanager related endpoints in the Prometheus API and the Alertmanager API itself. I'm pretty sure we'll come up with a new API version though at some point, so moving everything into a v1 would be good in general (or include the version in the types, but less readable and common).

…

On Mon, Apr 3, 2017, 16:45 André Carvalho ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In api/prometheus/api.go <#291 (comment)> : > QueryRange(ctx context.Context, query string, r Range) (model.Value, error) + // LabelValues performs a query for the values of the given label. + LabelValues(ctx context.Context, label string) (model.LabelValues, error) I guess Prometheus and Alertmanager makes sense since there aren't that many endpoints to justify doing QueryAPI, LabelsAPI etc. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaEsY7UbrQIVpEXvV4cpqLxA_1U8Iks5rsUw5gaJpZM4MwPXD> .

juliusv · 2017-04-04T00:41:29Z

Yet? ;) Alertmanager is difficult though, as there are alertmanager related endpoints in the Prometheus API and the Alertmanager API itself.

There are alert-related endpoints in Prometheus, but are there Alertmanager-related ones in Prometheus as well?

Agree on versioning, but that should happen for each of {Alertmanager, Prometheus, ...}, as their APIs are versioned separately. Maybe use a package for versioning, like the InfluxDB client does? https://godoc.org/github.com/influxdata/influxdb/client/v2

grobie · 2017-04-04T02:04:45Z

Check /api/v1/alertmanagers in https://prometheus.io/docs/querying/api/#alertmanagers. That's what I was referring to above as well, basically either we have one interface per /api/v1/<group> thing, or we only have one per API. An interface called Interface in a prometheus/v1 package describing all functions of the API might be enough for now?

andrestc · 2017-04-04T19:59:39Z

Looks good... I'm not so sure about the Interface name, maybe name the Interface API on prometheus/v1.

juliusv · 2017-04-04T22:53:55Z

Check /api/v1/alertmanagers in https://prometheus.io/docs/querying/api/#alertmanagers.

That's fine though, it's Prometheus's view of what Alertmanagers there are. It's not "an Alertmanager API", or part of it. It's just part of the Prometheus API. I'd be for prometheus/v1 (grouping all functionality under Prometheus's /api/v1/... endpoints) and alertmanager/vXXX (same for AM) packages. The two binaries are separate components with separate APIs and URLs.

In each package, I would expect an API type that gives access to all the endpoints of one particular binary. The httpClient and maybe even the apiClient wrapper around it could be shared between Prometheus and Alertmanager API packages.

andrestc · 2017-04-05T00:30:32Z

LGTM. If everyone agrees on that, should I update this PR with the new package/interface structure or open a new issue/pr for that?

grobie · 2017-04-11T16:04:56Z

Completely cool with that. I don't care about a new PR or continue using this one (but changing the name). Just make sure to use separate commits to first change the package structure and then another one to add your new feature/endpoint.

juliusv · 2017-04-11T20:18:59Z

@andrestc Oh yeah sorry, I meant to reply to this PR saying that we should wait until @grobie is back from his hiking to give him chance to object, but I forgot. Now we know he's fine with it, so go ahead :)

grobie · 2017-04-16T23:37:47Z

@andrestc Are you cool with the changes? This should unblock #248 as well.

andrestc · 2017-04-16T23:44:23Z

Yeah! I will update the PR with those, i'll probably have time for that on the next few days

This commit creates a new package to hold the prometheus v1 API interface. This interface will contain all the funcionality exposed by Prometheus v1 HTTP API. The underlying http client is kept on the api package since it may be reused across diferent API versions and also by the Alertmanager api package (to come.)

andrestc · 2017-04-19T21:30:40Z

Hey @juliusv, @grobie. I've pushed some of the changes we discussed.

I've left the http client on a separated package so it can be reused across Prometheus and Alertmanagers APIs(including future versions?).

Once you guys are OK with the changes I can rebase and make sure this last change comes before the label values query change.

grobie

Sorry, now you'll get a code review for code you didn't even write.

Looks good in general, due to the changes to the context and http packages, we can greatly simplify the client implementation.

grobie · 2017-04-20T11:44:13Z

api/client.go

+	"strings"
+	"time"
+
+	"golang.org/x/net/context"


The context package has been integrated into the stdlib, let's use that instead.

grobie · 2017-04-20T11:44:38Z

api/client.go

+	"time"
+
+	"golang.org/x/net/context"
+	"golang.org/x/net/context/ctxhttp"


Not longer necessary, the stdlib http package has native support for context now.

grobie · 2017-04-20T11:45:08Z

api/client.go

+
+// CancelableTransport is like net.Transport but provides
+// per-request cancelation functionality.
+type CancelableTransport interface {


Not longer necessary with the new http package. Will also fix #292.

grobie · 2017-04-20T11:49:39Z

api/client.go

+}
+
+func (c *httpClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte, error) {
+	resp, err := ctxhttp.Do(ctx, &http.Client{Transport: c.transport}, req)


I think we'll now have to use context.WithCancel here. I guess we should leave that to the user.

This commit removes the now unnecessary CancelableTransport and rely on the net/http context support.

andrestc · 2017-04-20T13:47:37Z

@grobie I've addressed your comments by removing the CancelableTransport and by passing down the ctx to the request (should I create a child context?).

We will need to drop go 1.6.x from travis and add go 1.8.x.

grobie · 2017-04-22T08:32:11Z

We will need to drop go 1.6.x from travis and add go 1.8.x.

Oh, @beorn7 will need to say if that's actually acceptable as requirement for client_golang users. What's the go1.8 feature? The new context package in stdlib and request.WithCancel() was added in go1.7 I thought?

grobie

LGTM

andrestc · 2017-04-22T14:38:16Z

Oh, I said to add go 1.8 just to keep the last two major versions on travis, we are fine on 1.7+.

beorn7 · 2017-04-22T15:15:40Z

@grobie We are talking only about the api package here, which is not widely used (and arguably experimental/unmaintained). the client_galang/prometheus/... packages should still be fine with older Go versions.

grobie · 2017-04-22T15:54:58Z

Once we stop running all tests with go1.6 it becomes very easy to unknowingly break compatibility in other packages. Although, if you're fine with such a setup, let's go forward here.

…

On Sat, Apr 22, 2017, 19:15 Björn Rabenstein ***@***.***> wrote: @grobie <https://github.com/grobie> We are talking only about the api package here, which is not widely used (and arguably experimental/unmaintained). the client_galang/prometheus/... packages should still be fine with older Go versions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaAa_8Dp3tbDq2Pm9YVWG_C5gYLgSks5ryhmcgaJpZM4MwPXD> .

beorn7 · 2017-04-22T18:00:58Z

OK, I see.

Yeah, that would be bad if the tests for an experimental package had an impact on the ones for a production package in frequent use.

Can we use build tags to not run the api tists on older go versions?

grobie · 2017-04-22T18:47:53Z

Yes, this should be possible, but we might need to put the build tags in both the test+api files.

…

On Sat, Apr 22, 2017 at 10:01 PM Björn Rabenstein ***@***.***> wrote: OK, I see. Yeah, that would be bad if the tests for an experimental package had an impact on the ones for a production package in frequent use. Can we use build tags to not run the api tists on older go versions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaJTCrFCghHGHXfjmpCqiMdXiCme7ks5rykBbgaJpZM4MwPXD> .

andrestc · 2017-04-24T21:10:21Z

I've pushed the change that includes the build tag to the api package files.

grobie

Thanks! One last comment, but looks great!

grobie · 2017-04-24T22:07:14Z

api/client.go

+// New returns a new Client.
+//
+// It is safe to use the returned Client from multiple goroutines.
+func New(cfg Config) (Client, error) {


Actually I think we should call this NewClient now, as the package is called API but we're not returning a type called api.

andrestc · 2017-04-25T14:38:01Z

Done! @grobie @juliusv

grobie

Thank you!

I'll leave it to @juliusv to change his review status and to @beorn7 to merge the PR.

juliusv

Good to go after two last nits :) Thanks!

juliusv · 2017-04-25T20:45:40Z

api/client.go

+
+// +build go1.7
+
+// Package api provides clients the HTTP API's.


API's -> APIs

clients the -> clients for the

juliusv · 2017-04-25T21:04:56Z

api/prometheus/v1/api.go

@@ -234,6 +70,16 @@ type Range struct {
 	Step time.Duration
 }

+// API provides bindings the Prometheus's v1 API.


bindings the -> bindings for

andrestc · 2017-04-25T21:27:10Z

Done, @juliusv

juliusv · 2017-04-25T21:35:55Z

Thanks!

tboerger · 2017-05-09T09:18:11Z

The build tag prevents the usage with go 1.8 :(

grobie · 2017-05-09T09:24:39Z

@tboerger Are you sure that's the problem? The documentation explicitly states - "go1.7", from Go version 1.7 onward https://golang.org/pkg/go/build/.

tboerger · 2017-05-09T09:26:57Z

I have created #298 now to allow go 1.8 as well.

api: adds label values query implementation

2f33fab

This commit adds a client side implementation for fetching label values from prometheus api.

juliusv requested changes Mar 31, 2017

View reviewed changes

andrestc added 4 commits March 31, 2017 21:05

api: stops ignoring errors creating requests

8cb7769

api: renames method for consistency

123637b

api: returns specific type for label values

57f23d3

api: fix godoc comment

7b5f0fd

grobie reviewed Apr 2, 2017

View reviewed changes

grobie reviewed Apr 20, 2017

View reviewed changes

andrestc added 2 commits April 20, 2017 09:57

api: uses stdlib context

11fae2e

api: uses context from net/http

09dcce7

This commit removes the now unnecessary CancelableTransport and rely on the net/http context support.

grobie approved these changes Apr 22, 2017

View reviewed changes

grobie mentioned this pull request Apr 24, 2017

Why CancelableTransport? #292

Closed

grobie changed the title ~~api: adds label values query implementation~~ api: reorganize API package and add label values endpoint implementation Apr 24, 2017

api: add build tag for go 1.7

6fdb468

grobie reviewed Apr 24, 2017

View reviewed changes

api: renames create client func

5d19d9d

grobie approved these changes Apr 25, 2017

View reviewed changes

grobie mentioned this pull request Apr 25, 2017

Instrument RoundTripper via middleware #295

Merged

juliusv reviewed Apr 25, 2017

View reviewed changes

api: fix typo in comments

8077e4b

juliusv approved these changes Apr 25, 2017

View reviewed changes

juliusv merged commit 7d94842 into prometheus:master Apr 25, 2017

andrestc deleted the query-label-values branch July 31, 2017 18:28

andrestc mentioned this pull request Apr 7, 2018

Add support for API admin methods #398

Merged


		// +build go1.7

		// Package api provides clients the HTTP API's.

api: reorganize API package and add label values endpoint implementation #291

api: reorganize API package and add label values endpoint implementation #291

Conversation

andrestc commented Mar 31, 2017

juliusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrestc commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie commented Apr 3, 2017 via email

juliusv commented Apr 4, 2017

grobie commented Apr 4, 2017

andrestc commented Apr 4, 2017

juliusv commented Apr 4, 2017

andrestc commented Apr 5, 2017 via email • edited Loading

grobie commented Apr 11, 2017

juliusv commented Apr 11, 2017

grobie commented Apr 16, 2017

andrestc commented Apr 16, 2017

andrestc commented Apr 19, 2017 • edited Loading

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrestc commented Apr 20, 2017

grobie commented Apr 22, 2017

grobie left a comment

Choose a reason for hiding this comment

andrestc commented Apr 22, 2017

beorn7 commented Apr 22, 2017

grobie commented Apr 22, 2017 via email

beorn7 commented Apr 22, 2017

grobie commented Apr 22, 2017 via email

andrestc commented Apr 24, 2017

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrestc commented Apr 25, 2017

grobie left a comment

Choose a reason for hiding this comment

juliusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrestc commented Apr 25, 2017

juliusv commented Apr 25, 2017

tboerger commented May 9, 2017

grobie commented May 9, 2017

tboerger commented May 9, 2017

andrestc commented Apr 5, 2017 via email •

edited

Loading

andrestc commented Apr 19, 2017 •

edited

Loading