cli: extract client bindings of the v1 API by simonpasquier · Pull Request #1278 · prometheus/alertmanager

simonpasquier · 2018-03-08T11:02:53Z

This is the first pass for #1035. It is mostly based on prometheus/client_golang#333 with a few changes. Notably it uses types.Silence instead of model.Silence because model.Silence represents silence ID as int instead of string.

~~For now the client code lives in the github.com/prometheus/alertmanager/api package because it was easier this way and I didn't know where it should live otherwise.~~ The code lives in the github.com/prometheus/alertmanager/cli package. A subsequent PR should move all the CLI commands to github.com/prometheus/alertmanager/cli/cmd and make them use this new code.

cc @stuartnelson3

beorn7 · 2018-03-08T11:16:20Z

I have a vague memory of a conversation with @stuartnelson3 , concluding that the AM API client should live in github.com/prometheus/client_golang/api/alertmanager . I don't feel strongly about it. But if you put a Go client directly into the alertmanager repo, will you then also put a Java or C++ or Python client there?

simonpasquier · 2018-03-08T11:19:46Z

I understood from this comment that the agreement was to put the client code here. But re-reading #1035, I realized that the scope was only to extract the interactions with the API from the cli package. I'll rework the PR...

beorn7 · 2018-03-08T11:21:46Z

Got it, so I misremembered the conclusion. All great then. Go on… :o)

stuartnelson3 · 2018-03-08T13:39:40Z

@simonpasquier would you like me to take a look now, or wait until you remove [WIP]?

simonpasquier · 2018-03-08T14:35:32Z

@stuartnelson3 wait until I remove the WIP please.

This is a continuation of [1] but the code is kept in the alertmanage repository rather than having it in client_golang. [1] prometheus/client_golang#333 Co-Authored-By: Fabian Reinartz <fab.reinartz@gmail.com> Co-Authored-By: Tristan Colgate <tcolgate@gmail.com> Co-Authored-By: Corin Lawson <corin@responsight.com> Co-Authored-By: stuart nelson <stuartnelson3@gmail.com>

stuartnelson3

In addition to the comments inline, the List methods for both silences and alerts contact endpoints that support filtering. The cli uses this filtering. It would be good to expose this functionality so that cli and other consumers can filter their responses.

stuartnelson3 · 2018-03-18T12:25:29Z

cli/client.go

+	}
+	for _, v := range sil.Matchers {
+		s.Matchers = append(s.Matchers, &types.Matcher{Name: string(v.Name), Value: v.Value, IsRegex: v.IsRegex})
+	}


Why are these lines necessary? It looks like a types.Silences is just being copied into a new one. I'm able to remove this extra code and have the method work.

This is left-over from a previous version.

stuartnelson3 · 2018-03-18T12:25:44Z

cli/client.go

+func (h *httpSilenceAPI) Set(ctx context.Context, sil types.Silence) (string, error) {
+	var (
+		u      *url.URL
+		method string


This can be removed and http.MethodPost inlined.

stuartnelson3 · 2018-03-18T12:37:16Z

cli/client.go

+	// List returns all the active alerts.
+	List(ctx context.Context) ([]*model.Alert, error)
+	// Push sends a list of alerts to the Alertmanager.
+	Push(ctx context.Context, alerts ...model.Alert) error


I'm having import issues when trying to use this library, since there's an import mismatch:

./main.go:50:15: cannot use alert (type "github.com/prometheus/common/model".Alert) as type "github.com/prometheus/alertmanager/vendor/github.com/prometheus/common/model".Alert in argument to alertAPI.Push

We're running into issue because alertmanager is being used as a library, which shouldn't vendor this, when it was originally an application, which should vendor. We might have to introduce an intermediate type for the API.

Still having issues with this. The easiest thing to do is to introduce the intermediate type, and then in a separate PR we can decide if we want to replace references to model.Alert throughout the code base.

@brian-brazil what's the current state on using common/model? Are we deprecating it?

I had success when copying over the types from common/model:

diff --git a/cli/client.go b/cli/client.go index c623b25..9d7e0d4 100644 --- a/cli/client.go +++ b/cli/client.go @@ -25,7 +25,6 @@ import ( "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/types" - "github.com/prometheus/common/model" ) const ( @@ -154,9 +153,9 @@ func (h *httpStatusAPI) Get(ctx context.Context) (*ServerStatus, error) { // AlertAPI provides bindings for the Alertmanager's alert API. type AlertAPI interface { // List returns all the active alerts. - List(ctx context.Context) ([]*model.Alert, error) + List(ctx context.Context) ([]*Alert, error) // Push sends a list of alerts to the Alertmanager. - Push(ctx context.Context, alerts ...model.Alert) error + Push(ctx context.Context, alerts ...Alert) error } // NewAlertAPI returns a new AlertAPI for the client. @@ -168,7 +167,20 @@ type httpAlertAPI struct { client api.Client } -func (h *httpAlertAPI) List(ctx context.Context) ([]*model.Alert, error) { +type Alert struct { + Labels LabelSet `json:"labels"` + Annotations LabelSet `json:"annotations"` + StartsAt time.Time `json:"startsAt,omitempty"` + EndsAt time.Time `json:"endsAt,omitempty"` + GeneratorURL string `json:"generatorURL"` +} + +type LabelSet map[LabelName]LabelValue +type LabelName string +type LabelValue string + +// List implements the AlertAPI interface. +func (h *httpAlertAPI) List(ctx context.Context) ([]*Alert, error) { u := h.client.URL(epAlerts, nil) req, _ := http.NewRequest(http.MethodGet, u.String(), nil) @@ -178,13 +190,14 @@ func (h *httpAlertAPI) List(ctx context.Context) ([]*model.Alert, error) { return nil, err } - var alts []*model.Alert - err = json.Unmarshal(body, &alts) + var alerts []*Alert + err = json.Unmarshal(body, &alerts) - return alts, err + return alerts, err } -func (h *httpAlertAPI) Push(ctx context.Context, alerts ...model.Alert) error { +// Push implements the AlertAPI interface. +func (h *httpAlertAPI) Push(ctx context.Context, alerts ...Alert) error { u := h.client.URL(epAlerts, nil) var buf bytes.Buffer

@brian-brazil what's the current state on using common/model? Are we deprecating it?

The general direction is to replace it, but there's still going to be a common library one way or another.

stuartnelson3 · 2018-03-18T12:38:32Z

cli/client.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cli


I don't think this should be in cli, as it's an api that the cli will consume. It should probably be in the api package.

For this initial change, I was following the description from #1035 where @josedonizetti said:

I propose we extract the query/parsing of the API from the amtool, and expose it properly in the cli package, moving the amtool commands to a new package inside the cli (e.g.: cli/cmd).

I'm fine with your suggestion too. The downside with this approach is that anyone wanting to use only the API client bits will have to pull the server-side dependencies too.

stuartnelson3 · 2018-03-18T12:40:20Z

cli/client.go

+	return err
+}
+
+func (h *httpSilenceAPI) Set(ctx context.Context, sil types.Silence) (string, error) {


This method was failing with an EOF, started working after these changes:

diff --git a/cli/client.go b/cli/client.go index 617eac8..25ca9bd 100644 --- a/cli/client.go +++ b/cli/client.go @@ -19,7 +19,6 @@ import ( "encoding/json" "fmt" "net/http" - "net/url" "time" "github.com/prometheus/client_golang/api" @@ -250,36 +249,30 @@ func (h *httpSilenceAPI) Delete(ctx context.Context, id string) error { } func (h *httpSilenceAPI) Set(ctx context.Context, sil types.Silence) (string, error) { - var ( - u *url.URL - method string - ) - - var buf bytes.Buffer - s := types.Silence{ - Matchers: make([]*types.Matcher, len(sil.Matchers)), - StartsAt: sil.StartsAt, - EndsAt: sil.EndsAt, - CreatedBy: sil.CreatedBy, - Comment: sil.Comment, - } - for _, v := range sil.Matchers { - s.Matchers = append(s.Matchers, &types.Matcher{Name: string(v.Name), Value: v.Value, IsRegex: v.IsRegex}) - } - if err := json.NewEncoder(&buf).Encode(&s); err != nil { + buf := new(bytes.Buffer) + if err := json.NewEncoder(buf).Encode(&sil); err != nil { return "", err } - u = h.client.URL(epSilences, nil) - method = http.MethodPost + u := h.client.URL(epSilences, nil) - req, _ := http.NewRequest(method, u.String(), &buf) + req, err := http.NewRequest(http.MethodPost, u.String(), buf) + if err != nil { + return "", err + } - _, body, err := h.client.Do(ctx, req) + resp, body, err := h.client.Do(ctx, req) if err != nil { return "", err } + if resp.StatusCode/100 != 2 { + return "", &clientError{ + code: resp.StatusCode, + msg: "bad response code", + } + } + var res struct { SilenceID string `json:"silenceId"` }

stuartnelson3 · 2018-03-18T12:41:05Z

cli/client.go

+
+	req, _ := http.NewRequest(http.MethodGet, u.String(), nil)
+
+	_, body, err := h.client.Do(ctx, req)


it would be good to check the response code. A 404 or 500 is still a successful http response, but something we want to inform the api consumers of.

This is a call to the local apiClient.Do() which already handles the HTTP response code and un-marshals the server's response.

simonpasquier

Thanks for the review! I guess that the main question is whether the new code is meant to:

be used as a library immediately.
decouple the API interactions from the CLI code (which was @josedonizetti proposition initially) but keeping it in the same package for now.

It would be good to expose this functionality so that cli and other consumers can filter their responses.

Sure. This PR was just a continuation of prometheus/client_golang#333 with minimal changes to get things started.

simonpasquier · 2018-03-19T09:32:36Z

cli/client.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package cli


For this initial change, I was following the description from #1035 where @josedonizetti said:

I propose we extract the query/parsing of the API from the amtool, and expose it properly in the cli package, moving the amtool commands to a new package inside the cli (e.g.: cli/cmd).

I'm fine with your suggestion too. The downside with this approach is that anyone wanting to use only the API client bits will have to pull the server-side dependencies too.

simonpasquier · 2018-03-19T10:04:18Z

cli/client.go

+	}
+	for _, v := range sil.Matchers {
+		s.Matchers = append(s.Matchers, &types.Matcher{Name: string(v.Name), Value: v.Value, IsRegex: v.IsRegex})
+	}


This is left-over from a previous version.

simonpasquier · 2018-03-19T10:16:49Z

cli/client.go

+
+	req, _ := http.NewRequest(http.MethodGet, u.String(), nil)
+
+	_, body, err := h.client.Do(ctx, req)


This is a call to the local apiClient.Do() which already handles the HTTP response code and un-marshals the server's response.

stuartnelson3 · 2018-03-19T10:31:57Z

I guess leave it in cli for now, and we'll figure something out. Potentially we had lib/api or something to indicate library uses of alertmanager functionality vs. application uses.

stuartnelson3

Things look good, we just need to figure out a way forward with regards to the vendoring of common/model, it causes issues when trying to use this code as a library.

stuartnelson3 · 2018-03-27T13:45:01Z

cli/client.go

+	// List returns all the active alerts.
+	List(ctx context.Context) ([]*model.Alert, error)
+	// Push sends a list of alerts to the Alertmanager.
+	Push(ctx context.Context, alerts ...model.Alert) error


Still having issues with this. The easiest thing to do is to introduce the intermediate type, and then in a separate PR we can decide if we want to replace references to model.Alert throughout the code base.

@brian-brazil what's the current state on using common/model? Are we deprecating it?

stuartnelson3 · 2018-03-27T13:48:40Z

cli/client.go

+	// List returns all the active alerts.
+	List(ctx context.Context) ([]*model.Alert, error)
+	// Push sends a list of alerts to the Alertmanager.
+	Push(ctx context.Context, alerts ...model.Alert) error


I had success when copying over the types from common/model:

diff --git a/cli/client.go b/cli/client.go index c623b25..9d7e0d4 100644 --- a/cli/client.go +++ b/cli/client.go @@ -25,7 +25,6 @@ import ( "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/types" - "github.com/prometheus/common/model" ) const ( @@ -154,9 +153,9 @@ func (h *httpStatusAPI) Get(ctx context.Context) (*ServerStatus, error) { // AlertAPI provides bindings for the Alertmanager's alert API. type AlertAPI interface { // List returns all the active alerts. - List(ctx context.Context) ([]*model.Alert, error) + List(ctx context.Context) ([]*Alert, error) // Push sends a list of alerts to the Alertmanager. - Push(ctx context.Context, alerts ...model.Alert) error + Push(ctx context.Context, alerts ...Alert) error } // NewAlertAPI returns a new AlertAPI for the client. @@ -168,7 +167,20 @@ type httpAlertAPI struct { client api.Client } -func (h *httpAlertAPI) List(ctx context.Context) ([]*model.Alert, error) { +type Alert struct { + Labels LabelSet `json:"labels"` + Annotations LabelSet `json:"annotations"` + StartsAt time.Time `json:"startsAt,omitempty"` + EndsAt time.Time `json:"endsAt,omitempty"` + GeneratorURL string `json:"generatorURL"` +} + +type LabelSet map[LabelName]LabelValue +type LabelName string +type LabelValue string + +// List implements the AlertAPI interface. +func (h *httpAlertAPI) List(ctx context.Context) ([]*Alert, error) { u := h.client.URL(epAlerts, nil) req, _ := http.NewRequest(http.MethodGet, u.String(), nil) @@ -178,13 +190,14 @@ func (h *httpAlertAPI) List(ctx context.Context) ([]*model.Alert, error) { return nil, err } - var alts []*model.Alert - err = json.Unmarshal(body, &alts) + var alerts []*Alert + err = json.Unmarshal(body, &alerts) - return alts, err + return alerts, err } -func (h *httpAlertAPI) Push(ctx context.Context, alerts ...model.Alert) error { +// Push implements the AlertAPI interface. +func (h *httpAlertAPI) Push(ctx context.Context, alerts ...Alert) error { u := h.client.URL(epAlerts, nil) var buf bytes.Buffer

simonpasquier · 2018-03-28T12:05:21Z

I've added the following types to the cli package for now: Alert (used by the push alert API) and ExtendedAlert (used by the list alert API). I'm sure integrating this new code with the client code will require other changes down the road but I feel that pushing more stuff in this PR will complicate the review.

stuartnelson3

awesome, thanks for all the work!

* cli: extract client bindings of the v1 API from amtool This is a continuation of [1] but the code is kept in the alertmanage repository rather than having it in client_golang. [1] prometheus/client_golang#333 Co-Authored-By: Fabian Reinartz <fab.reinartz@gmail.com> Co-Authored-By: Tristan Colgate <tcolgate@gmail.com> Co-Authored-By: Corin Lawson <corin@responsight.com> Co-Authored-By: stuart nelson <stuartnelson3@gmail.com> * cli: fix httpSilenceAPI.Set() method * vendor: remove github.com/prometheus/client_golang/api/alertmanager * cli: don't use the model.Alert type

simonpasquier changed the title ~~api: add client bindings for the v1 API~~ [WIP] cli: extract client bindings for the v1 API Mar 8, 2018

simonpasquier changed the title ~~[WIP] cli: extract client bindings for the v1 API~~ [WIP] cli: extract client bindings of the v1 API Mar 8, 2018

simonpasquier force-pushed the add-client-v1 branch 2 times, most recently from cafa171 to 8e1bdc6 Compare March 12, 2018 09:41

simonpasquier force-pushed the add-client-v1 branch from 8e1bdc6 to 299e54b Compare March 13, 2018 14:01

simonpasquier changed the title ~~[WIP] cli: extract client bindings of the v1 API~~ cli: extract client bindings of the v1 API Mar 13, 2018

stuartnelson3 self-requested a review March 15, 2018 12:46

stuartnelson3 suggested changes Mar 18, 2018

View reviewed changes

simonpasquier commented Mar 19, 2018

View reviewed changes

simonpasquier added 2 commits March 19, 2018 15:38

cli: fix httpSilenceAPI.Set() method

be3dac9

vendor: remove github.com/prometheus/client_golang/api/alertmanager

ebe72a4

stuartnelson3 reviewed Mar 27, 2018

View reviewed changes

cli: don't use the model.Alert type

ee16456

stuartnelson3 approved these changes Mar 28, 2018

View reviewed changes

stuartnelson3 merged commit 0c086e3 into prometheus:master Mar 28, 2018

bobmshannon mentioned this pull request Apr 6, 2018

Create API client for Alert Manager prometheus/client_golang#399

Closed

stuartnelson3 mentioned this pull request May 3, 2018

Refactoring: Extract alertmanager http cli from amtool #1035

Closed

cben mentioned this pull request Dec 26, 2019

api client examples: avoid shadowing package with variable prometheus/client_golang#695

Merged


		req, _ := http.NewRequest(http.MethodGet, u.String(), nil)

		_, body, err := h.client.Do(ctx, req)

Conversation

simonpasquier commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beorn7 commented Mar 8, 2018

Uh oh!

simonpasquier commented Mar 8, 2018

Uh oh!

beorn7 commented Mar 8, 2018

Uh oh!

stuartnelson3 commented Mar 8, 2018

Uh oh!

simonpasquier commented Mar 8, 2018

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpasquier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartnelson3 commented Mar 19, 2018

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpasquier commented Mar 28, 2018

Uh oh!

stuartnelson3 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simonpasquier commented Mar 8, 2018 •

edited

Loading

simonpasquier left a comment •

edited

Loading