-
Notifications
You must be signed in to change notification settings - Fork 324
Expose piped stat metrics at ops metrics endpoint #2202
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
Changes from 8 commits
78d42dd
21e0fa2
b967765
1847b7e
b61b053
d448919
6cad0a6
2f1a89d
028e358
ac20e18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") | ||
|
|
||
| go_library( | ||
| name = "go_default_library", | ||
| srcs = ["builder.go"], | ||
| importpath = "github.com/pipe-cd/pipe/pkg/app/ops/pipedstatsbuilder", | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "//pkg/cache:go_default_library", | ||
| "@org_uber_go_zap//:go_default_library", | ||
| ], | ||
| ) | ||
|
|
||
| go_test( | ||
| name = "go_default_test", | ||
| size = "small", | ||
| srcs = ["builder_test.go"], | ||
| data = glob(["testdata/**"]), | ||
| embed = [":go_default_library"], | ||
| deps = [ | ||
| "//pkg/cache:go_default_library", | ||
| "@com_github_stretchr_testify//assert:go_default_library", | ||
| "@org_uber_go_zap//:go_default_library", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // 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. | ||
|
|
||
| package pipedstatsbuilder | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "errors" | ||
| "io" | ||
|
|
||
| "go.uber.org/zap" | ||
|
|
||
| "github.com/pipe-cd/pipe/pkg/cache" | ||
| ) | ||
|
|
||
| type PipedStatsBuilder struct { | ||
| backend cache.Cache | ||
| logger *zap.Logger | ||
| } | ||
|
|
||
| func NewPipedStatsBuilder(c cache.Cache, logger *zap.Logger) *PipedStatsBuilder { | ||
| return &PipedStatsBuilder{ | ||
| backend: c, | ||
| logger: logger.Named("piped-metrics-builder"), | ||
| } | ||
| } | ||
|
|
||
| func (b *PipedStatsBuilder) Build() (io.Reader, error) { | ||
| res, err := b.backend.GetAll() | ||
| if err != nil { | ||
| b.logger.Error("failed to fetch piped stats from cache", zap.Error(err)) | ||
| return nil, err | ||
| } | ||
| data := make([][]byte, 0, len(res)) | ||
| for _, v := range res { | ||
| value, okValue := v.([]byte) | ||
| if !okValue { | ||
| err = errors.New("error value not a bulk of string value") | ||
| b.logger.Error("failed to marshal piped stat data", zap.Error(err)) | ||
| return nil, err | ||
| } | ||
| data = append(data, value) | ||
| } | ||
| return bytes.NewReader(bytes.Join(data, []byte("\n"))), nil | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's alright to be later, adding a test for Build() makes it more clear.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, just added a simple test 6cad0a6 👀 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // 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. | ||
|
|
||
| package pipedstatsbuilder | ||
|
|
||
| import ( | ||
| "io" | ||
| "io/ioutil" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "go.uber.org/zap" | ||
|
|
||
| "github.com/pipe-cd/pipe/pkg/cache" | ||
| ) | ||
|
|
||
| type mockBuilderBackend struct { | ||
| cache.Cache | ||
| srcs []string | ||
| } | ||
|
|
||
| func newMockBuilderBackend() *mockBuilderBackend { | ||
| return &mockBuilderBackend{ | ||
| srcs: []string{ | ||
| "./testdata/piped_stat_1", | ||
| "./testdata/piped_stat_2", | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func (m *mockBuilderBackend) GetAll() (map[string]interface{}, error) { | ||
| out := make(map[string]interface{}, len(m.srcs)) | ||
| for _, file := range m.srcs { | ||
| data, err := ioutil.ReadFile(file) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ioutil package is entirely deprecated. Let's use os.ReadFile
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets no ioutil, lol |
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| out[file] = data | ||
| } | ||
| return out, nil | ||
| } | ||
|
|
||
| func TestBuildPipedStat(t *testing.T) { | ||
| builder := NewPipedStatsBuilder(newMockBuilderBackend(), zap.NewNop()) | ||
| rc, err := builder.Build() | ||
| assert.NoError(t, err) | ||
| assert.NotNil(t, rc) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. require package would be better because it stops test execution when it fails.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, do we have convention of which check should be wrapped with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed on ac20e18
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case of nil and error, it's more likely to panic if proceeds. So we basically use |
||
| buf := new(strings.Builder) | ||
| _, err = io.Copy(buf, rc) | ||
| assert.NoError(t, err) | ||
| data, _ := ioutil.ReadFile("./testdata/expected") | ||
| assert.Equal(t, string(data), buf.String()) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops' deployment need this too.
https://github.com/pipe-cd/pipe/blob/master/manifests/pipecd/templates/deployment.yaml#L96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 👍
Address by b967765 🙏