Skip to content

Add automated documentation for metrics#6114

Merged
everettraven merged 1 commit into
operator-framework:masterfrom
avlitman:add-metrics-doc
Jan 23, 2023
Merged

Add automated documentation for metrics#6114
everettraven merged 1 commit into
operator-framework:masterfrom
avlitman:add-metrics-doc

Conversation

@avlitman

@avlitman avlitman commented Oct 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description of the change:
Add automated documentation for metrics.
The document path is docs/monitoring/metrics.md,
and the document content is created with monitoring/metricsdocs tool.

Motivation for the change:
See up-to-date documentation of all the metrics in one place (name, description, and type).

Note
Maybe we should consider adding the doc under: operator-framework/operator-sdk/tree/master/website/content/en/docs

jira-Ticket: https://issues.redhat.com/browse/CNV-21334
Signed-off-by: Aviv Litman alitman@redhat.com

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022
@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 26, 2022
sradco
sradco reviewed Oct 27, 2022
View reviewed changes
Comment thread testdata/go/v3/memcached-operator/docs/monitoring/metrics.md Outdated
sradco
sradco reviewed Oct 27, 2022
View reviewed changes
Comment thread testdata/go/v3/memcached-operator/docs/monitoring/metrics.md Outdated
@avlitman avlitman force-pushed the add-metrics-doc branch 2 times, most recently from 8cf940b to f21bd98 Compare October 27, 2022 11:55
@avlitman avlitman changed the title [WIP]: RFE - Add automated documentation for metrics RFE - Add automated documentation for metrics Oct 27, 2022
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2022
@avlitman avlitman requested review from sradco and removed request for everettraven and varshaprasad96 October 30, 2022 09:48
@avlitman

avlitman commented Oct 30, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @varshaprasad96 @machadovilaca,
We decided to separate the creation of the documentation file for metrics from the effort to produce a general plug-in for Observability, In order to add metrics documentation in the next version.

@sradco

sradco commented Oct 30, 2022

Copy link
Copy Markdown
Contributor

/assign @varshaprasad96

sradco
sradco reviewed Oct 30, 2022
View reviewed changes
Comment thread testdata/go/v3/memcached-operator/monitoring/metricsdocs/metricsdocs.go
machadovilaca
machadovilaca reviewed Oct 31, 2022
View reviewed changes
Comment thread testdata/go/v3/memcached-operator/monitoring/metricsdocs/metricsdocs.go
@avlitman avlitman force-pushed the add-metrics-doc branch 4 times, most recently from c8b88af to 6b956d7 Compare October 31, 2022 14:21
varshaprasad96
varshaprasad96 reviewed Nov 1, 2022
View reviewed changes

@varshaprasad96 varshaprasad96 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@avlitman Since the testdata can be regenerated, the existing changes would be overwritten. I think we may still need a plugin to make changes on scaffolding especially the Makefile. Reason being, the testdata/ projects are primarily for users to refer how a sample scaffolding looks like after using the operator-sdk commands. If we make custom changes in Makefile (on top of existing scaffolding, which does not come from a plugin or any commands) then for a new user it maybe confusing to infer the sample project.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2022
@avlitman avlitman mentioned this pull request Nov 13, 2022
Closed
@varshaprasad96

varshaprasad96 commented Dec 8, 2022

Copy link
Copy Markdown
Member

/ok-to-test

sradco
sradco reviewed Jan 1, 2023
View reviewed changes
Comment thread testdata/go/v4-alpha/monitoring/memcached-operator/monitoring/metricsdocs/metricsdocs.go Outdated
sradco
sradco reviewed Jan 1, 2023
View reviewed changes
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go Outdated
sradco
sradco reviewed Jan 1, 2023
View reviewed changes
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go Outdated
sradco
sradco reviewed Jan 1, 2023
View reviewed changes
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go Outdated
sradco
sradco reviewed Jan 1, 2023
View reviewed changes
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go Outdated
sradco
sradco reviewed Jan 1, 2023
View reviewed changes
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go Outdated
@varshaprasad96 varshaprasad96 mentioned this pull request Jan 4, 2023
Closed
varshaprasad96
varshaprasad96 reviewed Jan 17, 2023
View reviewed changes
Comment thread testdata/go/v3/monitoring/memcached-operator/Makefile
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go
Comment thread hack/generate/samples/internal/go/memcached-with-webhooks/memcached_with_webhooks.go Outdated
Comment thread testdata/go/v3/monitoring/memcached-operator/monitoring/metricsdocs/metricsdocs.go
Comment thread testdata/go/v3/monitoring/memcached-operator/docs/monitoring/metrics.md
@machadovilaca

machadovilaca commented Jan 18, 2023

Copy link
Copy Markdown

@varshaprasad96 Updated the name to memcached_with_customization

@avlitman

avlitman commented Jan 19, 2023

Copy link
Copy Markdown
Contributor Author

LGTM.

@sradco

sradco commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

LGTM

varshaprasad96
varshaprasad96 reviewed Jan 20, 2023
View reviewed changes

@varshaprasad96 varshaprasad96 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR looks good overall, except for two things:

  1. (nit): The log lines and comments which have been modified to with customizations. I would prefer them to be more specific, like "webhook and metrics" where apt, or just "with webhooks".
  2. The question of moving the common generator code to a library. I'm still curious on whether it would be useful to scaffold those out for every project. I agree that the generator should live in the operator project, but its helpers can still be moved to a common place. We can create an issue and fix that in a follow up.

Rest of it is good to go from my end. Thanks for working on this PR!
/lgtm
@everettraven @jberkhahn could you all take a look at this too before we go ahead and merge this.

Comment thread hack/generate/samples/internal/go/memcached-with-customization/memcached_with_customization.go Outdated
// Note that it should NOT be called in the e2e tests.
func GenerateSample(binaryPath, samplesPath string) {
log.Infof("starting to generate Go memcached sample with webhooks")
log.Infof("starting to generate Go memcached sample with customizations")

@varshaprasad96 varshaprasad96 Jan 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Can we improve logs to mention its "with webhooks and metrics documentation" where apt or "with webhooks" otherwise.

@avlitman avlitman Jan 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@varshaprasad96 The condition you mean using generateWithMonitoring var?
As for now I changed all to "with webhooks and metrics documentation"

Comment thread hack/generate/samples/internal/go/memcached-with-customization/memcached_with_customization.go Outdated
Comment thread testdata/go/v3/monitoring/memcached-operator/docs/monitoring/metrics.md
Comment thread testdata/go/v3/monitoring/memcached-operator/monitoring/metricsdocs/metricsdocs.go
varshaprasad96
varshaprasad96 approved these changes Jan 20, 2023
View reviewed changes

@varshaprasad96 varshaprasad96 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good to go after changing the log lines and comments from my end!

everettraven
everettraven approved these changes Jan 20, 2023
View reviewed changes

@everettraven everettraven left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall I think this looks good. I have a couple comments but they are non-blocking and could be done in a follow-up.

Comment thread testdata/go/v3/monitoring/memcached-operator/docs/monitoring/metrics.md
Comment thread testdata/go/v3/monitoring/memcached-operator/monitoring/metricsdocs/metricsdocs.go
Comment thread testdata/go/v3/monitoring/memcached-operator/monitoring/metricsdocs/metricsdocs.go Outdated
@avlitman

avlitman commented Jan 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The PR looks good overall, except for two things:

  1. (nit): The log lines and comments which have been modified to with customizations. I would prefer them to be more specific, like "webhook and metrics" where apt, or just "with webhooks".

As for now I changed all to "with webhooks and metrics documentation". regarding the condition I need more guidance. In the meanwhile added this to the follow up issue.

  1. The question of moving the common generator code to a library. I'm still curious on whether it would be useful to scaffold those out for every project. I agree that the generator should live in the operator project, but its helpers can still be moved to a common place. We can create an issue and fix that in a follow up.

I will make sure to add this in a follow up, as well as the README link addition, and the template in metricsdocs @everettraven suggested.

Rest of it is good to go from my end. Thanks for working on this PR! /lgtm @everettraven @jberkhahn could you all take a look at this too before we go ahead and merge this.

@avlitman avlitman mentioned this pull request Jan 23, 2023
Closed
@avlitman

avlitman commented Jan 23, 2023

Copy link
Copy Markdown
Contributor Author

Hi @sradco @varshaprasad96 @everettraven @jberkhahn.
I created an issue for all the follow up implementations needed:#6256.

@jberkhahn can you review and (if approved) merge this pr?

Thanks all!

@sradco

sradco commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci

openshift-ci Bot commented Jan 23, 2023

Copy link
Copy Markdown

@sradco: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

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.

@everettraven

everettraven commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

/hold cancel

/lgtm

@everettraven

everettraven commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

@avlitman Once all the tests pass I will merge. Thanks!

@everettraven

everettraven commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@avlitman
RFE - Add automated documentation for metrics
19188cd 
Jira-Ticket: https://issues.redhat.com/browse/CNV-21334
Signed-off-by: Aviv Litman <alitman@redhat.com>

apply the automated documentation for metrics

Signed-off-by: Aviv Litman <alitman@redhat.com>

Fix according to Joao's review

Signed-off-by: Aviv Litman <alitman@redhat.com>

Update testdata

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Update generator code

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Rename to memcached-with-customization and remove docs bin

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Fix comments

Signed-off-by: Aviv Litman <alitman@redhat.com>
@everettraven

everettraven commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

/lgtm

@avlitman avlitman mentioned this pull request Feb 26, 2023
Closed
@avlitman avlitman mentioned this pull request Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@everettraven everettraven everettraven approved these changes

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

+2 more reviewers

@sradco sradco sradco left review comments

@machadovilaca machadovilaca machadovilaca left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@everettraven everettraven

@varshaprasad96 varshaprasad96

Labels

lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@avlitman @sradco @varshaprasad96 @jberkhahn @machadovilaca @everettraven @openshift-merge-robot