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.

Sign up for GitHub

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

Jump to bottom

Add metrics to monitor reconcile status #346

Merged
merged 26 commits into from Sep 9, 2021
Merged

Add metrics to monitor reconcile status #346

merged 26 commits into from Sep 9, 2021

Conversation

mhoshi-vm
Copy link
Contributor

Feature Request for #337.
Add additional counter metrics to count the number reconciles with failures also.

  • Metrics will get initialised to all 0 during reconcile
  • The counter increments when status changes to "Reconciling" > kapp_reconcile_attempt_total
  • The counter increments when status changes to "Reconcile Succeed" > kapp_reconcile_success_total
  • The counter increments when status changes to "Reconcile Failed" > kapp_reconcile_failure_total
  • The counter increments when status changes to "Deleting" > kapp_reconcile_delete_attempt_total
  • The counter increments when status changes to "Delete Failed" > kapp_reconcile_delete_failed_total
  • The metrics get deleted when the delete completes. This is to avoid metrics existing forever while the real app has been deleted.

@mhoshi-vm mhoshi-vm changed the title Sbp metrics 2 Add metrics to monitor reconcile status Aug 23, 2021
cppforlife
cppforlife reviewed Aug 24, 2021
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
cppforlife
cppforlife reviewed Aug 27, 2021
View reviewed changes
Copy link
Contributor

@cppforlife cppforlife left a comment

Choose a reason for hiding this comment

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

starting to look really good!

cmd/controller/app_factory.go Outdated Show resolved Hide resolved
cmd/controller/app_factory.go Outdated Show resolved Hide resolved
cmd/controller/apps_reconciler.go Outdated Show resolved Hide resolved
cmd/controller/apps_reconciler.go Outdated Show resolved Hide resolved
cmd/controller/run.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@mhoshi-vm
* Fix app factory to construct metrics
5cd11b3 
* Move init metrics to reconcile in CRDApps
* Fix new lines in import
* Fix metrics naming to ServMetrics to AppMetrics
* Fix naming convention of metrics
@mhoshi-vm
Merge remote-tracking branch 'upstream/develop' into sbp-metrics-2
6492ce2
@mhoshi-vm
Update test code
ba4c625
@mhoshi-vm
Update test code
aec223a
cppforlife
cppforlife reviewed Sep 2, 2021
View reviewed changes
pkg/app/crd_app.go Outdated
@@ -6,6 +6,7 @@ package app
import (
"context"
"fmt"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/metrics"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move into next import subsection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

cppforlife
cppforlife reviewed Sep 2, 2021
View reviewed changes
pkg/metrics/app_metrics.go Outdated
)

// NewServerMetrics creates AppMetrics object
func NewServerMetrics() *AppMetrics {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: NewServerMetrics -> NewAppMetrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

cppforlife
cppforlife reviewed Sep 2, 2021
View reviewed changes
pkg/metrics/app_metrics.go Outdated

// NewServerMetrics creates AppMetrics object
func NewServerMetrics() *AppMetrics {
metricNamespace := "kappctrl"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can actually use const within functions as well e.g.

const (
	metricNamespace = "kappctrl"
	kappNameLabel = "app_name"
	kappNamespaceLabel = "namespace"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@cppforlife
Copy link
Contributor

might have to rebase...

cppforlife
cppforlife reviewed Sep 2, 2021
View reviewed changes
pkg/app/crd_app.go Outdated Show resolved Hide resolved
@cppforlife
Copy link
Contributor

super close @mhoshi-vm!

@mhoshi-vm
Copy link
Contributor Author

I should have asked at the beginning but do I need to implement some test code ?

@mhoshi-vm
Copy link
Contributor Author

I should have asked at the beginning but do I need to implement some test code ?

Guessing I need to build something like the following.
Would very appreciate if I can get some guide (where I should place the test code) in the current kapp controller structure
https://github.com/antrea-io/antrea/blob/4aaa90edf1f2d8b70282f55511b860765c342801/test/e2e/prometheus_test.go

@cppforlife cppforlife merged commit 22299af into carvel-dev:develop Sep 9, 2021
@cppforlife
Copy link
Contributor

@mhoshi-vm thanks for sticking with it! 🔥🔥🔥

@github-actions github-actions bot added the carvel-triage This issue has not yet been reviewed for validity label Sep 9, 2021
@aaronshurley aaronshurley removed the carvel-triage This issue has not yet been reviewed for validity label Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cppforlife cppforlife cppforlife left review comments

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mhoshi-vm @cppforlife @aaronshurley @vmwclabot