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 5 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
9 changes: 9 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,12 @@ func (a *WebAPI) ListAPIKeys(ctx context.Context, req *webservice.ListAPIKeysReq
Keys: apiKeys,
}, nil
}

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

@pipecd-bot pipecd-bot Dec 2, 2020

Choose a reason for hiding this comment

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

ctx is unused in GetApplicationDeploymentFrequency

Copy link
Collaborator

@pipecd-bot pipecd-bot Dec 2, 2020

Choose a reason for hiding this comment

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

req is unused in GetApplicationDeploymentFrequency

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.

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) {

// to pass golangci-lint(unused) check
fmt.Print(ctx)
fmt.Print(req)

return nil, nil
}
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, "")
}

13 changes: 13 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 @@ -85,6 +85,9 @@ service WebService {
rpc GenerateAPIKey(GenerateAPIKeyRequest) returns (GenerateAPIKeyResponse) {}
rpc DisableAPIKey(DisableAPIKeyRequest) returns (DisableAPIKeyResponse) {}
rpc ListAPIKeys(ListAPIKeysRequest) returns (ListAPIKeysResponse) {}

// Insights
rpc GetApplicationDeploymentFrequency(GetApplicationDeploymentFrequencyRequest) returns (GetApplicationDeploymentFrequencyResponse) {}
}

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

message GetApplicationDeploymentFrequencyRequest {
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.

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

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.

Oh, I forgot removing 👍

string application_id = 1 [(validate.rules).string.min_len = 1];
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.

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.

repeated int64 dates = 2;
Copy link
Contributor

@cakecatz cakecatz Dec 2, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sanposhiho sanposhiho Dec 2, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member

@nakabonne nakabonne Dec 2, 2020

Choose a reason for hiding this comment

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

(not including holidays in the graph, etc.)

Agree with you 👍

}

message GetApplicationDeploymentFrequencyResponse {
// key: date, value: deployment_count_of_the_day
map<int64, int64> deployment_counts = 1;
}
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.