✨📖 [plugins]: Create monitoring plugin #3106

avlitman · 2022-11-30T17:57:56Z

We would like to add a new monitoring plugin that will help operators developer with setting up Prometheus based monitoring for their operator, will provide them with best practices and tooling to shorten their time to get up to speed with monitoring requirements and help with standardizing the way monitoring is implemented in operators.

what issue it fixes:
Fixes Add Monitoring Plugin #3078

Signed-off-by: Aviv Litman [email protected]

linux-foundation-easycla · 2022-11-30T17:57:59Z

❌ - login: @avlitman / name: Aviv Litman . The commit (28fe31e) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

k8s-ci-robot · 2022-11-30T17:58:05Z

Hi @avlitman. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-11-30T17:58:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: avlitman
Once this PR has been reviewed and has the lgtm label, please assign jmrodri for approval by writing /assign @jmrodri in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

avlitman · 2022-11-30T18:09:32Z

Hi @machadovilaca @sradco @varshaprasad96 @everettraven @assafad.

Notes:
@varshaprasad96 @everettraven Please focus on the plugin structure and not the content yet in your review.

@machadovilaca:
I will appreciate if your review will also refer to the comments.
And in /monitoring/tools/metricsdocs.go
I need help with extracting the repo name substring from {{ .Repo }} since I want to use it in the consts part of the file,
I was not able to find the right way to use filepath.Base( {{ .Repo }} ).
last thing, I added summary and Histogram in util.go and we need to verify that the syntax is correct, and will work with real metrics.

camilamacedo86

We will need to discuss this one. So, just putting in hold for we are able to properly get the review and discuss the initiative.

camilamacedo86 · 2022-12-01T13:26:31Z

test/testdata/generate.sh

@@ -132,16 +135,17 @@ scaffold_test_project project-v2 --project-version=2
 # [Currently, default CLI plugin] - Project version 3 (default) uses plugin go/v3 (default).
 scaffold_test_project project-v3
 scaffold_test_project project-v3-multigroup
-scaffold_test_project project-v3-declarative-v1 --plugins="go/v3,declarative,grafana/v1-alpha"
+scaffold_test_project project-v3-declarative-v1 --plugins="go/v3,declarative,grafana/v1-alpha,monitoring/v1-alpha"


Could we you please do not add this plugin here? we should have in the declarative one juts the declarative.
Therefore, we need to remove the grafana/v1-alpha from master branch.

@camilamacedo86
We need to test both the init and edit commands with the new monitoring plugin.
at first I used scaffold_test_project project-v4-monitoring --plugins="go/v4-alpha,monitoring.kubebuilder.io/v1-alpha" to test the init commend.

but it created a confusing naming under testdata folder
project-v*-monitoring
project-v*-with-monitoring

Later I followed @varshaprasad96 suggestion:
"
I would suggest to test both the init and edit commands. We should have scaffold_test_project project-v4-with-monitoring and the other scaffolding which calls this command chained with other plugins (whichever seems fit), probably `scaffold_test_project project-v4-declarative-v1 --plugins="go/v4-alpha,declarative,grafana/v1-alpha,monitoring/v1-alpha". This will make sure that a user with pre-scaffolded project is able to add the monitoring plugin with the edit command, as well as someone who is scaffolding their project for the first time can chain this plugin with existing ones during init.
"

@varshaprasad96 and @everettraven suggested this to test all the plugins together, in oreder to see there are no conflicts.

So thinking about this again (I now think I understand the original problem better than I did before), I understand @camilamacedo86's reasoning for wanting them separate (definitely better for testing/maintenance IMO).

I don't feel particularly strongly one way or another but if I had to choose I would probably vote for separate project scaffolds. I understand that it could be confusing names under the testdata folder but in the end I think we need to do what is better for testing and maintenance. I think maybe naming the projects project-v*-init-monitoring and project-v*-edit-monitoring might make it a bit more clear which project was created with which subcommand.

camilamacedo86 · 2022-12-01T13:27:19Z

test/testdata/generate.sh

 scaffold_test_project project-v3-config --component-config
 scaffold_test_project project-v3-with-deploy-image
 scaffold_test_project project-v3-with-metrics
+scaffold_test_project project-v3-with-monitoring


Could you please add this new plugin to the metrics sample with seems the category releated and allow us to check only the diff scaffolds that will be done by it?

@camilamacedo86 This is the plan, and it will be done in later PR.

camilamacedo86 · 2022-12-01T13:30:52Z

testdata/project-v4-declarative-v1/monitoring/tools/metricsdocs.go

+// constant parts of the file
+const (
+	title      = "# sigs.k8s.io/kubebuilder/testdata/project-v4-declarative-v1 Operator Metrics\n"
+	background = "This document aims to help users that are not familiar with metrics exposed by the sigs.k8s.io/kubebuilder/testdata/project-v4-declarative-v1 operator.\n" +


we cannot scaffold it by default.
The final result of the plugin should not point out users for the master samples.

Also, same of : https://github.com/kubernetes-sigs/kubebuilder/pull/3106/files#r1037114035

It does not fit to an end-project scaffold. It should be a lib, tool and etc.

The plan is to give an example scaffold for adding new metrics, the files can be commented also.
but it's not by default, the user will need to init/edit with monitoring plugin in order to get these files.

camilamacedo86 · 2022-12-01T13:31:37Z

testdata/project-v4-declarative-v1/monitoring/tools/metricsdocs.go

+	footer = footerHeading + footerContent
+)
+
+func main() {


Is really required to implement a util to do that?
If that is a common good practice so have not tools and etc that does it already?

@camilamacedo86 Yes, This is a very useful addition for creating an automated metrics documentation.
It we be automatically updated after every new metric is added.
We plan to also add to it the recording rules, since they are used the same as metrics

I think what @camilamacedo86 is trying to point out here is that if every operator project that wants to create/use metrics needs to use this tooling, it might be better to instead have a library/binary with this functionality that can import/download/build and be used instead of scaffolding it in every operator project.

I agree with @camilamacedo86 here that we should probably consider finding a way to make this code reusable by many operator projects without actually scaffolding this code in every operator project.

Also, @avlitman @sradco,

If is a good practice to generate the docs so do not exist projects that are responsible for that already? Is really required to implement and build a binary for it ?

camilamacedo86 · 2022-12-01T13:34:15Z

testdata/project-v4-declarative-v1/monitoring/metrics/util/util.go

@@ -0,0 +1,145 @@
+package util


Should not this kind of implementation fit better in a lib such as controller-runtime or operator-lib or a new one since prometheus-lib?

If all projects would need to use this func so that does not seems reasonable to be scaffolded in a plugin for the final projects. That really seems to be more appropriated in a lib (ihmo)

@camilamacedo86 Not sure I understand right your comment, but this implementation (util) is only for adding Prometheus metrics to the operator.

I think what @camilamacedo86 is trying to point out here is that if every operator that wants to use metrics will have to use these utility functions, it may be better to make them a Go library that can be imported rather than scaffolded in every project.

I guess this ends up posing the question - Will every operator that wants to use metrics need to implement these utility functions?

camilamacedo86

(ihmo) what is proposed by this plugin do the scaffold fit better in libs/tools and not to the final "Operator" project itself. The project itself could take benefit and use the lib to generate the metrics and/or the binary to build the docs. Therefore, explain the good practices and how to use the libs / tools that could fit in docs.

However, if you would like to have a plugin to scaffold that and does not go along within the above thought then I think that is good case for external plugins. @varshaprasad96 @everettraven @Kavinjsir @erikgb wdyt? What are your thoughts about this one?

avlitman · 2022-12-01T13:45:45Z

We will need to discuss this one. So, just putting in hold for we are able to properly get the review and discuss the initiative.

Hi @camilamacedo86 , thank you so much for reviewing this PR!
We did discuses this issue and got approved from the community see #3078

testdata/project-v3-with-monitoring/monitoring/metrics/example_metrics.go

everettraven

First round. Overall everything looks good, but I share a few of the concerns @camilamacedo86 has regarding scaffolding code in every operator project that may be better suited to being a library.

everettraven · 2022-12-01T15:04:55Z

docs/book/src/plugins/monitoring-v1-alpha.md

+
+- `monitoring/`
+- `main.go`
+- `MakeFile`


Nit:

Suggested change

- `MakeFile`

- `Makefile`

everettraven · 2022-12-01T15:10:35Z

...tional/monitoring/v1alpha/scaffolds/internal/templates/monitoring/metrics/example_metrics.go

+
+var OperatorMetrics = []util.Metric{
+// When adding new metrics, Please follow the naming conventions best practices
+// TODO: Add the link to the Observability Best Practices - Metrics Naming


I would recommend moving this TODO comment out of this template. Having it in the template makes it seem like it is something the user of the monitoring plugin will have to do, but should instead be something that is included in the template (once it is available).

everettraven · 2022-12-01T15:13:27Z

...ins/optional/monitoring/v1alpha/scaffolds/internal/templates/monitoring/metrics/util/util.go

+	}
+}
+
+// TODO: add here a comment about the functions to explain what they do


This TODO comment seems more related to the maintainers of the monitoring plugin and not the user. I would recommend moving it out of the scaffold template.

everettraven · 2022-12-01T15:22:35Z

pkg/plugins/optional/monitoring/v1alpha/scaffolds/makefile.go

+	_out/metricsdocs > docs/monitoring/metrics.md
+
+.PHONY: build-metricsdocs
+build-metricsdocs:
+	go build -ldflags="${LDFLAGS}" -o _out/metricsdocs ./monitoring/tools/metricsdocs.go
+`


Since this is specifically a tool for development purposes (i.e not running in production as part of the operator) I think it might be a bit overkill to actually build a binary for generating the metrics docs.

I would personally prefer something like:

Suggested change

_out/metricsdocs > docs/monitoring/metrics.md

.PHONY: build-metricsdocs

build-metricsdocs:

go build -ldflags="${LDFLAGS}" -o _out/metricsdocs ./monitoring/tools/metricsdocs.go

`

go run ./monitoring/tools/metricsdocs.go > docs/monitoring/metrics.md

`

Thoughts?

I think that would fit well as a third-party project with the lib.
Therefore it shows too much Prometheus specific why not add those in the Prometheus project (https://github.com/prometheus/) and allow anyone that is using kubebuilder or not to take advantage of it?

everettraven · 2022-12-01T15:31:15Z

test/testdata/generate.sh

@@ -132,16 +135,17 @@ scaffold_test_project project-v2 --project-version=2
 # [Currently, default CLI plugin] - Project version 3 (default) uses plugin go/v3 (default).
 scaffold_test_project project-v3
 scaffold_test_project project-v3-multigroup
-scaffold_test_project project-v3-declarative-v1 --plugins="go/v3,declarative,grafana/v1-alpha"
+scaffold_test_project project-v3-declarative-v1 --plugins="go/v3,declarative,grafana/v1-alpha,monitoring/v1-alpha"


So thinking about this again (I now think I understand the original problem better than I did before), I understand @camilamacedo86's reasoning for wanting them separate (definitely better for testing/maintenance IMO).

I don't feel particularly strongly one way or another but if I had to choose I would probably vote for separate project scaffolds. I understand that it could be confusing names under the testdata folder but in the end I think we need to do what is better for testing and maintenance. I think maybe naming the projects project-v*-init-monitoring and project-v*-edit-monitoring might make it a bit more clear which project was created with which subcommand.

everettraven · 2022-12-01T15:36:43Z

testdata/project-v4-declarative-v1/monitoring/metrics/util/util.go

@@ -0,0 +1,145 @@
+package util


I think what @camilamacedo86 is trying to point out here is that if every operator that wants to use metrics will have to use these utility functions, it may be better to make them a Go library that can be imported rather than scaffolded in every project.

I guess this ends up posing the question - Will every operator that wants to use metrics need to implement these utility functions?

everettraven · 2022-12-01T15:41:19Z

testdata/project-v4-declarative-v1/monitoring/tools/metricsdocs.go

+	footer = footerHeading + footerContent
+)
+
+func main() {


I think what @camilamacedo86 is trying to point out here is that if every operator project that wants to create/use metrics needs to use this tooling, it might be better to instead have a library/binary with this functionality that can import/download/build and be used instead of scaffolding it in every operator project.

I agree with @camilamacedo86 here that we should probably consider finding a way to make this code reusable by many operator projects without actually scaffolding this code in every operator project.

GitHub-issue: kubernetes-sigs#3078 Signed-off-by: Aviv Litman <[email protected]> Uncomment monitoring go files Signed-off-by: João Vilaça <[email protected]> Generate working monitoring scaffolding Signed-off-by: João Vilaça <[email protected]> Allow kubebuilder init with monitoring bundle Signed-off-by: João Vilaça <[email protected]> Add metrics register to main file Signed-off-by: João Vilaça <[email protected]> Update testdata Signed-off-by: João Vilaça <[email protected]> Improve comments Signed-off-by: Aviv Litman <[email protected]>

Kavinjsir · 2022-12-01T19:00:31Z

(ihmo) what is proposed by this plugin do the scaffold fit better in libs/tools and not to the final "Operator" project itself. The project itself could take benefit and use the lib to generate the metrics and/or the binary to build the docs. Therefore, explain the good practices and how to use the libs / tools that could fit in docs.

@avlitman I have a similar idea to what @camilamacedo86 brought:
It feels more flexible to have this feature as a library, in that case, any operator project can imported. (so that they don't necessarily to be an origin scaffold layout from kubebuilder).

varshaprasad96 · 2022-12-01T20:36:04Z

@camilamacedo86 @Kavinjsir I have added the questions based on our discussions and some of my thoughts here: #3078 (comment).

k8s-ci-robot · 2022-12-09T10:15:27Z

@avlitman: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

avlitman · 2022-12-19T12:54:18Z

Hi @camilamacedo86 @everettraven @varshaprasad96 @Kavinjsir @sradco
First I want to thank you for taking the time to help and review this issue.

Since we still discussing the implementation method, and the capacity to maintain this by our team.
I would suggested closing this PR, and open a new one in case we decide to go foreword with a new method.

camilamacedo86 · 2023-01-24T10:37:03Z

Closing this one as not accepted, for further information see: #3078.
Thank you for your contribution and understanding.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2022

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 30, 2022

k8s-ci-robot requested review from jmrodri and pwittrock November 30, 2022 17:58

avlitman force-pushed the create-monitoring-plugin branch 3 times, most recently from a36bb60 to dba9c2b Compare December 1, 2022 12:15

avlitman changed the title ~~[WIP] ✨📖 [plugins]: Create monitoring plugin~~ ✨📖 [plugins]: Create monitoring plugin Dec 1, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2022

avlitman force-pushed the create-monitoring-plugin branch from dba9c2b to e5df7f7 Compare December 1, 2022 13:05

camilamacedo86 requested changes Dec 1, 2022

View reviewed changes

camilamacedo86 reviewed Dec 1, 2022

View reviewed changes

sradco reviewed Dec 1, 2022

View reviewed changes

testdata/project-v3-with-monitoring/monitoring/metrics/example_metrics.go Outdated Show resolved Hide resolved

avlitman force-pushed the create-monitoring-plugin branch 2 times, most recently from cff5220 to 85cdb6e Compare December 1, 2022 15:32

everettraven suggested changes Dec 1, 2022

View reviewed changes

avlitman force-pushed the create-monitoring-plugin branch from 85cdb6e to eb95c9b Compare December 1, 2022 15:53

avlitman force-pushed the create-monitoring-plugin branch from eb95c9b to 28fe31e Compare December 1, 2022 17:14

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2022

camilamacedo86 mentioned this pull request Jan 23, 2023

Add Monitoring Plugin #3078

Closed

camilamacedo86 closed this Jan 24, 2023

✨📖 [plugins]: Create monitoring plugin #3106

✨📖 [plugins]: Create monitoring plugin #3106

Conversation

avlitman commented Nov 30, 2022

linux-foundation-easycla bot commented Nov 30, 2022 • edited Loading

k8s-ci-robot commented Nov 30, 2022

k8s-ci-robot commented Nov 30, 2022

avlitman commented Nov 30, 2022 • edited Loading

camilamacedo86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avlitman Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avlitman Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avlitman Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment • edited Loading

Choose a reason for hiding this comment

avlitman commented Dec 1, 2022 • edited Loading

everettraven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kavinjsir commented Dec 1, 2022

varshaprasad96 commented Dec 1, 2022

k8s-ci-robot commented Dec 9, 2022

avlitman commented Dec 19, 2022 • edited Loading

camilamacedo86 commented Jan 24, 2023

linux-foundation-easycla bot commented Nov 30, 2022 •

edited

Loading

avlitman commented Nov 30, 2022 •

edited

Loading

avlitman Dec 1, 2022 •

edited

Loading

avlitman Dec 1, 2022 •

edited

Loading

avlitman Dec 1, 2022 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

avlitman commented Dec 1, 2022 •

edited

Loading

avlitman commented Dec 19, 2022 •

edited

Loading