-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add service monitor to Thanos Query #100
base: main
Are you sure you want to change the base?
Add service monitor to Thanos Query #100
Conversation
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
54b2ff9
to
142ce7e
Compare
Signed-off-by: Coleen Iona Quadros <coleen.quadros27@gmail.com>
b52194f
to
4d73886
Compare
@philipgough @saswatamcode its ready for review. I cannot add you as reviewers. |
// Enable self monitoring for the Thanos component. | ||
// +kubebuilder:validation:Optional | ||
// +kubebuilder:default:=true | ||
EnableSelfMonitor *bool `json:"enableSelfMonitor,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interested to hear with @saswatamcode thinks, but I think this should be its own struct because there are certain requirements for how service monitors can be created and where they should live to make this useful (thinking rhobs for example).
It could look something like this
type ServiceMonitorConfig struct {
// default true
Enabled
Labels map[string]string
// Can leave this out for now but it might be required or we can just hard default to current for now
Namespace
//
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have it in separate struct and maybe as part of commonthanosfields?
CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, | ||
CRDDirectoryPaths: []string{ | ||
filepath.Join("..", "..", "config", "crd", "bases"), | ||
filepath.Join("..", "..", "test", "configs", "service-monitor.yaml"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i misled you here because i thought that we already had those resources locally :/
a better test indeed might be in e2e - we could simply, in the first step, check against the prometheus to see if it reports the up metric for the component.
after deleting the SM we can just check that its gone. My apologies there I didnt check thoroughly enough the structure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to add the test here, but I wonder if we need to include CRD this way 🤔
@coleenquadros, I think the Environment struct has a CRDs field that can take in a Go CustomResourceDefinition struct. I wonder if we can import that struct from Prometheus Operator somehow? That way we don't need to maintain the CRD in-tree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maintaining the CRD in-tree is what I have the issue with. Ideally we would keep the integration tests as light as possible and enable running locally with out any network for example, as they are now.
Why I am suggesting adding this to e2e now instead is because we already take care of installing Prom operator there from my understanding and we can push much of the slower e2e work to ci. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup that works too, essentially skip testing this in integration.
If CRD isn't really available in Go way, makes sense to skip it. But I think Prometheus Operator may have some method that spits out Go CRD struct. We can then just use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, generally LGTM!
Some suggestions,
@@ -75,6 +75,10 @@ type CommonThanosFields struct { | |||
// +kubebuilder:default:=logfmt | |||
// +kubebuilder:validation:Optional | |||
LogFormat *string `json:"logFormat,omitempty"` | |||
// Enable self monitoring for the Thanos component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also mention Prometheus Operator version number in comment here?
CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, | ||
CRDDirectoryPaths: []string{ | ||
filepath.Join("..", "..", "config", "crd", "bases"), | ||
filepath.Join("..", "..", "test", "configs", "service-monitor.yaml"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to add the test here, but I wonder if we need to include CRD this way 🤔
@coleenquadros, I think the Environment struct has a CRDs field that can take in a Go CustomResourceDefinition struct. I wonder if we can import that struct from Prometheus Operator somehow? That way we don't need to maintain the CRD in-tree
return errors.IsNotFound(err) | ||
} | ||
|
||
func QueryPrometheus(query string) (*PrometheusResponse, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! Maybe for future, but we can probably copy some e2e thanos methods that do similar, if we want to check metrics.
For now I think this works well!
@@ -566,3 +582,48 @@ func VerifyCfgMapOrSecretEnvVarExists(c client.Client, expectResource ExpectApiR | |||
return false | |||
} | |||
} | |||
|
|||
func ApplyPrometheusCRD() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to comment above, but I think InstallPrometheusOperator
method, already provides us with the CRD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is for the prometheus object. But in that case, can we just define it in Go and apply? instead of having it as YAML.
I would avoid having definitions for tests in YAML as much as possible here, as things might change between versions which lead to confusing broken tests
No description provided.