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

Expose metrics to report time taken in fetch/template/deploy phase of app, pkgi, pkgr #1415

Merged
merged 9 commits into from
Jan 27, 2024
Merged

Expose metrics to report time taken in fetch/template/deploy phase of app, pkgi, pkgr #1415

merged 9 commits into from
Jan 27, 2024

Conversation

sethiyash
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?

Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:

@sethiyash
Added gauge metrics to record reconcile time for app and pkg repo
f0e42c0 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
@sethiyash
gaugevec metrics for pkg repo, app, pkg install
09d6cd0 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
prembhaskal
prembhaskal reviewed Dec 6, 2023
View reviewed changes
pkg/app/app.go Outdated Show resolved Hide resolved
prembhaskal
prembhaskal reviewed Dec 6, 2023
View reviewed changes
Copy link
Member

@prembhaskal prembhaskal left a comment

Choose a reason for hiding this comment

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

inline comments.

@sethiyash sethiyash force-pushed the expose-metrics branch 2 times, most recently from 00093da to 196bc45 Compare December 7, 2023 17:59
@sethiyash
final refactoring
1631608 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
rohitagg2020
rohitagg2020 reviewed Dec 11, 2023
View reviewed changes
pkg/app/app_factory.go Outdated Show resolved Hide resolved
pkg/app/app_fetch.go Outdated Show resolved Hide resolved
pkg/app/app_reconcile.go Outdated Show resolved Hide resolved
pkg/pkgrepository/crd_app.go Outdated Show resolved Hide resolved
pkg/pkgrepository/app_reconcile_test.go Outdated Show resolved Hide resolved
@rohitagg2020
Copy link
Contributor

Import statements in many files are not as per our convention. Please check them.

@sethiyash
resolved the nits
c871303 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
prembhaskal
prembhaskal approved these changes Dec 13, 2023
View reviewed changes
Copy link
Member

@prembhaskal prembhaskal left a comment

Choose a reason for hiding this comment

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

LGTM

@kumaritanushree
Copy link
Contributor

LGTM

praveenrewar
praveenrewar reviewed Dec 20, 2023
View reviewed changes
cmd/controller/run.go Outdated Show resolved Hide resolved
@sethiyash
having single struct for both metrics
0174469 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
praveenrewar
praveenrewar reviewed Dec 21, 2023
View reviewed changes
cmd/controller/run.go Outdated Show resolved Hide resolved
pkg/app/app.go Outdated Show resolved Hide resolved
pkg/app/app_reconcile.go Outdated
// Reconcile is not expected to be called concurrently
func (a *App) Reconcile(force bool) (reconcile.Result, error) {
defer a.flushUpdateStatus("app reconciled")

var err error

a.appMetrics.InitMetrics(a.Name(), a.Namespace())
a.appMetrics.ReconcileCountMetrics.InitMetrics(appResourceType, a.Name(), a.Namespace())
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a.Name() itself? Or maybe Kind() if we don't want to use name.
Same for package install and package repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no a.Kind() available here

Copy link
Contributor

@neil-hickey neil-hickey Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The kind for an app shouldn't be a constant in a reconciler. It should live with the object model.

In this case, our app struct probably ought to have another function return the kind, rather than a constant.

I think @praveenrewar 's original comment is my thinking also. Just because the data we need isn't there - doesn't mean it can't be. We need it to be, so let's just make it available.

pkg/app/app_deploy.go Outdated Show resolved Hide resolved
pkg/app/app_fetch.go Outdated Show resolved Hide resolved
pkg/app/app_reconcile.go Outdated Show resolved Hide resolved
pkg/app/crd_app.go Outdated Show resolved Hide resolved
@sethiyash
Using name namespace methods and refactor imports
621f947 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
praveenrewar
praveenrewar reviewed Jan 4, 2024
View reviewed changes
config/config/deployment.yml Outdated Show resolved Hide resolved
pkg/app/app_reconcile_test.go Outdated Show resolved Hide resolved
@sethiyash
making metrics port configurable and constructor for metrics
68d3e5d 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
praveenrewar
praveenrewar reviewed Jan 8, 2024
View reviewed changes
cmd/controller/run.go Show resolved Hide resolved
pkg/app/app_reconcile_test.go Show resolved Hide resolved
praveenrewar
praveenrewar approved these changes Jan 8, 2024
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM!

neil-hickey
neil-hickey reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@neil-hickey neil-hickey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall LGTM, I added a few nit things. And one suggestion: In our tests we do this pattern of metrics.New() everywhere, which is kinda silly because it's never used and we allocate all this memory to build the structures. It's just test code so it's not the end of the world, just something to consider / think about.

One concern is that we don't have any tests for this stuff at all (in this PR it seems). Given that the code is mostly registering metrics, it likely ought to be an e2e test. Even a simple one to just validate the a small sample of metrics is actually exposed / collected.

How do we know it works? :) And how do I as the reviewer?

pkg/app/app_reconcile.go Outdated Show resolved Hide resolved
pkg/app/app_reconcile.go Outdated Show resolved Hide resolved
pkg/pkgrepository/app_reconcile.go Show resolved Hide resolved
pkg/pkgrepository/app_reconcile.go Show resolved Hide resolved
pkg/pkgrepository/app_reconcile.go Outdated Show resolved Hide resolved
pkg/metrics/reconcile_count_metrics.go Outdated Show resolved Hide resolved
pkg/metrics/reconcile_count_metrics.go Outdated Show resolved Hide resolved
pkg/packageinstall/packageinstall.go Outdated Show resolved Hide resolved
@sethiyash sethiyash force-pushed the expose-metrics branch 8 times, most recently from a81a775 to 301d354 Compare January 17, 2024 05:25
@sethiyash
Added E2E testcases for exposing prometheus metrics
f9b21d3 
Signed-off-by: sethiyash <yashsethiya97@gmail.com>
Signed-off-by: Yash Sethiya <ysethiya@ysethiya4MD6M.vmware.com>
praveenrewar
praveenrewar approved these changes Jan 24, 2024
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM (Apart from the 2 nits)

test/e2e/kappcontroller/metrics_test.go Show resolved Hide resolved
test/e2e/kappcontroller/metrics_test.go Outdated Show resolved Hide resolved
Updated CR names in kind method
b292117 
Signed-off-by: Yash Sethiya <ysethiya@ysethiya4MD6M.vmware.com>
@sethiyash sethiyash merged commit e82ab40 into develop Jan 27, 2024
10 checks passed
@praveenrewar praveenrewar deleted the expose-metrics branch February 5, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitagg2020 rohitagg2020 rohitagg2020 left review comments

@neil-hickey neil-hickey neil-hickey left review comments

@praveenrewar praveenrewar praveenrewar approved these changes

@prembhaskal prembhaskal prembhaskal approved these changes

@100mik 100mik Awaiting requested review from 100mik

@joaopapereira joaopapereira Awaiting requested review from joaopapereira

Assignees
No one assigned
Labels
None yet
Projects
Carvel
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sethiyash @rohitagg2020 @kumaritanushree @prembhaskal @neil-hickey @praveenrewar