Skip to content

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

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

Merged
pipecd-bot merged 15 commits into from
Dec 3, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2765192
Add application deployment frequency proto file
sanposhiho Dec 2, 2020
174d9a8
Fix words day to date
sanposhiho Dec 2, 2020
4bd4505
Add function GetApplicationDeploymentFrequency to satisfy interface
sanposhiho Dec 2, 2020
117bd27
Fix to pass golangci-lint and comment about temporary implementation
sanposhiho Dec 2, 2020
fae84f5
Format comment with the way of golint
sanposhiho Dec 2, 2020
566976b
Fix to use blank identifier
sanposhiho Dec 3, 2020
e1084a5
Fix to be able to use same rpc to other insights graph
sanposhiho Dec 3, 2020
4cf59c7
Fix proto file path
sanposhiho Dec 3, 2020
d501d70
Fix typo and rm unuse import
sanposhiho Dec 3, 2020
b87b50b
Add insight.proto
sanposhiho Dec 3, 2020
b24c7a4
Remove unuse message
sanposhiho Dec 3, 2020
d2c5d9d
Rename to LEAD_TIME
sanposhiho Dec 3, 2020
e6b4ba3
Add some validate and rename some fields
sanposhiho Dec 3, 2020
dab071c
Fix to reponse Unimplemented
sanposhiho Dec 3, 2020
26663a5
Merge branch 'create-application-deployment-frequency-proto' of githu…
sanposhiho Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/app/api/grpcapi/web_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1309,3 +1309,8 @@ func (a *WebAPI) ListAPIKeys(ctx context.Context, req *webservice.ListAPIKeysReq
Keys: apiKeys,
}, nil
}

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

@nghialv nghialv Dec 3, 2020

Choose a reason for hiding this comment

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

nit:

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

17 changes: 17 additions & 0 deletions pkg/app/api/service/webservice/service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ option go_package = "github.com/pipe-cd/pipe/pkg/app/api/service/webservice";

import "validate/validate.proto";
import "pkg/model/common.proto";
import "pkg/model/insight.proto";
import "pkg/model/application.proto";
import "pkg/model/application_live_state.proto";
import "pkg/model/command.proto";
Expand Down Expand Up @@ -85,6 +86,9 @@ service WebService {
rpc GenerateAPIKey(GenerateAPIKeyRequest) returns (GenerateAPIKeyResponse) {}
rpc DisableAPIKey(DisableAPIKeyRequest) returns (DisableAPIKeyResponse) {}
rpc ListAPIKeys(ListAPIKeysRequest) returns (ListAPIKeysResponse) {}

// Insights
rpc GetInsightData(GetInsightDataRequest) returns (GetInsightDataResponse) {}
}

message AddEnvironmentResponse {
Expand Down Expand Up @@ -401,3 +405,16 @@ message ListAPIKeysRequest {
message ListAPIKeysResponse {
repeated model.APIKey keys = 1;
}

message GetInsightDataRequest {
pipe.model.InsightMetricsKind metrics_kind = 1 [(validate.rules).enum.defined_only = true];
pipe.model.InsightStep step = 2 [(validate.rules).enum.defined_only = true];
int64 range_from = 3 [(validate.rules).int64.gt = 0];
int64 range_to = 4 [(validate.rules).int64.gt = 0];
string application_id = 5;
}
Copy link
Member

@nghialv nghialv Dec 3, 2020

Choose a reason for hiding this comment

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

Please add the validation for those fields.

Copy link
Member

@nghialv nghialv Dec 3, 2020

Choose a reason for hiding this comment

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

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


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

@nghialv nghialv Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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;
}

Copy link
Member

@nakabonne nakabonne Dec 3, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@sanposhiho sanposhiho Dec 3, 2020

Choose a reason for hiding this comment

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

Good :)
fixed like above.

1 change: 1 addition & 0 deletions pkg/model/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ proto_library(
"environment.proto",
"event.proto",
"logblock.proto",
"insight.proto",
"piped.proto",
"piped_stats.proto",
"project.proto",
Expand Down
39 changes: 39 additions & 0 deletions pkg/model/insight.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2020 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package pipe.model;
option go_package = "github.com/pipe-cd/pipe/pkg/model";

import "validate/validate.proto";

message InsightDataPoint {
int64 timestamp = 1 [(validate.rules).int64.gt = 0];
float value = 2 [(validate.rules).float.gt = 0];
}

enum InsightMetricsKind {
DEPLOYMENT_FREQUENCY = 0;
CHANGE_FAILURE_RATE = 1;
MTTR = 2;
LEAD_TIME = 3;
}

enum InsightStep {
DAILY = 0;
WEEKLY = 1;
MONTHLY = 2;
YEARLY = 3;
}