operator-sdk icon indicating copy to clipboard operation
operator-sdk copied to clipboard

[WIP] Add auto generated documentation for metrics

Open avlitman opened this issue 1 year ago • 7 comments

Create auto-generated documentation for the test operator metrics.

This change will follow the best practice: OpenShift Operators Best Practices: Monitoring

Jira-ticket: https://issues.redhat.com/browse/CNV-17014

avlitman avatar Aug 22 '22 13:08 avlitman

/hold

avlitman avatar Aug 22 '22 13:08 avlitman

Hi @machadovilaca, and @camilamacedo86 I can't add you as reviewers for some reason so I added you here.

avlitman avatar Aug 22 '22 13:08 avlitman

@machadovilaca Hi, I will appreciate your review on the comments/documentation added. Note that I also added the type to metrics.md. And fixed typos, from renconcile to reconcile.

avlitman avatar Aug 29 '22 11:08 avlitman

@sradco

avlitman avatar Sep 04 '22 09:09 avlitman

/hold

avlitman avatar Sep 18 '22 11:09 avlitman

@avlitman, @machadovilaca Do we want to have these changes scaffold with make generate using memcached_with_webhooks.go?

assafad avatar Sep 19 '22 11:09 assafad

/hold

avlitman avatar Sep 20 '22 08:09 avlitman

@assafad We will definitely use make generate later to create these changes, but I think it's easier to first agree on the design and implementation, and later add the generators.

machadovilaca avatar Sep 29 '22 15:09 machadovilaca

@fabianvf and @varshaprasad96 are you able to review the proposed changes by @avlitman in this PR, so we can start working on generating the code?

machadovilaca avatar Oct 18 '22 17:10 machadovilaca

@avlitman @machadovilaca This seems to be a very good addition. Including metrics doc will add value to consumers of operator authors. But I have a few questions here: Looks like these changes have been manually done to testdata/ which is a sample operator to follow the openshift best practices. This seems to make the memcached operator here to be a sample project which has metrics enabled. However, for this to be usable by all SDK consumers, it would be helpful if we could add additional scaffolding to the plugins as you have mentioned. To do so, we have 2 options:

  • (1) Modify an existing plugin (probably go-v4-alpha) to include these changes in scaffolded templates. I'm not specifying go/v3 for now since its stable.
  • (2) Create a new plugin (https://book.kubebuilder.io/plugins/plugins.html), similar to https://book.kubebuilder.io/plugins/grafana-v1-alpha.html, that adds additional changes to an existing scaffolded project to include monitoring related documentation and helper methods. Both of these have their own pros and cons.
  1. Modify existing plugin: Q1. What happens to operators who would not like to deal with anything related to metrics? These additional targets in Makefile will be of no additional value and eventually are unused pieces of code. Q2. This best practice is a norm by "Openshift operators", how do we provide flexibility for authors to customize it.

  2. Additional plugin:

  • This will help in keeping the scaffolding related to monitoring isolated. This way only the operator authors which intend to use Prometheus metrics can use this. Q1. Creating a new plugin just to add documentation does seem to be an overdo in terms of even maintaining the plugin code. Is there a possibility wherein we could abstract methods to register metrics (etc.), and scaffold additional code for user to easily onboard and start collecting metrics?

These are the few questions that comes to my mind while thinking about scaffolding code. Please add your thoughts for discussion. cc: @operator-framework/team-sdk

varshaprasad96 avatar Oct 18 '22 17:10 varshaprasad96

@avlitman @machadovilaca This seems to be a very good addition. Including metrics doc will add value to consumers of operator authors. But I have a few questions here: Looks like these changes have been manually done to testdata/ which is a sample operator to follow the openshift best practices. This seems to make the memcached operator here to be a sample project which has metrics enabled. However, for this to be usable by all SDK consumers, it would be helpful if we could add additional scaffolding to the plugins as you have mentioned. To do so, we have 2 options:

  • (1) Modify an existing plugin (probably go-v4-alpha) to include these changes in scaffolded templates. I'm not specifying go/v3 for now since its stable.
  • (2) Create a new plugin (https://book.kubebuilder.io/plugins/plugins.html), similar to https://book.kubebuilder.io/plugins/grafana-v1-alpha.html, that adds additional changes to an existing scaffolded project to include monitoring related documentation and helper methods. Both of these have their own pros and cons.
  1. Modify existing plugin: Q1. What happens to operators who would not like to deal with anything related to metrics? These additional targets in Makefile will be of no additional value and eventually are unused pieces of code. Q2. This best practice is a norm by "Openshift operators", how do we provide flexibility for authors to customize it.
  2. Additional plugin:
  • This will help in keeping the scaffolding related to monitoring isolated. This way only the operator authors which intend to use Prometheus metrics can use this. Q1. Creating a new plugin just to add documentation does seem to be an overdo in terms of even maintaining the plugin code. Is there a possibility wherein we could abstract methods to register metrics (etc.), and scaffold additional code for user to easily onboard and start collecting metrics?

These are the few questions that comes to my mind while thinking about scaffolding code. Please add your thoughts for discussion. cc: @operator-framework/team-sdk

@varshaprasad96 thank you for the detailed review, I discussed it with @sradco and as for now we think that option 2, to create a new plugin is the best approach, we also agree on Q1, to abstract methods to register metrics, and scaffold additional code for user to easily onboard and start collecting metrics.

On Grafana Plugin docs, there are references to metrics:

Do you think there could be duplication of information (with this pr) that could be confusing?

Another thing we thought could be useful going forward, is adding a metric to Telemetry that checks the use of the new plugin.

@varshaprasad96 In order to discuss the issue further, I would like to set up a meeting. Is that okay with you? Your help is greatly appreciated!

avlitman avatar Oct 19 '22 13:10 avlitman

@avlitman having a separate plugin is also a good option. But we would have to think about a few more points related to implementation too. For example:

  1. Where would the plugin be located. If this is generic for all KB projects, then it would make more sense to have it there. But at the same time, with please 2 plugins coming in, we can have them in an external repository.
  2. What would the scaffolding look like? Can existing pre-scaffolded projects adopt this? (i.e) does any pre-scaffolded file needs to be modified?
  3. Since this is monitoring related and we also have a grafana plugin which is alpha, can we make changes to the same?

This feature is really helpful and I'd really be happy to discuss the next steps for moving forward. Would you like to join the Operator SDK community meeting, which happens every week on Monday (11AM - 12noon PST) - link: https://zoom.us/j/8415370125. It would be helpful to get thoughts from the community on the next steps.

varshaprasad96 avatar Oct 21 '22 02:10 varshaprasad96

@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.

openshift-merge-robot avatar Nov 12 '22 22:11 openshift-merge-robot

The changes proposed here will be handled differently by 3 separate pr's. The metrics structure pr is: https://github.com/operator-framework/operator-sdk/pull/6150 The docs generation pr is: https://github.com/operator-framework/operator-sdk/pull/6114 The new metrics pr is still in progress.

avlitman avatar Nov 13 '22 19:11 avlitman