Update the docs and scaffold to make clear how works the metrics and what is required to do for third party API's. #2606
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
openshift-ci-robot added the size/M 
camilamacedo86 changed the title camilamacedo86 requested review from asmacdo, bharathi-tenneti, fabianvf, hasbro17 and jmccormick2001 camilamacedo86 added metrics kind/documentation 
| Hi @jmccormick2001, Really thank you for your input. Let me know what do you think now? |
jmccormick2001 approved these changes Feb 27, 2020
estroz previously requested changes Feb 27, 2020
doc/user-guide.md Outdated
| | ||
| #### Default Metrics exported with 3rd party resource | ||
| | ||
| By default the projects built with SDK will be implemented to export metrics. For further information check the [metrics section][metrics_doc]. Then, in the `main.go` you will see: |
There was a problem hiding this comment.
Suggested change
| By default the projects built with SDK will be implemented to export metrics. For further information check the [metrics section][metrics_doc]. Then, in the `main.go` you will see: | |
| By default, SDK operator projects are set up to [export metrics][metrics_doc] through `serveCRMetrics` in `cmd/manager/main.go`: |
doc/user-guide.md Outdated
| | ||
| ```go | ||
| func serveCRMetrics(cfg *rest.Config) error { | ||
| |
internal/scaffold/cmd.go Outdated
Comment on lines 222 to 225
| // Note that if you are adding third party API schemas, probably you will need to customize this implementation | ||
| // to not face issues such as; "Failed to list *unstructured.Unstructured:" which may occur because | ||
| // the operator has not permission to List the schemas added. See that when a schema is added | ||
| // it also may add others schemas which are required by it and then, they will not be filtered by default. |
There was a problem hiding this comment.
Discussing errors in cmd/manager/main.go in depth is unnecessary. If anything, link to the relevant user guide section.
There was a problem hiding this comment.
I tried to improve that. Let me know if it is ok for you now.
internal/scaffold/cmd.go Outdated
| func serveCRMetrics(cfg *rest.Config) error { | ||
| // Below function returns filtered operator/CustomResource specific GVKs. | ||
| // For more control override the below GVK list with your own custom logic. | ||
| // The function below returns filtered operator/CR specific GVKs. For more control, override the GVK list below |
There was a problem hiding this comment.
Suggested change
| // The function below returns filtered operator/CR specific GVKs. For more control, override the GVK list below | |
| // The function below returns a list of filtered operator-/CR-specific GVKs. | |
| // To collect metrics on fewer types, filter the GVK list below further. |
There was a problem hiding this comment.
I'd rather see this wording than the wording you're currently using. There is no "overriding" here, just further filtering.
This was referenced Feb 27, 2020
Co-Authored-By: Eric Stroczynski <estroczy@redhat.com>
camilamacedo86 dismissed estroz’s stale review eric is in PTO, however, all his suggestions were accepted and done.
| All changes and suggestions made by @estroz are applied |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Description of the change:
Motivation for the change:
Closes #2577
Closes #1858