Improve insight collector #1340
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
The following files are not gofmt-ed. By commenting pkg/app/ops/insightcollector/collector_test.go--- pkg/app/ops/insightcollector/collector_test.go.orig
+++ pkg/app/ops/insightcollector/collector_test.go
@@ -20,10 +20,12 @@
"testing"
"time"
- "github.com/pipe-cd/pipe/pkg/filestore/filestoretest"
"go.uber.org/zap"
+ "github.com/pipe-cd/pipe/pkg/filestore/filestoretest"
+
"github.com/golang/mock/gomock"
+
"github.com/pipe-cd/pipe/pkg/datastore"
"github.com/pipe-cd/pipe/pkg/datastore/datastoretest"
|
pipecd-bot
reviewed
Dec 27, 2020
Collaborator
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
| i.logger.Error("failed to update application chunks", zap.Error(err)) | ||
| } | ||
| var returnErr error | ||
| for appId, ds := range appMap { |
Collaborator
There was a problem hiding this comment.
range var appId should be appID
| } | ||
| return nil | ||
| } | ||
| for proId, ds := range projectMap { |
Collaborator
There was a problem hiding this comment.
range var proId should be proID
| i.logger.Error("failed to update application chunks", zap.Error(err)) | ||
| } | ||
| var returnErr error | ||
| for appId, ds := range appMap { |
Collaborator
There was a problem hiding this comment.
range var appId should be appID
| maxCreatedAt = apps[len(apps)-1].CreatedAt | ||
| } | ||
| return nil | ||
| for proId, ds := range projectMap { |
Collaborator
There was a problem hiding this comment.
range var proId should be proID
| return err | ||
| } | ||
|
|
||
| dc, err := i.getDeploymentsWithCreatedAt(ctx, m.DeploymentCreatedAtMilestone, targetDate.Unix()) |
Collaborator
There was a problem hiding this comment.
ineffectual assignment to err
| continue | ||
| } | ||
| for _, k := range aggregateKinds { | ||
| dc, err := i.getDeploymentsWithCompletedAt(ctx, m.DeploymentCompletedAtMilestone, targetDate.Unix()) |
Collaborator
There was a problem hiding this comment.
ineffectual assignment to err
Collaborator
|
Code coverage for golang is
|
Collaborator
|
Code coverage for golang is
|
Collaborator
|
Code coverage for golang is
|
Collaborator
|
Code coverage for golang is
|
nghialv
reviewed
Jan 4, 2021
cmd/pipecd/ops.go
Outdated
| t.Logger.Info("application insight collector successfully finished", zap.Duration("duration", time.Since(start))) | ||
| retryAggregateWithCompletedAt := true | ||
| retryAggregateWithCreatedAt := true | ||
| for i := 0; i < cfg.InsightCollector.RetryTime; i++ { |
Member
There was a problem hiding this comment.
We are having a backoff package for this retrying purpose. So could you apply it to this?
https://github.com/pipe-cd/pipe/tree/master/pkg/backoff
https://github.com/pipe-cd/pipe/blob/master/pkg/backoff/backoff.go#L34
https://github.com/pipe-cd/pipe/blob/master/pkg/backoff/constant.go#L26
| // updated all project's insights completely | ||
| break | ||
| func (i *InsightCollector) AggregateWithCreatedAt(ctx context.Context) error { | ||
| currentTime := time.Now() |
| if len(projects) == 0 { | ||
| // updated all project's insights completely | ||
| break | ||
| func (i *InsightCollector) AggregateWithCreatedAt(ctx context.Context) error { |
Member
There was a problem hiding this comment.
nit: processNewlyCreatedDeployments
| if len(apps) == 0 { | ||
| // updated all application's insights completely | ||
| break | ||
| func (i *InsightCollector) AggregateWithCompletedAt(ctx context.Context) error { |
Member
There was a problem hiding this comment.
nit: processNewlyCompletedDeployments
| appMap, projectMap := i.groupingDeployments(dc) | ||
|
|
||
| var returnErr error | ||
| for appID, ds := range appMap { |
| if err != nil { | ||
| return err | ||
| } | ||
| appMap, projectMap := i.groupingDeployments(dc) |
Member
There was a problem hiding this comment.
nit: apps, projects := i.groupDeployments(dc)
| } | ||
| appMap, projectMap := i.groupingDeployments(dc) | ||
|
|
||
| var returnErr error |
| if returnErr == nil { | ||
| m.DeploymentCreatedAtMilestone = targetDate.Unix() | ||
| } | ||
| if err := i.insightstore.PutMilestone(ctx, m); err != nil { |
Member
There was a problem hiding this comment.
Do we need to update the milestone when returnErr was not nil?
Member
|
@sanposhiho Hi, thanks for your great work! |
Collaborator
|
Code coverage for golang is
|
Member
Author
|
@nghialv |
Member
|
Yep, looks neat 👍 |
nghialv
reviewed
Jan 5, 2021
| Schedule string `json:"schedule"` | ||
| Schedule string `json:"schedule"` | ||
| RetryTime int `json:"retryTime"` | ||
| RetryIntervalHour int `json:"retryIntervalHour"` |
Member
There was a problem hiding this comment.
Please set the default value for these fields. Currently, they are zero it means no jobs will be executed.
nghialv
reviewed
Jan 5, 2021
cmd/pipecd/ops.go
Outdated
| var failed bool | ||
| start := time.Now() | ||
| if err = collector.ProcessNewlyCompletedDeployments(ctx); err != nil { | ||
| t.Logger.Error("failed to process the insight collector with completedAt", zap.Error(err)) |
Member
There was a problem hiding this comment.
nit: failed to process the newly completed deployments while accumulating insight data
cmd/pipecd/ops.go
Outdated
| t.Logger.Error("failed to process the insight collector with completedAt", zap.Error(err)) | ||
| failed = true | ||
| } else { | ||
| t.Logger.Info("processing the insight collector with completedAt successfully finished", zap.Duration("duration", time.Since(start))) |
Member
There was a problem hiding this comment.
nit: successfully processed the newly completed deployments while accumulating insight data
cmd/pipecd/ops.go
Outdated
|
|
||
| start = time.Now() | ||
| if err = collector.ProcessNewlyCreatedDeployments(ctx); err != nil { | ||
| t.Logger.Error("failed to process the insight collector with createdAt", zap.Error(err)) |
Member
There was a problem hiding this comment.
nit: failed to process the newly created deployments while accumulating insight data
cmd/pipecd/ops.go
Outdated
| t.Logger.Error("failed to process the insight collector with createdAt", zap.Error(err)) | ||
| failed = true | ||
| } else { | ||
| t.Logger.Info("processing the insight collector with createdAt successfully finished", zap.Duration("duration", time.Since(start))) |
Member
There was a problem hiding this comment.
nit: successfully processed the newly created deployments while accumulating insight data
cmd/pipecd/ops.go
Outdated
| for retry.WaitNext(ctx) { | ||
| var failed bool | ||
| start := time.Now() | ||
| if err = collector.ProcessNewlyCompletedDeployments(ctx); err != nil { |
Member
There was a problem hiding this comment.
Can you skip calling this function when it was completed successfully while ProcessNewlyCreatedDeployments was failed? I know that the insight data will not be affected but we can save some unnecessary DB calls by skip calling this one.
|
|
||
| // getInsightDataForDeployFrequency accumulate insight data in target range for deploy frequency. | ||
| func (i *InsightCollector) getInsightDataForDeployFrequency( | ||
| func (i *InsightCollector) getDeploymentsWithCreatedAt( |
Member
There was a problem hiding this comment.
nit: findDeploymentsCreatedInRange
| i.logger.Error("failed to get deployments", zap.Error(err)) | ||
| return &insight.ChangeFailureRate{}, time.Time{}, fmt.Errorf("failed to get deployments") | ||
| // groupingDeployments groups deoloyments by applicationID and projectID | ||
| func (i *InsightCollector) groupingDeployments(deployments []*model.Deployment) (map[string][]*model.Deployment, map[string][]*model.Deployment) { |
| if err != nil { | ||
| i.logger.Error("failed to get deployments", zap.Error(err)) | ||
| return &insight.ChangeFailureRate{}, time.Time{}, fmt.Errorf("failed to get deployments") | ||
| // groupingDeployments groups deoloyments by applicationID and projectID |
| return &insight.ChangeFailureRate{}, time.Time{}, fmt.Errorf("failed to get deployments") | ||
| // groupingDeployments groups deoloyments by applicationID and projectID | ||
| func (i *InsightCollector) groupingDeployments(deployments []*model.Deployment) (map[string][]*model.Deployment, map[string][]*model.Deployment) { | ||
| appmap := map[string][]*model.Deployment{} |
nghialv
reviewed
Jan 5, 2021
| ) | ||
|
|
||
| // extractDeployFrequency extracts deploy frequency from deployments with specified range | ||
| func (i *InsightCollector) extractDeployFrequency(deployments []*model.Deployment, from, to int64, targetTimestamp int64) (*insight.DeployFrequency, []*model.Deployment) { |
Member
There was a problem hiding this comment.
This can be an independent function, doesn't need to belong to InsightCollector.
nghialv
reviewed
Jan 5, 2021
| } | ||
|
|
||
| // extractDeployFrequency extracts change failure rate from deployments with specified range | ||
| func (i *InsightCollector) extractChangeFailureRate(deployments []*model.Deployment, from, to int64, targetTimestamp int64) (*insight.ChangeFailureRate, []*model.Deployment) { |
Member
There was a problem hiding this comment.
This can be an independent function, doesn't need to belong to InsightCollector.
Member
|
@sanposhiho Thanks for your updates. Could you take a look at my recent reviews? |
Collaborator
|
Code coverage for golang is
|
Member
|
@sanposhiho Many thanks for your efforts. |
Collaborator
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Change the insight collector logic to reduce communication with datastore.
Which issue(s) this PR fixes:
Fixes #1314
Does this PR introduce a user-facing change?: