Add support for fetching rules with API client #508

bobmshannon · 2018-11-21T20:02:59Z

This PR adds support for fetching rules via the /api/v1/rules endpoint using the API client. Currently, the API client exposes no way to do this and it would be nice to have for external systems that wish to retrieve this information.

cc @beorn7

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon · 2018-11-21T20:05:16Z

api/prometheus/v1/api.go

+type RuleGroup struct {
+	Name     string  `json:"name"`
+	File     string  `json:"file"`
+	Rules    []Rule  `json:"rules"`


Since a Rule can be either recording or alerting, I'm not sure if we actually want to use a stronger type here with two fields RecordingRules and AlertingRules, or put everything in the same list to preserve the rule order like https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L721 does.

beorn7 · 2018-11-21T23:54:12Z

That one is for @krasi-georgiev . 😄

krasi-georgiev

LGTM , but would be best if @simonpasquier and @mxinden can also have a quick look.

One thing I don't like is that we are using server mocks for the tests, but that is a story for another PR tracking in #485 (comment)

mxinden

Thanks for looking into this @bobmshannon. I am hoping that in the future we could generate clients based on e.g. OpenAPI specifications like we will soon do in github.com/prometheus/alertmanager.

I have a couple of comments. What are your thoughts?

mxinden · 2018-11-23T16:04:28Z

api/prometheus/v1/api.go

+	RuleTypeAlerting  RuleType = "alerting"
+
+	// Possible values for RuleHealth.
+	RuleHealthGood    = "ok"


Why not import them from here? https://github.com/prometheus/prometheus/blob/master/rules/manager.go#L46

mxinden · 2018-11-23T16:05:40Z

api/prometheus/v1/api.go

@@ -67,6 +82,15 @@ const (
 	HealthGood    HealthStatus = "up"
 	HealthUnknown HealthStatus = "unknown"
 	HealthBad     HealthStatus = "down"
+
+	// Possible values for RuleType.
+	RuleTypeRecording RuleType = "recording"


Mhh, its a bummer that I didn't expose these as constants when I added this. Sorry about that. Feel free to propose a patch on prometheus/prometheus.

mxinden · 2018-11-23T16:10:23Z

api/prometheus/v1/api.go

+	Groups []RuleGroup `json:"groups"`
+}
+
+type RuleGroup struct {


Why not use https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L718 ?

in general I am ok with duplicating types if this avoid a package dependency.
I follow the principle - import a package for its functionality and not for its types.

Here doesn't matter that much so I am ok either way.

The primary reasons for not not importing RuleGroup from the v1 API package referenced above:

RuleGroup models the list of rules as interface{} - this requires the client to use type assertion to determine whether a given rule is a recording or alerting rule.

It adds an external dependency that the client will need to import in order to use this API client package.

mxinden · 2018-11-23T16:11:12Z

api/prometheus/v1/api.go

+	Interval float64 `json:"interval"`
+}
+
+type Rule struct {


The name Rule is misleading, as recording rules do not expose e.g. Alerts.

Again, could we import structs from prometheus/prometheus instead?

I think we should just create two new types here: RecordingRule and AlertingRule. Is there any reason why order needs to be preserved by putting both types of rules in the same array?

If we create these two new types, I think we'll need some changes to the imported structs, for example recordingRule and alertingRule are not exported so can't be consumed by this package:
https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L730
https://github.com/prometheus/prometheus/blob/master/web/api/v1/api.go#L743

bobmshannon · 2018-11-25T21:06:48Z

I'm happy make the changes to this PR such that the types are imported from prometheus/prometheus, however I'll also need to make a patch to export some types so that they can be consumed here. Additionally, I think I'd prefer to keep the types encapsulated as much as possible so that clients don't need to import a whole bunch of external packages just to use the API client. Let me know what you all think.

As for using OpenAPI to generate the client code, agreed it would be a very nice place to get to.

bobmshannon · 2018-12-03T19:42:56Z

@mxinden Do you mind having another look at this one? I addressed some of the comments and added new fields AlertRules and RecordingRules to the RuleGroup struct to provide stronger typing and reduce ambiguity from a client perspective.

I think that importing the types at this point may be more trouble than it's worth, and we should probably just plan to supercede this client with the OpenAPI generated one, however I could be wrong about this.

Signed-off-by: Bob Shannon <bshannon@palantir.com>

mxinden · 2018-12-10T08:54:06Z

api/prometheus/v1/api.go

+	var recordingRules []RecordingRule
+
+	for _, rule := range v.Rules {
+		// Because both recording and alerting rules are stored in the same


They are stored in the same JSON array as the ordering among them matters (See prometheus/prometheus#4318 (comment) for more details). That way users can understand which rules depend on which.

I am not sure this is a blocker. But probably worth mentioning in a comment somewhere. What do you think?

Thanks for pointing me to that.

To keep things consistent, I've modified the client to preserve rule order as returned by the API, and added some documentation on this behavior regarding how to use a type switch to access each recording or alerting rule.

mxinden · 2018-12-10T09:03:32Z

On the one hand given the fact that this client is already tightly coupled with the prometheus/prometheus structs (e.g. when we change something in prometheus/prometheus this will likely break this client), I think this dependency should also be expressed in the code, hence importing it.

On the other hand according to the README of this repository we don't give any stability guarantees in regards to the client. Thereby I am fine with going either way.

Signed-off-by: Bob Shannon <bshannon@palantir.com>

mxinden · 2018-12-11T10:51:41Z

api/prometheus/v1/api.go

+//   default:
+//   	fmt.Printf("unknown rule type %s", v)
+//   }
+type Rules []interface{}


Where is this type used? Am I missing something?

Nice catch, fixed.

mxinden · 2018-12-11T10:56:10Z

api/prometheus/v1/api.go

+		if !ok {
+			return errors.New("rule has no type field")
+		}
+		ruleJSON, err := json.Marshal(rule)


I am a bit confused here. You are marshaling a rule to unmarshal it?

How about unmarshaling the rule into something like this first:

struct { type string }

and depending on type unmarshal as a recording or an alerting rule?

With that approach I think we'd still need to get the complete byte or JSON encoding of each individual rule (by using MarshalJSON) in order to unmarshal them into a typed recording or alerting rule. That said, I think most of the complexity here is from decoding the mixed type slice and there may not be a super elegant way to do it in Go.

I've made an update to this PR that should be easier to read and is more in-line with the approach suggested in this comment.

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon · 2018-12-17T18:07:53Z

@mxinden I've made some adjustments to this PR based on the most recent comments, please let me know what you think.

bobmshannon · 2019-01-07T17:37:27Z

@beorn7 @mxinden Just wanted to circle back on this one, is there anything I can do to help push this one through? It would be really nice to have this functionality in the API client for my use-case and for any external system that fetches alert and rules data from Prometheus. Thanks in advanced. :)

beorn7 · 2019-01-07T17:38:45Z

I'd like to see @mxinden and @krasi-georgiev to have the final look here.

krasi-georgiev · 2019-01-07T18:17:39Z

yeah waiting for Max for the final input.

mxinden

Given that the general preference seems to go towards duplication over dependency, this looks good to me.

I am sorry for the delayed response, just came back from a couple of weeks of vacation.

bobmshannon · 2019-01-14T20:06:09Z

Great, thanks!

krasi-georgiev · 2019-01-15T21:35:00Z

Thanks

Add support for fetching rules with API client

5c575dd

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon commented Nov 21, 2018

View reviewed changes

beorn7 requested a review from krasi-georgiev November 21, 2018 23:53

krasi-georgiev mentioned this pull request Nov 22, 2018

refactor and simplify the http client tests #485

Closed

krasi-georgiev approved these changes Nov 22, 2018

View reviewed changes

mxinden reviewed Nov 23, 2018

View reviewed changes

bobmshannon force-pushed the bs/rules_endpoint_support branch from ccc3e2d to d813b11 Compare December 3, 2018 19:38

Add RecordingRules and AlertingRules fields

e41661f

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon force-pushed the bs/rules_endpoint_support branch from d813b11 to e41661f Compare December 3, 2018 21:51

mxinden reviewed Dec 10, 2018

View reviewed changes

Preserve rule ordering

5d5fea7

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon force-pushed the bs/rules_endpoint_support branch from 62f239d to 5d5fea7 Compare December 10, 2018 20:47

mxinden reviewed Dec 11, 2018

View reviewed changes

Bob Shannon added 2 commits December 11, 2018 08:31

Doc tweak

c73aa0f

Signed-off-by: Bob Shannon <bshannon@palantir.com>

Use Rules type

40f84cb

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon force-pushed the bs/rules_endpoint_support branch from bb466da to 40f84cb Compare December 11, 2018 13:53

Use json.RawMessage to delay initial unmarshaling of mixed slice

2c872b9

Signed-off-by: Bob Shannon <bshannon@palantir.com>

bobmshannon force-pushed the bs/rules_endpoint_support branch from f6357a9 to 2c872b9 Compare December 11, 2018 18:29

mxinden approved these changes Jan 14, 2019

View reviewed changes

krasi-georgiev merged commit 18d13ea into prometheus:master Jan 15, 2019

bobmshannon deleted the bs/rules_endpoint_support branch January 16, 2019 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for fetching rules with API client #508

Add support for fetching rules with API client #508

bobmshannon commented Nov 21, 2018

bobmshannon Nov 21, 2018

beorn7 commented Nov 21, 2018

krasi-georgiev left a comment

mxinden left a comment

mxinden Nov 23, 2018

mxinden Nov 23, 2018

mxinden Nov 23, 2018

krasi-georgiev Nov 23, 2018

bobmshannon Nov 25, 2018 •

edited

mxinden Nov 23, 2018

mxinden Nov 23, 2018

bobmshannon Nov 25, 2018 •

edited

bobmshannon commented Nov 25, 2018

bobmshannon commented Dec 3, 2018

mxinden Dec 10, 2018

bobmshannon Dec 10, 2018

mxinden commented Dec 10, 2018

mxinden Dec 11, 2018

bobmshannon Dec 11, 2018

mxinden Dec 11, 2018

bobmshannon Dec 11, 2018

bobmshannon Dec 11, 2018

bobmshannon commented Dec 17, 2018

bobmshannon commented Jan 7, 2019

beorn7 commented Jan 7, 2019

krasi-georgiev commented Jan 7, 2019

mxinden left a comment

bobmshannon commented Jan 14, 2019

krasi-georgiev commented Jan 15, 2019

Add support for fetching rules with API client #508

Add support for fetching rules with API client #508

Conversation

bobmshannon commented Nov 21, 2018

Choose a reason for hiding this comment

beorn7 commented Nov 21, 2018

krasi-georgiev left a comment

Choose a reason for hiding this comment

mxinden 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

bobmshannon Nov 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobmshannon Nov 25, 2018 • edited

Choose a reason for hiding this comment

bobmshannon commented Nov 25, 2018

bobmshannon commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Dec 10, 2018

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

bobmshannon commented Dec 17, 2018

bobmshannon commented Jan 7, 2019

beorn7 commented Jan 7, 2019

krasi-georgiev commented Jan 7, 2019

mxinden left a comment

Choose a reason for hiding this comment

bobmshannon commented Jan 14, 2019

krasi-georgiev commented Jan 15, 2019

bobmshannon Nov 25, 2018 •

edited

bobmshannon Nov 25, 2018 •

edited