Skip to content

doc/user: Add metrics and service-monitor docs #719

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.

Sign up for GitHub

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

Jump to bottom
Merged
lilic merged 19 commits into from
Mar 6, 2019
Merged

doc/user: Add metrics and service-monitor docs #719

lilic merged 19 commits into from
Mar 6, 2019

Conversation

@lilic
Copy link
Member

@lilic lilic commented Nov 8, 2018 •
edited
Loading

Description of the change:

Basic documentation for monitoring.

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 8, 2018
@lilic
Copy link
Member Author

lilic commented Nov 9, 2018

As agreed offline: we want to also a doc.go file to the pkg/metrics.

@lilic lilic force-pushed the lili/metrics-docs branch from 13605b9 to 1654c1f Compare January 9, 2019 10:04
@lilic lilic force-pushed the lili/metrics-docs branch 2 times, most recently from 97cbe94 to a73a77d Compare February 8, 2019 16:32
@lilic lilic changed the title WIP: doc/user: Add monitoring with prometheus documentation doc/user: Add metrics and service-monitor docs Feb 8, 2019
@openshift-ci-robot openshift-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 Feb 8, 2019
@lilic
Copy link
Member Author

lilic commented Feb 8, 2019

@shawn-hurley You mentioned you wanted me to add a doc.go file to the pkg/metrics/, tried doing that but think I would just duplicate what it says here. Ideas welcome!

@lilic lilic force-pushed the lili/metrics-docs branch from a73a77d to 28dd07c Compare February 8, 2019 16:37
@lilic
Copy link
Member Author

lilic commented Feb 8, 2019

Blocked until #1037 is merged, but ready for review.

@lilic
Copy link
Member Author

lilic commented Feb 8, 2019

Happy with all the suggestions here, docs are not my strong point. :)

joelanford
joelanford reviewed Feb 19, 2019
View reviewed changes
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.

Looks pretty good to me. I have a couple of suggestions. Also, should we link to these from the user guide?

doc/user/metrics/service_monitor.md Outdated
}
```

*Note:* Create one `ServiceMonitor` per application and per `Namespace`.
Copy link
Member

@joelanford joelanford Feb 19, 2019

Choose a reason for hiding this comment

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

Maybe expand on this a little bit.

Copy link
Member Author

@lilic lilic Feb 25, 2019

Choose a reason for hiding this comment

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

Moved this as a comment in code example so it hopefully makes more sense.

@adellape adellape mentioned this pull request Feb 22, 2019
Merged
@lilic
Copy link
Member Author

lilic commented Feb 25, 2019

Renamed the file as well to service-monitor.md We have mixed file names but managed to find a style guide that mentions file naming conventions for markdown, and according to that hyphens are the way to go -> http://www.cirosantilli.com/markdown-style-guide/#file-name

@lilic
Copy link
Member Author

lilic commented Feb 25, 2019

@joelanford Made some adjustments, can you please have another look.

Also, should we link to these from the user guide?

Yes, was thinking the same but wasn't sure where it should be placed, if you have any suggestions?

joelanford
joelanford approved these changes Feb 25, 2019
View reviewed changes
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.

LGTM other than a couple more suggestions.

@joelanford
Copy link
Member

joelanford commented Feb 25, 2019

Yes, was thinking the same but wasn't sure where it should be placed, if you have any suggestions?

@lilic I'd say under the "Advanced Topics" section would be a good spot. WDYT?

https://github.com/operator-framework/operator-sdk/blob/master/doc/user-guide.md#advanced-topics

@lilic lilic force-pushed the lili/metrics-docs branch from b4d83dc to ff0c644 Compare February 25, 2019 15:30
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2019
@lilic lilic force-pushed the lili/metrics-docs branch from ff0c644 to a117683 Compare February 25, 2019 15:37
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2019
AlexNPavel
AlexNPavel approved these changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@AlexNPavel AlexNPavel left a comment

Choose a reason for hiding this comment

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

A couple minor nits, but overall LGTM

hasbro17
hasbro17 reviewed Mar 4, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 4, 2019
View reviewed changes
AlexNPavel and others added 5 commits March 4, 2019 11:04
@AlexNPavel @lilic
Update doc/user/metrics/service-monitor.md
ff00784 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@AlexNPavel @lilic
Update doc/user/metrics/service-monitor.md
be804d1 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@AlexNPavel @lilic
Update doc/user/metrics/service-monitor.md
f4546a5 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@hasbro17 @lilic
Update doc/user-guide.md
cc206ec 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@lilic
Change link name
f0c3529
@lilic
Copy link
Member Author

lilic commented Mar 4, 2019

Done, PTALA @hasbro17

hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
hasbro17
hasbro17 reviewed Mar 5, 2019
View reviewed changes
@hasbro17
Copy link
Contributor

hasbro17 commented Mar 5, 2019

Perhaps this is outside the scope of this PR and we can do a follow up but I don't see any documentation on what the actual metrics being exposed are.
Should we list out what exactly are the controller related metrics being recorded by the controller-runtime? And perhaps give an example of querying the endpoint to see these metrics in an example?
https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/internal/controller/metrics
https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/internal/controller/metrics/metrics.go#L50-L58

  • Number of reconciliations
  • Number of reconciliation errors per controller
  • Reconciliation duration
  • Resource metrics like CPU, memory usage, file descriptor usage
  • Go runtime metrics like number of Go routines, GC duration

@lilic lilic mentioned this pull request Mar 5, 2019
Closed
hasbro17 and others added 7 commits March 5, 2019 09:49
@hasbro17 @lilic
Update doc/user/metrics/service-monitor.md
4e24d78 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@hasbro17 @lilic
Update doc/user/metrics/README.md
46bfff2 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@hasbro17 @lilic
Update doc/user/metrics/service-monitor.md
280a2e1 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@hasbro17 @lilic
Update doc/user/metrics/service-monitor.md
be7d2c3 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@hasbro17 @lilic
Update doc/user/metrics/README.md
6f2a9fd 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@hasbro17 @lilic
Update doc/user/metrics/service-monitor.md
5abe606 
Co-Authored-By: LiliC <cosiclili@gmail.com>
@lilic
Address comments
20d07b7
@lilic
Copy link
Member Author

lilic commented Mar 5, 2019

@hasbro17 Done, addressed the comments. I opened issue for the above suggestion to tackle in a follow up PR, when I am back from vacation. PTALA, thanks!

hasbro17
hasbro17 approved these changes Mar 5, 2019
View reviewed changes
Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM

@lilic lilic merged commit 3a83a0c into operator-framework:master Mar 6, 2019
@lilic lilic deleted the lili/metrics-docs branch March 6, 2019 07:08
@bergerhoffer bergerhoffer mentioned this pull request Apr 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joelanford joelanford joelanford approved these changes

@estroz estroz Awaiting requested review from estroz

@shawn-hurley shawn-hurley Awaiting requested review from shawn-hurley

@theishshah theishshah Awaiting requested review from theishshah

+2 more reviewers

@AlexNPavel AlexNPavel AlexNPavel approved these changes

@hasbro17 hasbro17 hasbro17 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lilic @joelanford @hasbro17 @AlexNPavel @openshift-ci-robot