Metrics/GenevaActions for Clustersync #3785
Conversation
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
5 times, most recently
from
3cdbf8c
to
8d1a6e9
Compare
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
6 times, most recently
from
b5ac73b
to
99fa8df
Compare
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
18 times, most recently
from
08835f8
to
41a6e7c
Compare
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
3 times, most recently
from
8d33911
to
afbaf3b
Compare
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
from
afbaf3b
to
548e097
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
2 times, most recently
from
88d5029
to
99c51f8
Compare
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
from
99c51f8
to
a04c6bb
Compare
|
Please rebase pull request. |
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
from
a04c6bb
to
99c3f75
Compare
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
2 times, most recently
from
60aee73
to
e884d16
Compare
merging 8659 and 9545 Metrics for SyncSet and SelectorSyncSets
rhamitarora
force-pushed
the
rhamitarora/ARO-9545-syncset-metrics
branch
from
e884d16
to
4522c6e
Compare
| @@ -23,7 +23,9 @@ var _ = Describe("Monitor", func() { | |||
| wg.Add(1) | |||
| mon, err := cluster.NewMonitor(log, clients.RestConfig, &api.OpenShiftCluster{ | |||
| ID: resourceIDFromEnv(), | |||
| }, &noop.Noop{}, nil, true, &wg) | |||
| }, &api.OpenShiftClusterDocument{ | |||
There was a problem hiding this comment.
I think it might be worth adding E2E tests for both the monitor and Geneva Actions functionality. We have various contexts in which E2E runs against an RP with Hive enabled (production/release E2E, PR E2E after we move to the containerized implementation).
There was a problem hiding this comment.
If it is too difficult to add the E2E tests in this PR, this can become follow-up work, however.
There was a problem hiding this comment.
New story created for the same
| } | ||
|
|
||
| func NewMonitor(log *logrus.Entry, restConfig *rest.Config, oc *api.OpenShiftCluster, m metrics.Emitter, hiveRestConfig *rest.Config, hourlyRun bool, wg *sync.WaitGroup) (*Monitor, error) { | ||
| func NewMonitor(log *logrus.Entry, restConfig *rest.Config, oc *api.OpenShiftCluster, doc *api.OpenShiftClusterDocument, m metrics.Emitter, hiveRestConfig *rest.Config, hourlyRun bool, wg *sync.WaitGroup, hiveClusterManager hive.ClusterManager) (*Monitor, error) { |
There was a problem hiding this comment.
It's a little strange to me that we have both oc and doc here (as oc should be a subproperty of doc), and hiveRestConfig and hiveClusterManager.
I think there's an opportunity for us to deduplicate some of these dependencies, but that can be a follow-up refactor.
There was a problem hiding this comment.
We used the doc property here because it is utilized in the common function GetClusterSync, which is referenced in pkg/frontend/admin_hive_syncset_resources.go. Although this point was considered earlier, it led to code duplication in another area.
Which issue this PR addresses:
ARO-9545 and ARO-8659 both JIRA's have common code
What this PR does / why we need it:
Test plan for issue:
Unit test cases added.
Need to create respective metrics dashboard in Geneva.
Is there any documentation that needs to be updated for this PR?
Will create TSGs for respective metrics.
How do you know this will function as expected in production?
Monitor from Geneva Dashboard.