-
Notifications
You must be signed in to change notification settings - Fork 323
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 3 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 |
|---|---|---|
|
|
@@ -29,11 +29,14 @@ import ( | |
| "github.com/pipe-cd/pipe/pkg/app/ops/insightcollector" | ||
| "github.com/pipe-cd/pipe/pkg/app/ops/mysqlensurer" | ||
| "github.com/pipe-cd/pipe/pkg/app/ops/orphancommandcleaner" | ||
| "github.com/pipe-cd/pipe/pkg/app/ops/pipedstatsbuilder" | ||
| "github.com/pipe-cd/pipe/pkg/cache/rediscache" | ||
| "github.com/pipe-cd/pipe/pkg/cli" | ||
| "github.com/pipe-cd/pipe/pkg/config" | ||
| "github.com/pipe-cd/pipe/pkg/datastore" | ||
| "github.com/pipe-cd/pipe/pkg/insight/insightstore" | ||
| "github.com/pipe-cd/pipe/pkg/model" | ||
| "github.com/pipe-cd/pipe/pkg/redis" | ||
| "github.com/pipe-cd/pipe/pkg/version" | ||
| ) | ||
|
|
||
|
|
@@ -44,13 +47,15 @@ type ops struct { | |
| enableInsightCollector bool | ||
| configFile string | ||
| gcloudPath string | ||
| cacheAddress string | ||
| } | ||
|
|
||
| func NewOpsCommand() *cobra.Command { | ||
| s := &ops{ | ||
| httpPort: 9082, | ||
| adminPort: 9085, | ||
| gracePeriod: 15 * time.Second, | ||
| httpPort: 9082, | ||
| adminPort: 9085, | ||
| cacheAddress: "cache:6379", | ||
| gracePeriod: 15 * time.Second, | ||
| } | ||
| cmd := &cobra.Command{ | ||
| Use: "ops", | ||
|
|
@@ -62,6 +67,7 @@ func NewOpsCommand() *cobra.Command { | |
| cmd.Flags().DurationVar(&s.gracePeriod, "grace-period", s.gracePeriod, "How long to wait for graceful shutdown.") | ||
| cmd.Flags().StringVar(&s.configFile, "config-file", s.configFile, "The path to the configuration file.") | ||
| cmd.Flags().StringVar(&s.gcloudPath, "gcloud-path", s.gcloudPath, "The path to the gcloud command executable.") | ||
| cmd.Flags().StringVar(&s.cacheAddress, "cache-address", s.cacheAddress, "The address to cache service.") | ||
| return cmd | ||
| } | ||
|
|
||
|
|
@@ -144,6 +150,10 @@ func (s *ops) run(ctx context.Context, t cli.Telemetry) error { | |
| }) | ||
| } | ||
|
|
||
| rd := redis.NewRedis(s.cacheAddress, "") | ||
| statCache := rediscache.NewTTLHashCache(rd, cfg.Cache.TTLDuration(), defaultPipedStatHashKey) | ||
| psb := pipedstatsbuilder.NewPipedStatsBuilder(statCache, t.Logger) | ||
|
|
||
| // Start running admin server. | ||
| { | ||
| var ( | ||
|
|
@@ -157,7 +167,7 @@ func (s *ops) run(ctx context.Context, t cli.Telemetry) error { | |
| admin.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { | ||
| w.Write([]byte("ok")) | ||
| }) | ||
| admin.Handle("/metrics", t.PrometheusMetricsHandler()) | ||
| admin.Handle("/metrics", t.CustomedMetricsHandlerFor(psb)) | ||
|
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. with this customed metrics handler, we ignore all other metrics exposed to the current ops metrics registry ( which currently is zero ). Will carefully handle those ops's metrics later 🙏
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. nit: I feel the name should be -> |
||
|
|
||
| group.Go(func() error { | ||
| return admin.Run(ctx) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| load("@io_bazel_rules_go//go:def.bzl", "go_library") | ||
|
|
||
| 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", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| // 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" | ||
| "io/ioutil" | ||
|
|
||
| "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.ReadCloser, 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 ioutil.NopCloser(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. iouti.NopClose is deprecated. Instead using io.NopCloser is preferable.
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. Well, why does it need to be an io.ReadCloser? Seems like io.Reader is enough.
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. 👀 Just want to carefully close the []byte stream on the hander side. Or is it unnecessary? 🤔
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. First, NopCloser.Close does nothing. And with the current implementation of
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. If you're planning to directly read from any kinds of stream like Redis in the future, then you will have to add the close operation.
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. I see 👍 Since we will keep the GetAll interface which returns data ( not the reader or else to direct stream data ), it's safe to remove this unnecessary closer as you mentioned 🙏 Lets me address it here 🙆♀️
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 d448919 🙏 |
||
| } | ||
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 🙏