Repository: submariner-io/lighthouse
Pull request #468 (2021-02-23 13:57:03)

Add Service Import and Service Export metrics
Related issue #349

Signed-off-by: Maayan Friedman <maafried@redhat.com>

maayanf24: @skitt @tpantelis @mangelajo @sridhargaddam @nyechiel  - what do you think about prefix the metrics with "submariner_" for the lighthouse?
Should it be prefixed with "lighthouse_" instead?
nyechiel: > @skitt @tpantelis @mangelajo @sridhargaddam @nyechiel - what do you think about prefix the metrics with "submariner_" for the lighthouse?
> Should it be prefixed with "lighthouse_" instead?

For context, see https://github.com/submariner-io/submariner/pull/1143

Either option should work fine. Technically, users can run Lighthouse without Submariner, but I think that the majority will run both (and even if not, Lighthouse is still a repo under Submariner) - so for simplicity I am leaning towards "submariner_". 
tpantelis: > > @skitt @tpantelis @mangelajo @sridhargaddam @nyechiel - what do you think about prefix the metrics with "submariner_" for the lighthouse?
> > Should it be prefixed with "lighthouse_" instead?
> 
> For context, see [submariner-io/submariner#1143](https://github.com/submariner-io/submariner/pull/1143)
> 
> Either option should work fine. Technically, users can run Lighthouse without Submariner, but I think that the majority will run both (and even if not, Lighthouse is still a repo under Submariner) - so for simplicity I am leaning towards "submariner_".

I agree with "_submariner__" - "_lighthouse_" is just a repo name.  If we were to separate it from submariner then I'd suggest "_service_discovery_".
aswinsuryan: > > > @skitt @tpantelis @mangelajo @sridhargaddam @nyechiel - what do you think about prefix the metrics with "submariner_" for the lighthouse?
> > > Should it be prefixed with "lighthouse_" instead?
> > 
> > 
> > For context, see [submariner-io/submariner#1143](https://github.com/submariner-io/submariner/pull/1143)
> > Either option should work fine. Technically, users can run Lighthouse without Submariner, but I think that the majority will run both (and even if not, Lighthouse is still a repo under Submariner) - so for simplicity I am leaning towards "submariner_".
> 
> I agree with "_submariner__" - "_lighthouse_" is just a repo name. If we were to separate it from submariner then I'd suggest "_service_discovery_".

+1
vthapar: @skitt @aswinsuryan @maayanf24 Changes look good but I have a question. Should ServiceExport/Import of same service in different clusters be considered as single or multiple?
maayanf24: > @skitt @aswinsuryan @maayanf24 Changes look good but I have a question. Should ServiceExport/Import of same service in different clusters be considered as single or multiple?

@vthapar - each cluster only show local perspective of ServiceExport/Import
mangelajo: Oh I see it does actually use those prometheus counters from the admiral resource syncer, awesome.
mangelajo: Hey @maayanf24 sorry, this needs a rebase due to conflicts at this point.

Patch:
diff --git a/pkg/agent/controller/agent.go b/pkg/agent/controller/agent.go
index a324166..6e01008 100644
--- a/pkg/agent/controller/agent.go
+++ b/pkg/agent/controller/agent.go
@@ -20,6 +20,7 @@ import (
 	"reflect"
 
 	"github.com/pkg/errors"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/submariner-io/admiral/pkg/log"
 	"github.com/submariner-io/admiral/pkg/syncer"
 	"github.com/submariner-io/admiral/pkg/syncer/broker"
@@ -45,9 +46,15 @@ const (
 	clusterIP              = "cluster-ip"
 )
 
+type AgentConfig struct {
+	ServiceImportCounterName string
+	ServiceExportCounterName string
+}
+
 var MaxExportStatusConditions = 10
 
-func New(spec *AgentSpecification, syncerConf broker.SyncerConfig, kubeClientSet kubernetes.Interface) (*Controller, error) {
+func New(spec *AgentSpecification, syncerConf broker.SyncerConfig, kubeClientSet kubernetes.Interface,
+	syncerMetricNames AgentConfig) (*Controller, error) {
 	agentController := &Controller{
 		clusterID:        spec.ClusterID,
 		namespace:        spec.Namespace,
@@ -70,6 +77,10 @@ func New(spec *AgentSpecification, syncerConf broker.SyncerConfig, kubeClientSet
 			LocalSourceNamespace: metav1.NamespaceAll,
 			LocalResourceType:    &mcsv1a1.ServiceImport{},
 			BrokerResourceType:   &mcsv1a1.ServiceImport{},
+			SyncCounterOpts: &prometheus.GaugeOpts{
+				Name: syncerMetricNames.ServiceImportCounterName,
+				Help: "Count of imported services",
+			},
 		},
 	}
 
@@ -111,6 +122,10 @@ func New(spec *AgentSpecification, syncerConf broker.SyncerConfig, kubeClientSet
 		Transform:        agentController.serviceExportToServiceImport,
 		OnSuccessfulSync: agentController.onSuccessfulServiceImportSync,
 		Scheme:           syncerConf.Scheme,
+		SyncCounterOpts: &prometheus.GaugeOpts{
+			Name: syncerMetricNames.ServiceExportCounterName,
+			Help: "Count of exported services",
+		},
 	})
 	if err != nil {
 		return nil, err
diff --git a/pkg/agent/main.go b/pkg/agent/main.go
index 36abe10..711594c 100644
--- a/pkg/agent/main.go
+++ b/pkg/agent/main.go
@@ -105,7 +105,10 @@ func main() {
 		LocalClient:     localClient,
 		RestMapper:      restMapper,
 		Scheme:          scheme.Scheme,
-	}, kubeClientSet)
+	}, kubeClientSet,
+		controller.AgentConfig{
+			ServiceImportCounterName: "submariner_service_import",
+			ServiceExportCounterName: "submariner_service_export"})
 	if err != nil {
 		klog.Fatalf("Failed to create lighthouse agent: %v", err)
 	}


Test patch:
diff --git a/pkg/agent/controller/agent_test.go b/pkg/agent/controller/agent_test.go
index f26d36b..d062146 100644
--- a/pkg/agent/controller/agent_test.go
+++ b/pkg/agent/controller/agent_test.go
@@ -16,8 +16,10 @@ limitations under the License.
 package controller_test
 
 import (
+	"crypto/rand"
 	"errors"
 	"fmt"
+	"math/big"
 	"reflect"
 	"sort"
 	"time"
@@ -512,7 +514,20 @@ func (c *cluster) init(syncerConfig broker.SyncerConfig) {
 
 func (c *cluster) start(t *testDriver, syncerConfig broker.SyncerConfig) {
 	syncerConfig.LocalClient = c.localDynClient
-	agentController, err := controller.New(&c.agentSpec, syncerConfig, c.localKubeClient)
+	bigint, err := rand.Int(rand.Reader, big.NewInt(1000000))
+	Expect(err).To(Succeed())
+
+	serviceImportCounterName := "submariner_service_import" + bigint.String()
+
+	bigint, err = rand.Int(rand.Reader, big.NewInt(1000000))
+	Expect(err).To(Succeed())
+
+	serviceExportCounterName := "submariner_service_export" + bigint.String()
+
+	agentController, err := controller.New(&c.agentSpec, syncerConfig, c.localKubeClient,
+		controller.AgentConfig{
+			ServiceImportCounterName: serviceImportCounterName,
+			ServiceExportCounterName: serviceExportCounterName})
 
 	Expect(err).To(Succeed())
 	Expect(agentController.Start(t.stopCh)).To(Succeed())