Introducing ReportStat service to pipedservice #2181

khanhtc1202 · 2021-07-05T11:53:02Z

What this PR does / why we need it:

Add pipedID as label of piped's metrics
Replace Ping service with ReportStat service

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

…rolplane

pipecd-bot

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

pkg/app/api/grpcapi/piped_api.go

pkg/app/api/service/pipedservice/pipedclientfake/fakeclient.go

pipecd-bot · 2021-07-05T12:00:04Z

Code coverage for golang is 32.24%. This pull request decreases coverage by -0.10%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ReportStat	`--`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.Run	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.report	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.collect	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/statsreporter/reporter.go	parsePrometheusMetrics	`95.45%`	`--`	`-95.45%`

nghialv · 2021-07-06T00:42:54Z

pkg/app/api/grpcapi/piped_api.go


 // Ping is periodically sent to report its realtime status/stats to control-plane.
 // The received stats will be pushed to the metrics collector.
+// Note: This service is deprecated, use ReportStat instead.


nit: "This rpc"

nghialv · 2021-07-06T00:49:50Z

pkg/app/piped/cmd/piped/piped.go

+	// TODO: Add piped version as label.
+	prometheus.WrapRegistererWith(
+		prometheus.Labels{"piped": pipedID},
+		r,


Nice. One thing we have to update is fixing cli to use this wrapped register instead of the default one.
https://github.com/pipe-cd/pipe/blob/master/pkg/cli/cmd.go#L121

But that can be done in another PR.

Nice catch, lets me address another missing in this PR 🙏
ref: 0eb829f

pipecd-bot · 2021-07-06T01:19:49Z

Code coverage for golang is 32.24%. This pull request decreases coverage by -0.10%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ReportStat	`--`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.Run	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.report	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.collect	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/statsreporter/reporter.go	parsePrometheusMetrics	`95.45%`	`--`	`-95.45%`

nghialv · 2021-07-06T01:22:43Z

pkg/app/api/service/pipedservice/service.proto

+}
+
+message ReportStatResponse {
    int64 ping_interval = 1;


nit: report_interval

addressed by 2bb3bcb 🙏

nghialv · 2021-07-06T01:24:19Z

Nice. I have just left a nit.
/lgtm

nakabonne · 2021-07-06T01:31:05Z

pkg/app/api/service/pipedservice/service.proto

+}
+
+message ReportStatRequest {
+    bytes piped_stats = 1 [(validate.rules).bytes.min_len = 1];


Nice. For those who look at this first will be more likely to wonder why it's represented as a byte sequence. I feel like it should be commented like: "Metrics byte sequence in OpenMetrics format"

👍 Lets me adopt your idea, thx 🙆‍♂️

addressed by 2bb3bcb 🙏

pipecd-bot · 2021-07-06T01:51:03Z

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. Add piped version as label.

https://github.com/pipe-cd/pipe/blob/2bb3bcb399c6d2f09f3bc0b8417038270b8829b3/pkg/app/piped/cmd/piped/piped.go#L621-L624

This was created by todo plugin since "TODO:" was found in `2bb3bcb` when #2181 was merged. cc: @khanhtc1202.

nakabonne · 2021-07-06T01:51:37Z

/approve

pipecd-bot · 2021-07-06T01:51:40Z

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.

pipecd-bot · 2021-07-06T01:59:19Z

Code coverage for golang is 32.24%. This pull request decreases coverage by -0.10%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/piped_api.go	PipedAPI.ReportStat	`--`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.report	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.Run	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/statsreporter/reporter.go	reporter.collect	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/statsreporter/reporter.go	parsePrometheusMetrics	`95.45%`	`--`	`-95.45%`

**What this PR does / why we need it**: - address #2181 (comment) - fix #2182 - remove unnecessary validation in ReportStatRequest message. We should always allow the report stat to be push to the control-plane, the validation better is handled by control-plane. **Which issue(s) this PR fixes**: Fixes #2182 **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

khanhtc1202 added 3 commits July 5, 2021 19:24

Add pipedID as piped metrics label

a1cf903

Add ReportStat service to pipedservice

3c2a96e

Replace Ping request by ReportStat request to push piped stat to cont…

67f6614

…rolplane

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

pipecd-bot requested review from nakabonne and nghialv July 5, 2021 11:53

pipecd-bot added the size/L label Jul 5, 2021

pipecd-bot reviewed Jul 5, 2021

View reviewed changes

nghialv reviewed Jul 6, 2021

View reviewed changes

Fix comments

0eb829f

nghialv reviewed Jul 6, 2021

View reviewed changes

pipecd-bot added the lgtm label Jul 6, 2021

nakabonne reviewed Jul 6, 2021

View reviewed changes

Rename message field

2bb3bcb

pipecd-bot removed the lgtm label Jul 6, 2021

pipecd-bot added the approved label Jul 6, 2021

pipecd-bot merged commit dfb10de into master Jul 6, 2021

pipecd-bot deleted the add-piped-label branch July 6, 2021 01:59

pipecd-bot mentioned this pull request Jul 6, 2021

Add piped version as label. #2182

Closed

khanhtc1202 mentioned this pull request Jul 6, 2021

Use custom piped metrics registry instead of the default one #2183

Merged

pipecd-bot mentioned this pull request Jul 7, 2021

Explicitly use string as type for key in cache package #2200

Merged

Introducing ReportStat service to pipedservice #2181

Introducing ReportStat service to pipedservice #2181

Uh oh!

Conversation

khanhtc1202 commented Jul 5, 2021

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pipecd-bot commented Jul 5, 2021

Uh oh!

nghialv Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

nghialv Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Jul 6, 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!

pipecd-bot commented Jul 6, 2021

Uh oh!

nghialv Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

nghialv commented Jul 6, 2021

Uh oh!

nakabonne Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Jul 6, 2021

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jul 6, 2021

1. Add piped version as label.

This was created by todo plugin since "TODO:" was found in 2bb3bcb when #2181 was merged. cc: @khanhtc1202.

Uh oh!

nakabonne commented Jul 6, 2021

Uh oh!

pipecd-bot commented Jul 6, 2021

Uh oh!

pipecd-bot commented Jul 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

khanhtc1202 Jul 6, 2021 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `2bb3bcb` when #2181 was merged. cc: @khanhtc1202.