Refactor piped metrics exporter #2177

khanhtc1202 · 2021-07-05T05:41:10Z

What this PR does / why we need it:

Introducing k8s cloudprovider metrics and k8s livestatestore metrics package

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics/metrics.go

pkg/app/piped/livestatestore/kubernetes/kubernetesmetrics/metrics.go

pipecd-bot · 2021-07-05T05:48:19Z

Code coverage for golang is 32.73%. This pull request increases coverage by 0.30%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/kubernetes/diff.go	DiffListResult.NoChange	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/diff.go	Diff	`--`	`100.00%`	`+100.00%`
pkg/app/piped/cloudprovider/kubernetes/diff.go	DiffList	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/diff.go	DiffListResult.DiffString	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/diff.go	groupManifests	`--`	`92.00%`	`+92.00%`
pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics/metrics.go	IncKubectlCallsCounter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics/metrics.go	Register	`--`	`0.00%`	`+0.00%`
pkg/app/piped/livestatestore/kubernetes/kubernetesmetrics/metrics.go	Register	`--`	`0.00%`	`+0.00%`
pkg/app/piped/livestatestore/kubernetes/kubernetesmetrics/metrics.go	requestResultCollector.Increment	`--`	`0.00%`	`+0.00%`
pkg/app/piped/livestatestore/kubernetes/kubernetesmetrics/metrics.go	IncResourceEventsCounter	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/kubernetes.go	init	`100.00%`	`--`	`-100.00%`
pkg/app/piped/cloudprovider/kubernetes/manifest.go	Diff	`100.00%`	`--`	`-100.00%`
pkg/app/piped/cloudprovider/kubernetes/metrics.go	metricsKubectlCalled	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/cloudprovider/kubernetes/metrics.go	registerMetrics	`100.00%`	`--`	`-100.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	NewDetector	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.Run	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.check	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.checkApplication	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.loadHeadManifests	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.listGroupedApplication	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.loadDeploymentConfiguration	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	detector.ProviderName	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	groupManifests	`92.00%`	`--`	`-92.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	makeSyncedState	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	makeOutOfSyncState	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/driftdetector/kubernetes/detector.go	filterIgnoringManifests	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/livestatestore/kubernetes/kubernetes.go	init	`100.00%`	`--`	`-100.00%`
pkg/app/piped/livestatestore/kubernetes/metrics.go	registerMetrics	`100.00%`	`--`	`-100.00%`
pkg/app/piped/livestatestore/kubernetes/metrics.go	requestResultCollector.Increment	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/livestatestore/kubernetes/metrics.go	incrementResourceEventCounter	`0.00%`	`--`	`+-0.00%`

nghialv · 2021-07-05T06:12:05Z

pkg/app/piped/cloudprovider/kubernetes/kubernetes.go

-func init() {
-	registerMetrics()
-}
-


nghialv · 2021-07-05T06:17:52Z

Nice. Thank you.
/lgtm

nakabonne · 2021-07-05T06:24:36Z

nits: how about:

pkg/app/piped/livestatestore/kubernetes/kubernetesmetrics -> pkg/app/piped/livestatestore/kubernetes/metrics
pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics -> pkg/app/piped/cloudprovider/kubernetes/metrics

I feel a bit redundant

khanhtc1202 · 2021-07-05T06:39:36Z

nits: how about:

pkg/app/piped/livestatestore/kubernetes/kubernetesmetrics -> pkg/app/piped/livestatestore/kubernetes/metrics

pkg/app/piped/cloudprovider/kubernetes/kubernetesmetrics -> pkg/app/piped/cloudprovider/kubernetes/metrics

I feel a bit redundant

I get your point, just felt the same before 😅 But since we have cachemetrics already, I think it's better to have a kind of convention: xxxmetrics package means metrics for xxx (parent package) will be implemented here. How do you think? 🤔

nghialv · 2021-07-05T06:41:54Z

I prefer to keep that convention. The metrics package name will be parent-package-name + metrics.
This is a special case when 2 parent-package-name have the same name. (pkg/app/piped/cloudprovider/kubernetes, pkg/app/piped/livestatestore/kubernetes), but in most cases, this convention helps avoid the need for aliases while importing.

This convention is the same for our test, mock package, redistest, redismock...
and standard go packages are using the same convention for httptest, httputil inside http package.

nakabonne · 2021-07-05T07:00:38Z

makes sense. no objection to that 👍
/approve

pipecd-bot · 2021-07-05T07:00:41Z

This pull request is APPROVED by nakabonne.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

nghialv · 2021-07-05T08:25:18Z

/changelog

pipecd-bot · 2021-07-05T08:25:21Z

@nghialv: Changelog has been generated in response to this comment.

Details

Changelog since v0.10.2

Notable Changes

Fix wrong focus on STAGE_NOT_STARTED_YET stage in deployment detail page (#2150)
Fix a bug that can't register multiple events during a single fetch interval (#2144)

Internal Changes

Refactor piped metrics exporter (#2177)
Make planpreview's builder shows detailed results (#2176)
Update ecs examples to make it works with fargate as default (#2178)
Refactor deploysource to use fmt.Fprintf directly and add missing newlines (#2175)
Move manifest comparison part to provider package to be reusable (#2174)
Fix a typo in envoy config (#2173)
Fix typo in envoy config (#2172)
Add sync-strategy field to output of planner (#2171)
Re-enable to track request response size statistics (#2170)
Update envoy to v0.18.3 (#2169)
Update planpreview's builder to detect which apps should be triggered (#2165)
Add inmemory cache metrics (#2166)
Add relabel config for kube-state-metrics (#2163)
Revert "Enable to track request response size statistics (Enable to track request response size statistics #2160)" (#2162)
Make get operation of env-store to be cancelable and single-flight (#2161)
Enable to track request response size statistics (#2160)
Refactor components to supress unnecessary rendering (#2159)
Add Prometheus relabeling config to manipulate metrics from cAdvisor (#2158)
Refactor cachemetrics interface and label name (#2157)
Introducing cachemetrics package (#2156)
Enable Node exporter and Kube state metrics (#2155)
Make pipectl plan-preview command renders the results to stdout and file (#2154)
Update contributor list (#2153)
Fix wrong name of a terraform example application (#2151)
Add Prometheus jobs to fetch k8s resource metrics (#2149)
Fix: obsolete terraform flags (#2138)
Add release panel to ja docs (#2148)
Fix cut index on getting versioning information (#2147)
Implement planpreview handler (#2143)
Update docs cover (#2141)
Fix: gnu sed support (#2139)
Fix wrong release filetype (#2142)
Fix wrong link to secret management in feature status page (#2140)

khanhtc1202 added 3 commits July 5, 2021 12:42

Introducing cloudprovider kubernetesmetrics package

00808e0

rename cloudprovider k8smetrics key

2a16d5d

Introducing livestatestore metrics package

b64d0bf

pipecd-bot added release-note-none size/L area/piped labels Jul 5, 2021

pipecd-bot requested review from nakabonne and nghialv July 5, 2021 05:41

pipecd-bot reviewed Jul 5, 2021

View reviewed changes

nghialv reviewed Jul 5, 2021

View reviewed changes

pkg/app/piped/cloudprovider/kubernetes/kubernetes.go

func init() {

registerMetrics()

}

Copy link

Member

nghialv Jul 5, 2021

Choose a reason for hiding this comment

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

Nice.

pipecd-bot added the lgtm label Jul 5, 2021

pipecd-bot added the approved label Jul 5, 2021

pipecd-bot merged commit d66b743 into master Jul 5, 2021

pipecd-bot deleted the refactor-piped-metrics-exporter branch July 5, 2021 07:09

Refactor piped metrics exporter #2177

Refactor piped metrics exporter #2177

Uh oh!

Conversation

khanhtc1202 commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pipecd-bot commented Jul 5, 2021

Uh oh!

nghialv Jul 5, 2021

Choose a reason for hiding this comment

Uh oh!

nghialv commented Jul 5, 2021

Uh oh!

nakabonne commented Jul 5, 2021

Uh oh!

khanhtc1202 commented Jul 5, 2021

Uh oh!

nghialv commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nakabonne commented Jul 5, 2021

Uh oh!

pipecd-bot commented Jul 5, 2021

Uh oh!

nghialv commented Jul 5, 2021

Uh oh!

pipecd-bot commented Jul 5, 2021

Changelog since v0.10.2

Notable Changes

Internal Changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

khanhtc1202 commented Jul 5, 2021 •

edited

Loading

nghialv commented Jul 5, 2021 •

edited

Loading