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

pkg/kube-metrics: Add initial operator specific metrics #1277

Merged
merged 27 commits into from
Jun 13, 2019

Conversation

lilic
Copy link
Member

@lilic lilic commented Apr 2, 2019

Operator specific metrics are generated based on the api/kind given
deployed in the given namespace(s).

See https://github.com/operator-framework/operator-sdk/blob/master/doc/proposals/metering-operator-metrics.md for the full background on this work.

Things to be done:

  • kube-state-metrics has not been released yet, so currently pinning this to master, so we should wait until that is released so we can pin. But there should not be any major changes from master until the next release so this is ready for review and feedback.
  • Currently we detect the namespace in which the operator is deployed in and use that by default along with the namespaces passed by the user. In the future we might want to also detect in which namespace all the CRs are deployed in, if the operator is cluster scoped. Not sure if we want this done in the current PR? Since everything will be done in the kube-metrics package and will not be user facing I think it can be done at a later point.
  • Expose the metrics port in the Service file. This includes breaking changes and would prefer to do it in another PR as this one large enough to review.
  • Update proposal to done once this is merged.

Closes #616

@lilic lilic changed the title Wpkg/kube-metrics: Add initial operator specific metrics WIP: pkg/kube-metrics: Add initial operator specific metrics Apr 2, 2019
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 2, 2019
@lilic
Copy link
Member Author

lilic commented Apr 3, 2019

This is ready for review, the WIP can be removed when the new kube-state-metrics release happens. :)

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Looks good overall, but a few nits and style suggestions need to be addressed.

internal/pkg/scaffold/metrics.go Outdated Show resolved Hide resolved
internal/pkg/scaffold/metrics.go Outdated Show resolved Hide resolved
pkg/kube-metrics/collector.go Outdated Show resolved Hide resolved
pkg/kube-metrics/collector.go Show resolved Hide resolved
pkg/kube-metrics/server.go Show resolved Hide resolved
pkg/kube-metrics/uclient.go Outdated Show resolved Hide resolved
pkg/kube-metrics/uclient.go Outdated Show resolved Hide resolved
@lilic lilic force-pushed the lili/op-specific-metrics branch 3 times, most recently from 67cdc23 to 6d2bc78 Compare April 4, 2019 09:34
@lilic
Copy link
Member Author

lilic commented Apr 4, 2019

Done @estroz cam you PTAL, thanks!

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Since setting up metrics goes hand-in-hand with adding APIs, can we structure the scaffolding and registration of the metrics the same way that we structure the scaffolding and scheme building of the APIs?

I think that would mean:

  1. When operator-sdk new is called:
    • Create pkg/metrics/metrics.go, similar to pkg/apis/apis.go
    • In cmd/manager/main.go, use what's been setup via the exposed vars/functions in pkg/metrics/metrics.go in the call to ServeOperatorSpecificMetrics(). Since the API-specific stuff would be handled elsewhere, I think ServeOperatorSpecificMetrics could be made generic and included in the SDK.
  2. When operator-sdk add api is called:

I think this approach would solve the issues I mentioned in the review, and it would align better with how we scaffold and register other project-specific resources.

For the namespace question, can we pass a slice of namespaces into ServeOperatorSpecificMetrics (to be forward-compatible with upcoming multi-namespace support), and for now use what we get from k8sutil.GetWatchNamespace()?

Lastly (and this may have been discussed before because it sounds familiar, so forgive me), is it possible/does it make sense to hook these metrics into the controller-runtime metrics server so we don't have to run two servers on two ports?

internal/pkg/scaffold/metrics.go Outdated Show resolved Hide resolved
internal/pkg/scaffold/cmd_test.go Outdated Show resolved Hide resolved
internal/pkg/scaffold/metrics_test.go Outdated Show resolved Hide resolved
@lilic
Copy link
Member Author

lilic commented Apr 8, 2019

Lastly (and this may have been discussed before because it sounds familiar, so forgive me), is it possible/does it make sense to hook these metrics into the controller-runtime metrics server so we don't have to run two servers on two ports?

No, because these metrics don't use the prometheus client_golang library we can't do that right now.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2019
@lilic lilic force-pushed the lili/op-specific-metrics branch 3 times, most recently from da02b28 to fda1196 Compare April 24, 2019 13:33
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2019
@lilic lilic force-pushed the lili/op-specific-metrics branch from 77ee3d2 to 4046c9c Compare May 2, 2019 14:43
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2019
@lilic lilic force-pushed the lili/op-specific-metrics branch 4 times, most recently from f703099 to bdb4c90 Compare May 6, 2019 10:48
@lilic lilic changed the title WIP: pkg/kube-metrics: Add initial operator specific metrics pkg/kube-metrics: Add initial operator specific metrics May 6, 2019
@lilic lilic merged commit dcc2b19 into operator-framework:master Jun 13, 2019
@lilic lilic deleted the lili/op-specific-metrics branch June 13, 2019 08:01
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Aug 26, 2019
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Aug 26, 2019
joelanford added a commit that referenced this pull request Aug 27, 2019
* version,cmd/operator-sdk/version: add go version, os, and arch to version subcommand

* CHANGELOG.md: added lines for #1857 and #1863, removed dup for #1277
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator specific metrics as part of the operator-metering story
6 participants