Add a definition of function for the deployment frequency graph #1179

sanposhiho · 2020-12-02T07:38:07Z

What this PR does / why we need it:

Add a definition of function to pass data to plot the deployment frequency graph

Which issue(s) this PR fixes:

Deployment Frequency: How often does your application/project deploy code to production.

Does this PR introduce a user-facing change?:

NONE

nakabonne · 2020-12-02T07:48:48Z

The commit message (as well as the PR title) should start with an uppercase letter like "Add application...".

cakecatz · 2020-12-02T08:05:12Z

pkg/app/api/service/webservice/service.proto

+
+message GetApplicationDeploymentFrequencyRequest {
+    string application_id = 1 [(validate.rules).string.min_len = 1];
+    repeated int64 dates = 2;


@sanposhiho If we want to fetch for a week data, how we use this parameter?
[startDate, endDate] or [date1, date2, date3, ..., date7] ?

[date1, date2, date3, ..., date7], so that we may want to add filter for dates(not including holidays in the graph, etc.) later.

(not including holidays in the graph, etc.)

Agree with you 👍

pipecd-bot

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

pipecd-bot · 2020-12-02T08:58:37Z

pkg/app/api/grpcapi/web_api.go

 	}, nil
 }
+
+func (a *WebAPI) GetApplicationDeploymentFrequency(ctx context.Context, req *webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {


ctx is unused in GetApplicationDeploymentFrequency

pipecd-bot · 2020-12-02T08:58:38Z

pkg/app/api/grpcapi/web_api.go

 	}, nil
 }
+
+func (a *WebAPI) GetApplicationDeploymentFrequency(ctx context.Context, req *webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {


req is unused in GetApplicationDeploymentFrequency

pipecd-bot

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

pipecd-bot · 2020-12-02T09:43:07Z

pkg/app/api/grpcapi/web_api.go

 	}, nil
 }
+
+// [WIP] Made a temporary implementation to satisfy interface


comment on exported method WebAPI.GetApplicationDeploymentFrequency should be of the form "GetApplicationDeploymentFrequency ..."

https://golang.org/wiki/CodeReviewComments#doc-comments

pipecd-bot · 2020-12-02T09:48:52Z

Code coverage for golang is 33.26%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.GetApplicationDeploymentFrequency	`--`	`0.00%`	`+0.00%`

pipecd-bot · 2020-12-02T09:54:52Z

Code coverage for golang is 33.26%. This pull request decreases coverage by -0.01%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.GetApplicationDeploymentFrequency	`--`	`0.00%`	`+0.00%`

nghialv · 2020-12-02T13:19:22Z

pkg/app/api/service/webservice/service.proto

+message GetApplicationDeploymentFrequencyResponse {
+    // key: date, value: deployment_count_of_the_day
+    map<int64, int64> deployment_counts = 1;
+}


I think we can abstract the models/definitions around this Insights functionality.
First of all, a graph can be considered as a series of data points.
So maybe only one rpc to retrieve the graph data is enough.

The web client sends a request that includes:

metrics_kind: what kind of metrics should be returned (deployment frequency, change failure rate, MTTR...)

step: the time unit (daily, weekly, monthly, yearly)

range: from when to when

filters: e.g. application id...

and the response will contain either a list of data points or an error.

Here are suggestions:

inside the model package

message InsightDataPoint { int64 timestamp float value } enum InsightMetricsKind { DEPLOYMENT_FREQUENCY CHANGE_FAILURE_RATE ... } enum InsightStep { DAILY WEEKLY ... }

inside the service package

rpc GetInsightData()... message GetInsightDataRequest{ InsightMetricsKind metrics_kind; ... } message GetInsightDataResponse { int64 updated_at; repeated InsightDataPoint data_points; }

Looks good. Some metrics kind may not be able to be represented by only timestamp and value alone, but then let's reconsider it at that time.

Good :)
fixed like above.

nakabonne · 2020-12-03T00:12:08Z

pkg/app/api/grpcapi/web_api.go

 }
+
+// GetApplicationDeploymentFrequency [WIP] Made a temporary implementation to satisfy interface
+func (a *WebAPI) GetApplicationDeploymentFrequency(ctx context.Context, req *webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {


With _, can be a clear indication of what not to use in a function.

Suggested change

func (a *WebAPI) GetApplicationDeploymentFrequency(ctx context.Context, req *webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {

func (a *WebAPI) GetApplicationDeploymentFrequency(_ context.Context, _ *webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {

pipecd-bot · 2020-12-03T00:34:22Z

Code coverage for golang is 33.27%. This pull request decreases coverage by -0.00%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.GetApplicationDeploymentFrequency	`--`	`0.00%`	`+0.00%`

nakabonne · 2020-12-03T02:57:35Z

pkg/app/api/service/webservice/service.proto

    repeated model.APIKey keys = 1;
 }
+
+message GetApplicationDeploymentFrequencyRequest {


Still is it needed? I feel like all operations can be done via GetInsightDataRequest.

Oh, I forgot removing 👍

Co-authored-by: Ryo Nakao <[email protected]>

pipecd-bot · 2020-12-03T03:45:52Z

Code coverage for golang is 33.27%. This pull request decreases coverage by -0.00%.

File	Function	Base	Head	Diff
pkg/app/api/grpcapi/web_api.go	WebAPI.GetInsightData	`--`	`0.00%`	`+0.00%`

nakabonne · 2020-12-03T05:12:45Z

/lgtm

nghialv · 2020-12-03T05:28:39Z

pkg/model/insight.proto

+enum InsightStep {
+  DAILY = 0;
+  WEEKLY = 1;
+  YEARLY = 2;


Missing the MONTHLY

nghialv · 2020-12-03T05:29:42Z

pkg/model/insight.proto

+option go_package = "github.com/pipe-cd/pipe/pkg/model";
+
+message InsightDataPoint {
+  int64 timestamp = 1;


Please add the validation for this field.

nghialv · 2020-12-03T05:30:02Z

pkg/app/api/service/webservice/service.proto

+message GetInsightDataResponse {
+    int64 updated_at = 1;
+    repeated pipe.model.InsightDataPoint data_points = 2;
+}


Please add an empty line at the end of the file.

Do we have lint for this kind of problem? Missing line at the end of the file is a regular issue. 👀

We were talking about that adding a Kapetanios plugin to perform the linter for protobuf before, but still not addressed it...

I see, thanks 🙆‍♂️

nghialv · 2020-12-03T05:30:31Z

pkg/app/api/service/webservice/service.proto

+    pipe.model.InsightStep insight_step = 3;
+    int64 first_date = 4;
+    int64 last_date = 5;
+}


Please add the validation for those fields.

nghialv · 2020-12-03T05:33:15Z

pkg/app/api/grpcapi/web_api.go

+// GetInsightData [WIP] Made a temporary implementation to satisfy interface
+func (a *WebAPI) GetInsightData(_ context.Context, _ *webservice.GetInsightDataRequest) (*webservice.GetInsightDataResponse, error) {
+	return nil, nil
+}


nit:

// GetInsightData returns the accumulated insight data. func (a *WebAPI) GetInsightData(_ context.Context, _ *webservice.GetInsightDataRequest) (*webservice.GetInsightDataResponse, error) { return nil, status.Error(codes. Unimplemented, "") }

nghialv · 2020-12-03T05:34:21Z

pkg/app/api/service/webservice/service.proto

+message GetInsightDataRequest {
+    string application_id = 1 [(validate.rules).string.min_len = 1];
+    pipe.model.InsightMetricsKind metrics_kind = 2;
+    pipe.model.InsightStep insight_step = 3;


nit: I think "step" is fine. We are also using "metrics_kind" instead of "insight_metrics_kind"

nghialv · 2020-12-03T05:35:35Z

pkg/app/api/service/webservice/service.proto

+    pipe.model.InsightMetricsKind metrics_kind = 2;
+    pipe.model.InsightStep insight_step = 3;
+    int64 first_date = 4;
+    int64 last_date = 5;


These are the Unix times so, range_from, and range_to looks good.

nghialv · 2020-12-03T05:37:40Z

pkg/app/api/service/webservice/service.proto

+    pipe.model.InsightStep insight_step = 3;
+    int64 first_date = 4;
+    int64 last_date = 5;
+}


Can we reorder the fields by their stability/importance. I mean the metrics_kind must be required so it should at the top.

metrics_kind step range_from range_to application_id

nghialv · 2020-12-03T05:38:39Z

pkg/app/api/service/webservice/service.proto

 }
+
+message GetInsightDataRequest {
+    string application_id = 1 [(validate.rules).string.min_len = 1];


I think the application id is optional.
If it was specified we find the data for only the specified one.
If it was empty, we find the data for the whole project.

nghialv · 2020-12-03T05:43:41Z

@sanposhiho Thank you. I have just left some comments.

…b.com:pipe-cd/pipe into create-application-deployment-frequency-proto

sanposhiho · 2020-12-03T06:32:59Z

@nghialv
Thanks for much reviews :D

Fix:

add validation on some fields
rename some fields

nghialv · 2020-12-03T06:36:26Z

Nice. Thank you.
/approve

pipecd-bot · 2020-12-03T06:36:29Z

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.

sanposhiho requested review from cakecatz and nakabonne December 2, 2020 07:38

pipecd-bot added release-note-none area/api do-not-merge/wip size/S labels Dec 2, 2020

sanposhiho changed the title ~~add application deployment frequency proto file~~ Add a definition of function for the deployment frequency graph Dec 2, 2020

sanposhiho added 2 commits December 2, 2020 16:55

Add application deployment frequency proto file

2765192

Fix words day to date

174d9a8

sanposhiho force-pushed the create-application-deployment-frequency-proto branch from a603228 to 174d9a8 Compare December 2, 2020 07:56

cakecatz reviewed Dec 2, 2020

View reviewed changes

Add function GetApplicationDeploymentFrequency to satisfy interface

4bd4505

pipecd-bot reviewed Dec 2, 2020

View reviewed changes

Fix to pass golangci-lint and comment about temporary implementation

117bd27

pipecd-bot reviewed Dec 2, 2020

View reviewed changes

Format comment with the way of golint

fae84f5

nghialv reviewed Dec 2, 2020

View reviewed changes

nakabonne reviewed Dec 3, 2020

View reviewed changes

Fix to use blank identifier

566976b

Fix to be able to use same rpc to other insights graph

e1084a5

pipecd-bot added size/M and removed size/S labels Dec 3, 2020

sanposhiho marked this pull request as ready for review December 3, 2020 02:48

pipecd-bot removed the do-not-merge/wip label Dec 3, 2020

sanposhiho marked this pull request as draft December 3, 2020 02:48

Fix proto file path

4cf59c7

nakabonne reviewed Dec 3, 2020

View reviewed changes

sanposhiho and others added 4 commits December 3, 2020 12:29

Fix typo and rm unuse import

d501d70

Add insight.proto

b87b50b

Remove unuse message

b24c7a4

Rename to LEAD_TIME

d2c5d9d

Co-authored-by: Ryo Nakao <[email protected]>

sanposhiho marked this pull request as ready for review December 3, 2020 05:00

pipecd-bot added the lgtm label Dec 3, 2020

nghialv reviewed Dec 3, 2020

View reviewed changes

pkg/model/insight.proto Outdated

enum InsightStep {

DAILY = 0;

WEEKLY = 1;

YEARLY = 2;

Copy link

Member

nghialv Dec 3, 2020

Choose a reason for hiding this comment

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

Missing the MONTHLY

nghialv reviewed Dec 3, 2020

View reviewed changes

sanposhiho added 3 commits December 3, 2020 15:29

Add some validate and rename some fields

e6b4ba3

Fix to reponse Unimplemented

dab071c

Merge branch 'create-application-deployment-frequency-proto' of githu…

26663a5

…b.com:pipe-cd/pipe into create-application-deployment-frequency-proto

pipecd-bot removed the lgtm label Dec 3, 2020

pipecd-bot added the approved label Dec 3, 2020

pipecd-bot merged commit 162c018 into master Dec 3, 2020

pipecd-bot deleted the create-application-deployment-frequency-proto branch December 3, 2020 06:45

pipecd-bot mentioned this pull request Dec 16, 2020

Add doc for pipectl #1266

Merged

	func (a WebAPI) GetApplicationDeploymentFrequency(ctx context.Context, req webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {
	func (a WebAPI) GetApplicationDeploymentFrequency(_ context.Context, _ webservice.GetApplicationDeploymentFrequencyRequest) (*webservice.GetApplicationDeploymentFrequencyResponse, error) {

Add a definition of function for the deployment frequency graph #1179

Add a definition of function for the deployment frequency graph #1179

Uh oh!

Conversation

sanposhiho commented Dec 2, 2020

Uh oh!

nakabonne commented Dec 2, 2020

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 left a comment

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Dec 2, 2020

Uh oh!

pipecd-bot commented Dec 2, 2020

Uh oh!

nghialv Dec 2, 2020 • 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!

pipecd-bot commented Dec 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Dec 3, 2020

Uh oh!

nakabonne commented Dec 3, 2020

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!

khanhtc1202 Dec 3, 2020 • 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!

khanhtc1202 Dec 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv commented Dec 3, 2020

Uh oh!

nghialv Dec 2, 2020 •

edited

Loading

khanhtc1202 Dec 3, 2020 •

edited

Loading

khanhtc1202 Dec 3, 2020 •

edited

Loading