feat: added alertmanager client package #873
Conversation
atighineanu
force-pushed
the
feature/alertmanager
branch
4 times, most recently
from
e5945de to
226da7f
Compare
There was a problem hiding this comment.
Sorry for the long delay and thanks for your work.
I left some basic comments, In general I think this is a good start. Maybe we can cleanup some types, but thats not a priority topic.
Regarding your questions:
- The protocol is part of the URL now and I think that's fine, no need for additional configs
- I think we should offer basic-auth at the beginning, TLS-Auth can be added afterwards if needed.
| return &Client{ | ||
| Token: os.Getenv("ALERTMGR_TOKEN"), | ||
| HostURL: os.Getenv("ALERTMGR_URL"), | ||
| Client: new(http.Client), |
There was a problem hiding this comment.
Please use cli-flags instead (they can be populated with env-variables as well)
There was a problem hiding this comment.
Perfect, thanks for feedback. That's exactly what I needed to know!
atighineanu
force-pushed
the
feature/alertmanager
branch
4 times, most recently
from
985d213 to
7779ec1
Compare
atighineanu
force-pushed
the
feature/alertmanager
branch
from
7779ec1 to
924b504
Compare
Signed-off-by: atighineanu <[email protected]>
atighineanu
force-pushed
the
feature/alertmanager
branch
from
924b504 to
123c6df
Compare
Signed-off-by: atighineanu <[email protected]>
|
This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days). |
| rebootStart string | ||
| rebootEnd string | ||
| timezone string | ||
| annotateNodes bool |
There was a problem hiding this comment.
nit: As I don't think we preserve the order of the addition vars in this section, we might as well sort them alphabetically?
| } | ||
|
|
||
| // Status builds the Status endpoint | ||
| func (c *Client) Status() *StatusEndpoint { |
There was a problem hiding this comment.
what's your intention behind this?
I missed where the statusEndpoint (and its Get()) is used.
Do you intend to use Status() to alter the behaviour of kured? For example, if alertmanager is not reachable, Status would return an error, who would prevent a reboot?
I like the concept, just not understanding where we are heading here (sorry if I missed a part of the code!)
| } | ||
|
|
||
| // BuildURL builds the full URL for silences Endpoint | ||
| func (s *SilencesEndpoint) BuildURL() error { |
There was a problem hiding this comment.
Same reasoning as above.
|
|
||
| // Get receives information about alert manager overall status | ||
| func (s *StatusEndpoint) Get() (*StatusResponse, error) { | ||
| err := s.BuildURL() |
There was a problem hiding this comment.
Why does this BuildURL() method exist, while the StatusEndpoint data structure receives the FullURL (which I suppose contains status)?
If FullURL does not intend to be the complete URL (status included) and only intends to be base URL, then I guess having this logic could make sense.
However, in that case, I think I would implement it using the standard go net/url instead, because we have a recent enough version of go. This allows us to remove BuildURL() completely.
For example, in statusendpoint get(), I would do url.JoinPath(s.baseUrl,"status") (see https://pkg.go.dev/net/[email protected]#JoinPath).
This automatically parses the url and will output any errors of parsing: https://cs.opensource.google/go/go/+/refs/tags/go1.20:src/net/url/url.go;l=1262.
Hence the purpose of L61 of this file is still a one liner, yet it removes the whole BuildURL.
On top of that, I feel it is easier to grasp the Get() if you have all the details there.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| request.Header.Add("Authentication", fmt.Sprintf("Bearer %s", s.Token)) |
There was a problem hiding this comment.
what if there is no auth? Do we still have to add the header?
For example if AlertManager is empty.
| "time" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Did you consider using the alertmanager cli go module instead?
Would it be more complex to maintain?
| ) | ||
|
|
||
| var ( | ||
| silenceStates = map[string]bool{"expired": true, "active": true, "pending": true} |
There was a problem hiding this comment.
Maybe a comment or another data structure would help here, as the intent is not obvious (our goal should not be to have our own alertmanager client, just the minimum we need for the functionality required)
| } | ||
|
|
||
| // SilencesEndpoint is the silences enpoint of the alert manager client | ||
| type SilencesEndpoint struct { |
There was a problem hiding this comment.
Isn't that the same structure?
If you still wish to use a not complete url and keep the different data types (did you think of interfaces?), you could then use a variable in such structure for relative endpoint, which could be "status" for StatusEndpoint. This could be filled by default with tags. It would improve readability IMO, but less than including the whole mumbojumbo in the Get().
... On top of that, I prefer the endpoint being the full URL to be honest :) I avoid the temptation to guess.
|
This PR was automatically considered stale due to lack of activity. Please refresh it and/or join our slack channels to highlight it, before it automatically closes (in 7 days). |
Hi everyone.
To finish this PR, I'd need more input on what kind of features for the alert manager client would you like to have. In the alert manager documentation one can set up tls, authentication. I've created an empty http.Client and would need more info about:
The client does (so far) only two things:
(tested on an alert manager inside a kind cluster)
Status().Get()
Silences().Get()
Thanks