Add service monitor to Thanos Query #100
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default:=true | ||
| EnableSelfMonitor *bool `json:"enableSelfMonitor,omitempty"` | ||
|
There was a problem hiding this comment. 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. Let's have it in separate struct and maybe as part of commonthanosfields? |
||
| } | ||
|
|
||
| type Additional struct { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ import ( | |
| "runtime" | ||
| "testing" | ||
|
|
||
| monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" | ||
|
|
||
| . "github.com/onsi/ginkgo/v2" | ||
| . "github.com/onsi/gomega" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
@@ -64,7 +66,11 @@ var _ = BeforeSuite(func() { | |
|
|
||
| By("bootstrapping test environment") | ||
| testEnv = &envtest.Environment{ | ||
| CRDDirectoryPaths: []string{ | ||
| filepath.Join("..", "..", "config", "crd", "bases"), | ||
| filepath.Join("..", "..", "test", "configs", "service-monitor.yaml"), | ||
|
There was a problem hiding this comment. 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. 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. 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. 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? |
||
| }, | ||
|
|
||
| ErrorIfCRDPathMissing: true, | ||
|
|
||
| // The BinaryAssetsDirectory is only required if you want to run the tests directly | ||
|
|
@@ -85,6 +91,9 @@ var _ = BeforeSuite(func() { | |
| err = monitoringthanosiov1alpha1.AddToScheme(scheme.Scheme) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| err = monitoringv1.AddToScheme(scheme.Scheme) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| //+kubebuilder:scaffold:scheme | ||
|
|
||
| k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ | ||
|
|
||
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?