Skip to content
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 Metrics to the sync manager #244

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Conversation

evanfoster
Copy link
Contributor

Description

This PR adds metrics to the new sync manager component. I've tried to find the metrics that will allow us to identify/meaningfully alert on issues with the sync manager, but I haven't had much experience with implementing metrics—Please question any choices I've made if the motivation behind them isn't clear.

Additionally, I may need some help getting the unit tests to run correctly. Right now, the monitoring/manager/metrics_test.go tests seem to run far more often than they should, and they don't seem to run under the correct package.

Evan Foster added 2 commits September 6, 2024 17:04
@evanfoster evanfoster requested a review from a team as a code owner September 6, 2024 23:10
Copy link
Member

@aalexandru aalexandru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good!

pkg/sync/manager/controller.go Outdated Show resolved Hide resolved
pkg/sync/manager/controller.go Outdated Show resolved Hide resolved
pkg/monitoring/manager/metrics.go Show resolved Hide resolved
@aalexandru
Copy link
Member

@evanfoster I tried to run all the tests under /pkg/monitoring and they seem to be running fine:

❯ go test -race ./pkg/monitoring/... -count=1 -v
=== RUN   TestNewMetrics
--- PASS: TestNewMetrics (0.00s)
=== RUN   TestMetricsRegistered
--- PASS: TestMetricsRegistered (0.00s)
=== RUN   TestRecordStatusErrorCnt
--- PASS: TestRecordStatusErrorCnt (0.00s)
=== RUN   TestRecordEgressRequestCnt
--- PASS: TestRecordEgressRequestCnt (0.00s)
=== RUN   TestRecordEgressRequestDur
--- PASS: TestRecordEgressRequestDur (0.00s)
=== RUN   TestRecordIngressRequestCnt
--- PASS: TestRecordIngressRequestCnt (0.00s)
=== RUN   TestRecordIngressRequestDur
--- PASS: TestRecordIngressRequestDur (0.00s)
PASS
ok  	github.com/adobe/cluster-registry/pkg/monitoring/apiserver	1.573s
=== RUN   TestNewMetrics
--- PASS: TestNewMetrics (0.00s)
=== RUN   TestInit
--- PASS: TestInit (0.00s)
=== RUN   TestRecordEgressRequestCnt
--- PASS: TestRecordEgressRequestCnt (0.00s)
=== RUN   TestRecordEgressRequestDur
--- PASS: TestRecordEgressRequestDur (0.00s)
PASS
ok  	github.com/adobe/cluster-registry/pkg/monitoring/client	2.080s
=== RUN   TestNewMetrics
--- PASS: TestNewMetrics (0.00s)
=== RUN   TestInit
--- PASS: TestInit (0.00s)
=== RUN   TestRecordRequeueCnt
--- PASS: TestRecordRequeueCnt (0.00s)
=== RUN   TestRecordReconciliationCnt
--- PASS: TestRecordReconciliationCnt (0.00s)
=== RUN   TestRecordReconciliationDur
--- PASS: TestRecordReconciliationDur (0.00s)
=== RUN   TestRecordEnqueueCnt
--- PASS: TestRecordEnqueueCnt (0.00s)
=== RUN   TestRecordEnqueueDur
--- PASS: TestRecordEnqueueDur (0.00s)
=== RUN   TestRecordErrCnt
--- PASS: TestRecordErrCnt (0.00s)
PASS
ok  	github.com/adobe/cluster-registry/pkg/monitoring/manager	2.588s

What issues are you seeing when running these tests? And what do you mean by "seem to run far more often than they should, and they don't seem to run under the correct package"?

@evanfoster
Copy link
Contributor Author

@aalexandru I just thought i was seeing my tests run more frequently than I thought they should have. I'm not very good at parsing the output of Go's unit testing, so it's probably just a brain fart on my part.

@aalexandru aalexandru self-requested a review September 11, 2024 10:40
@aalexandru aalexandru merged commit e0348b1 into adobe:main Sep 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants