-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
pkg/{sdk,generator} Expose Prometheus metrics port #323
pkg/{sdk,generator} Expose Prometheus metrics port #323
Conversation
55bc84f
to
404791e
Compare
CI fail because during @theishshah In |
The |
404791e
to
6b89548
Compare
My bad, it's still failing After investigation :
It should finally work 😄 |
Hey, we noticed that last night as well, PR #329 should address this issue |
6b89548
to
825da34
Compare
Thanks @theishshah |
@etiennecoutaud Sorry for the delay. I've actually been trying to test your PR for a day now but I'm running into an issue. When I do
However now the issue is that the updated vendored SDK depends on imports like $ operator-sdk build quay.io/coreos/operator-sdk-dev:app-operator
Error: failed to build: (building app-operator...
vendor/github.com/operator-framework/operator-sdk/pkg/sdk/metrics.go:8:2: cannot find package "github.com/prometheus/client_golang/prometheus/promhttp" in any of:
/Users/haseeb/work/go-space/src/github.com/example-inc/app-operator/vendor/github.com/prometheus/client_golang/prometheus/promhttp (vendor tree)
/usr/local/go/src/github.com/prometheus/client_golang/prometheus/promhttp (from $GOROOT)
/Users/haseeb/work/go-space/src/github.com/prometheus/client_golang/prometheus/promhttp (from $GOPATH)
) Since app-operator does not directly use that import you can't force the prometheus dependencies to be vendored by adding it as a constraint in app-operator/Gopkg.toml either. So I was wondering if you had managed to do a local build and test of this with the README example? |
@hasbro17 no problem for the delay. I reproduced your issue, It works and I'm able to build on my side because I have
Regarding this, why CI is green 🤔 ? |
pkg/generator/generator_test.go
Outdated
@@ -276,7 +282,7 @@ func TestGenDeploy(t *testing.T) { | |||
} | |||
|
|||
buf = &bytes.Buffer{} | |||
if err := renderFile(buf, operatorTmplName, operatorYamlTmpl, tmplData{ProjectName: appProjectName, Image: appImage}); err != nil { | |||
if err := renderFile(buf, operatorTmplName, operatorYamlTmpl, tmplData{ProjectName: appProjectName, Image: appImage, MetricsPort: k8sutil.PrometheusMetricsPort, MetricsPortName: k8sutil.PrometheusMetricsPortName, OperatorNameEnv: k8sutil.OperatorNameEnvVar}); err != nil { |
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.
Define the tmplData before this since it's too long:
td := tmplData{
ProjectName: appProjectName,
Image: appImage,
MetricsPort: k8sutil.PrometheusMetricsPort,
MetricsPortName: k8sutil.PrometheusMetricsPortName,
OperatorNameEnv: k8sutil.OperatorNameEnvVar,
}
if err := renderFile(buf, operatorTmplName, operatorYamlTmpl, td); err != nil {
t.Error(err)
}
@etiennecoutaud I've just tested this out by pulling the dependencies locally and it works. We can figure out what's wrong with the CI later. I'm going to push this change up as a separate branch on the repo and so I can test whether or not |
825da34
to
d1c5dbd
Compare
@hasbro17 okay ! |
"strconv" | ||
|
||
k8sutil "github.com/operator-framework/operator-sdk/pkg/util/k8sutil" | ||
"github.com/prometheus/client_golang/prometheus/promhttp" |
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.
As a suggestion, the prometheus OpenCensus exporter library could be used.
Here is a code snippet to illustrate the usage:
import (
"github.com/sirupsen/logrus"
"github.com/prometheus/client_golang/prometheus"
ocprometheus "go.opencensus.io/exporter/prometheus"
ocview "go.opencensus.io/stats/view"
)
metricsRegistry := prometheus.NewRegistry()
metricsRegistry.MustRegister(prometheus.NewGoCollector())
prometheusExporter, err := ocprometheus.NewExporter(ocprometheus.Options{
Namespace: "operator_sdk",
Registry: metricsRegistry,
})
if err != nil {
logrus.Fatalf("Failed to create prometheus exporter: %v", err)
return
}
ocview.RegisterExporter(prometheusExporter)
http.Handle("/"+k8sutil.PrometheusMetricsPortName, prometheusExporter)
IMHO, this project should integrate and use the OpenCensus library.
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.
@secat I'm not too familiar with the OpenCensus library so I can't comment on the need for it yet. However this PR is only meant to be the starting point for exposing the Prometheus metrics so I would prefer to keep it simple right now and iterate on more changes after this.
It'll be helpful if you can create an issue so we can discuss the use cases for the OpenCensus library in the SDK.
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.
LGTM. |
…ebase-master Merge upstream tag v1.29.0
related to #222 discuss
This PR is similar to #241
\cc @hasbro17