Skip to content
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.

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

Introducing pipedstat model #2261

Merged
merged 5 commits into from
Jul 19, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions BUILD.bazel
Expand Up @@ -62,6 +62,7 @@ genrule(
# gazelle:exclude pkg/model/logblock.pb.validate.go
# gazelle:exclude pkg/model/notificationevent.pb.validate.go
# gazelle:exclude pkg/model/piped.pb.validate.go
# gazelle:exclude pkg/model/piped_stat.pb.validate.go
# gazelle:exclude pkg/model/piped_stats.pb.validate.go
# gazelle:exclude pkg/model/planpreview.pb.validate.go
# gazelle:exclude pkg/model/project.pb.validate.go
Expand Down
14 changes: 13 additions & 1 deletion pkg/app/api/grpcapi/piped_api.go
Expand Up @@ -16,6 +16,7 @@ package grpcapi

import (
"context"
"encoding/json"
"errors"
"time"

Expand Down Expand Up @@ -98,7 +99,18 @@ func (a *PipedAPI) ReportStat(ctx context.Context, req *pipedservice.ReportStatR
if err != nil {
return nil, err
}
if err := a.pipedStatCache.Put(pipedID, req.PipedStats); err != nil {

now := time.Now().Unix()
val, err := json.Marshal(model.PipedStat{Timestamp: now, Stats: req.PipedStats})
if err != nil {
a.logger.Error("failed to store the reported piped stat",
zap.String("piped-id", pipedID),
zap.Error(err),
)
return nil, status.Error(codes.Internal, "failed to encode the reported piped stat")
}

if err := a.pipedStatCache.Put(pipedID, val); err != nil {
a.logger.Error("failed to store the reported piped stat",
zap.String("piped-id", pipedID),
zap.Error(err),
Expand Down
2 changes: 2 additions & 0 deletions pkg/datastore/pipedstatsstore.go
Expand Up @@ -27,6 +27,7 @@ var pipedStatsFactory = func() interface{} {
return &model.PipedStats{}
}

// Deprecated: PipedStats model is deprecated, along with its store interface.
khanhtc1202 marked this conversation as resolved.
Show resolved Hide resolved
type PipedStatsStore interface {
AddPipedStats(ctx context.Context, ps *model.PipedStats) error
ListPipedStatss(ctx context.Context, opts ListOptions) ([]model.PipedStats, error)
Expand All @@ -37,6 +38,7 @@ type pipedStatsStore struct {
nowFunc func() time.Time
}

// Deprecated: PipedStats model is deprecated, along with its store interface.
khanhtc1202 marked this conversation as resolved.
Show resolved Hide resolved
func NewPipedStatsStore(ds DataStore) PipedStatsStore {
return &pipedStatsStore{
backend: backend{
Expand Down
1 change: 1 addition & 0 deletions pkg/model/BUILD.bazel
Expand Up @@ -16,6 +16,7 @@ proto_library(
"logblock.proto",
"notificationevent.proto",
"piped.proto",
"piped_stat.proto",
"piped_stats.proto",
"planpreview.proto",
"project.proto",
Expand Down
25 changes: 25 additions & 0 deletions pkg/model/piped_stat.proto
@@ -0,0 +1,25 @@
// Copyright 2021 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 PipedStat {
Copy link
Member

Choose a reason for hiding this comment

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

How about adding PipedID into this message?
I know we are having that ID from the map key but ID inside its data could be helpful as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure 👍 Btw I'm thinking about the current model name, just want to make it PipedMetrics. In that case, PipedMetrics.PipedID and PipedMetrics.Stat looks better than PipedStat.PipedID and PipedStat.Stat as currently. Just wonder is that name too generics or something 🤔 How do you think about that?

Copy link
Member

Choose a reason for hiding this comment

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

I think PipedMetrics is good too. But currently, our RPC name is Report...Stats, right?
So maybe we should keep that model name as PipedStat and change its Stat field to Metrics.
Or we can reuse the old PipedStats model, deprecate unused fields and add new fields like PipedID, Metrics.

Copy link
Member Author

Choose a reason for hiding this comment

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

So maybe we should keep that model name as PipedStat and change its Stat field to Metrics.

Get your point, lets me adopt this 👍 ( Personally don't think the name should be XXXStats since this message covers only a single value of piped committed stat at a time )

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed by ae07feb 🙏

int64 timestamp = 1 [(validate.rules).int64.gt = 0];
bytes stats = 2;
}
1 change: 1 addition & 0 deletions pkg/model/piped_stats.proto
Expand Up @@ -39,6 +39,7 @@ message PrometheusMetrics {
}

message PipedStats {
option deprecated = true;
string version = 1 [(validate.rules).string.min_len = 1];
int64 timestamp = 2 [(validate.rules).int64.gt = 0];
repeated PrometheusMetrics prometheus_metrics = 3;
Expand Down