Expose piped stat metrics at ops metrics endpoint #2202

khanhtc1202 · 2021-07-08T04:18:50Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2021-07-08T04:18:52Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Implement builder to gather piped stats metrics in redis.

https://github.com/pipe-cd/pipe/blob/78d42ddc18c3135fbd299de069ee990aac18ffc4/pkg/app/ops/pipedstatsbuilder/builder.go#L30-L33

This was created by todo plugin since "TODO:" was found in `78d42dd` when #2202 was merged. cc: @khanhtc1202.

pipecd-bot

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

pkg/app/ops/pipedstatsbuilder/builder.go

pipecd-bot · 2021-07-08T04:25:49Z

Code coverage for golang is 32.03%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/cli/cmd.go	Telemetry.CustomedMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

pipecd-bot · 2021-07-08T05:28:49Z

Code coverage for golang is 32.03%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/cli/cmd.go	Telemetry.CustomedMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

nghialv · 2021-07-08T06:30:31Z

cmd/pipecd/ops.go

 	cmd.Flags().DurationVar(&s.gracePeriod, "grace-period", s.gracePeriod, "How long to wait for graceful shutdown.")
 	cmd.Flags().StringVar(&s.configFile, "config-file", s.configFile, "The path to the configuration file.")
 	cmd.Flags().StringVar(&s.gcloudPath, "gcloud-path", s.gcloudPath, "The path to the gcloud command executable.")
+	cmd.Flags().StringVar(&s.cacheAddress, "cache-address", s.cacheAddress, "The address to cache service.")


Ops' deployment need this too.
https://github.com/pipe-cd/pipe/blob/master/manifests/pipecd/templates/deployment.yaml#L96

Nice catch 👍
Address by b967765 🙏

khanhtc1202 · 2021-07-08T06:32:10Z

cmd/pipecd/ops.go

 			w.Write([]byte("ok"))
 		})
-		admin.Handle("/metrics", t.PrometheusMetricsHandler())
+		admin.Handle("/metrics", t.CustomedMetricsHandlerFor(psb))


with this customed metrics handler, we ignore all other metrics exposed to the current ops metrics registry ( which currently is zero ). Will carefully handle those ops's metrics later 🙏

nit: I feel the name should be -> CustomMetricsHandlerFor

pipecd-bot · 2021-07-08T06:46:34Z

Code coverage for golang is 32.03%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/cli/cmd.go	Telemetry.CustomedMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-07-08T07:07:54Z

pkg/app/ops/pipedstatsbuilder/builder.go

+		}
+		data = append(data, value)
+	}
+	return ioutil.NopCloser(bytes.NewReader(bytes.Join(data, []byte("\n")))), nil


iouti.NopClose is deprecated. Instead using io.NopCloser is preferable.
https://golang.org/pkg/io/ioutil/#NopCloser

Well, why does it need to be an io.ReadCloser? Seems like io.Reader is enough.

👀 Just want to carefully close the []byte stream on the hander side. Or is it unnecessary? 🤔
ref: https://github.com/pipe-cd/pipe/pull/2202/files#diff-c9b9fc9a691a60f03cc1d752ce9fac11e932d6a2867c984bd26923131704fdeaR152

First, NopCloser.Close does nothing.

And with the current implementation of PipedStatsBuilder.Build(), all byte sequences are on the heap. So no need to do close operations at all.

If you're planning to directly read from any kinds of stream like Redis in the future, then you will have to add the close operation.

I see 👍 Since we will keep the GetAll interface which returns data ( not the reader or else to direct stream data ), it's safe to remove this unnecessary closer as you mentioned 🙏 Lets me address it here 🙆‍♀️

Addressed on d448919 🙏

pipecd-bot · 2021-07-08T07:18:49Z

Code coverage for golang is 32.03%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/cli/cmd.go	Telemetry.CustomMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-07-08T07:34:36Z

pkg/app/ops/pipedstatsbuilder/builder.go

+		}
+		data = append(data, value)
+	}
+	return bytes.NewReader(bytes.Join(data, []byte("\n"))), nil


it's alright to be later, adding a test for Build() makes it more clear.

Sure, just added a simple test 6cad0a6 👀

pipecd-bot · 2021-07-08T07:36:34Z

Code coverage for golang is 32.03%. This pull request decreases coverage by -0.03%.

File	Function	Base	Head	Diff
pkg/cli/cmd.go	Telemetry.CustomMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

nghialv · 2021-07-08T08:28:33Z

pkg/app/ops/pipedstatsbuilder/metricsdata/expected

@@ -0,0 +1,180 @@
+# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.


metricsdata -> testdata

nghialv · 2021-07-08T08:29:23Z

pkg/app/ops/pipedstatsbuilder/builder.go

+		}
+		data = append(data, value)
+	}
+	return bytes.NewReader(bytes.Join(data, []byte(""))), nil


Use \n as the separator is safer for all cases.

btw, an empty line between metrics is fine.

got it 🙆‍♀️

pipecd-bot · 2021-07-08T08:42:05Z

Code coverage for golang is 32.07%. This pull request increases coverage by 0.01%.

File	Function	Base	Head	Diff
pkg/app/ops/pipedstatsbuilder/builder.go	NewPipedStatsBuilder	`--`	`100.00%`	`+100.00%`
pkg/app/ops/pipedstatsbuilder/builder.go	PipedStatsBuilder.Build	`--`	`61.54%`	`+61.54%`
pkg/cli/cmd.go	Telemetry.CustomMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-07-08T09:12:30Z

pkg/app/ops/pipedstatsbuilder/builder_test.go

+func (m *mockBuilderBackend) GetAll() (map[string]interface{}, error) {
+	out := make(map[string]interface{}, len(m.srcs))
+	for _, file := range m.srcs {
+		data, err := ioutil.ReadFile(file)


ioutil package is entirely deprecated. Let's use os.ReadFile
https://golang.org/pkg/io/ioutil/#ReadFile

Lets no ioutil, lol
028e358

nakabonne · 2021-07-08T09:17:58Z

pkg/app/ops/pipedstatsbuilder/builder_test.go

+	assert.NoError(t, err)
+	assert.NotNil(t, rc)


require package would be better because it stops test execution when it fails.

To be honest, do we have convention of which check should be wrapped with require and which should be with assert 👀 I saw the require is used somewhere but mixed with assert 👀

addressed on ac20e18

In the case of nil and error, it's more likely to panic if proceeds. So we basically use require for NoError and NotNil.

pipecd-bot · 2021-07-08T09:23:34Z

Code coverage for golang is 32.07%. This pull request increases coverage by 0.01%.

File	Function	Base	Head	Diff
pkg/app/ops/pipedstatsbuilder/builder.go	NewPipedStatsBuilder	`--`	`100.00%`	`+100.00%`
pkg/app/ops/pipedstatsbuilder/builder.go	PipedStatsBuilder.Build	`--`	`61.54%`	`+61.54%`
pkg/cli/cmd.go	Telemetry.CustomMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

pipecd-bot · 2021-07-08T09:32:49Z

Code coverage for golang is 32.07%. This pull request increases coverage by 0.01%.

File	Function	Base	Head	Diff
pkg/app/ops/pipedstatsbuilder/builder.go	NewPipedStatsBuilder	`--`	`100.00%`	`+100.00%`
pkg/app/ops/pipedstatsbuilder/builder.go	PipedStatsBuilder.Build	`--`	`61.54%`	`+61.54%`
pkg/cli/cmd.go	Telemetry.CustomMetricsHandlerFor	`--`	`0.00%`	`+0.00%`

nakabonne · 2021-07-08T10:08:10Z

Great work.
/lgtm

pipecd-bot · 2021-07-08T10:18:31Z

This pull request is APPROVED by nghialv.

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.

Expose piped stat metrics

78d42dd

pipecd-bot added the release-note-none label Jul 8, 2021

pipecd-bot added area/ops do-not-merge/wip area/server labels Jul 8, 2021

pipecd-bot requested review from nakabonne and nghialv July 8, 2021 04:18

pipecd-bot reviewed Jul 8, 2021

View reviewed changes

pkg/app/ops/pipedstatsbuilder/builder.go Outdated Show resolved Hide resolved

Implement simple build piped stat logic

21e0fa2

pipecd-bot added size/L and removed size/M labels Jul 8, 2021

khanhtc1202 changed the title ~~WIP Expose piped stat metrics on ops metrics endpoint~~ Expose piped stat metrics on ops metrics endpoint Jul 8, 2021

pipecd-bot removed the do-not-merge/wip label Jul 8, 2021

khanhtc1202 changed the title ~~Expose piped stat metrics on ops metrics endpoint~~ Expose piped stat metrics at ops metrics endpoint Jul 8, 2021

nghialv reviewed Jul 8, 2021

View reviewed changes

khanhtc1202 commented Jul 8, 2021

View reviewed changes

Add cache address to ops deplopment manifest

b967765

pipecd-bot added the area/manifests label Jul 8, 2021

nakabonne reviewed Jul 8, 2021

View reviewed changes

khanhtc1202 added 2 commits July 8, 2021 16:09

Rename function

1847b7e

Using io.NopCloser

b61b053

Using io.Reader instead

d448919

nakabonne reviewed Jul 8, 2021

View reviewed changes

Add builder test

6cad0a6

pipecd-bot added size/XL and removed size/L labels Jul 8, 2021

nghialv reviewed Jul 8, 2021

View reviewed changes

Update testdata

2f1a89d

nakabonne reviewed Jul 8, 2021

View reviewed changes

Remove ioutil

028e358

nakabonne reviewed Jul 8, 2021

View reviewed changes

Using require

ac20e18

pipecd-bot added the lgtm label Jul 8, 2021

nghialv approved these changes Jul 8, 2021

View reviewed changes

pipecd-bot added the approved label Jul 8, 2021

pipecd-bot merged commit 654dee3 into master Jul 8, 2021

pipecd-bot deleted the expose-piped-stat-metrics branch July 8, 2021 10:18

khanhtc1202 mentioned this pull request Jul 8, 2021

Returns ops's metrics along with the pipeds' metrics at ops admin.metrics #2205

Closed

This was referenced Jul 12, 2021

Fix bug that all plan-preview commands that come after Piped has been running for 5 minutes will not be handled #2222

Merged

Add metrics for plan-preview component #2223

Merged

		@@ -0,0 +1,180 @@
		# HELP go_gc_duration_seconds A summary of the pause duration of garbage collection cycles.

Expose piped stat metrics at ops metrics endpoint #2202

Expose piped stat metrics at ops metrics endpoint #2202

Uh oh!

Conversation

khanhtc1202 commented Jul 8, 2021

Uh oh!

pipecd-bot commented Jul 8, 2021

1. Implement builder to gather piped stats metrics in redis.

This was created by todo plugin since "TODO:" was found in 78d42dd when #2202 was merged. cc: @khanhtc1202.

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jul 8, 2021

Uh oh!

This was created by todo plugin since "TODO:" was found in `78d42dd` when #2202 was merged. cc: @khanhtc1202.

khanhtc1202 Jul 8, 2021 •

edited

Loading

nakabonne Jul 8, 2021 •

edited

Loading

khanhtc1202 Jul 8, 2021 •

edited

Loading