Skip to content
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.

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

Clearly Specify Router Metrics file format returned #10549

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

mburke5678
Copy link
Contributor

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 29, 2018
@openshift openshift deleted a comment from jcantrill Jun 29, 2018
@mburke5678
Copy link
Contributor Author

@knobunc Is this something your team can help with? (I pulled a QuickLab instance to test, but it won't allow users to get metrics.) Per BZ:

Describe the issue: 
1. Documentation states, if ROUTER_METRICS_TYPE is set (by default) to haproxy then 
http://admin:<Password>@localhost:1936/metrics should return Prometheus format statistics.
My testing revealed we get a text file with csv Prometheus format statistics with only /metrics exposed.

2.  Documentation states, if ROUTER_METRICS_TYPE is deleted we get HAproxy csv metrics on http://admin:<Password>@localhost:1936/metrics.

My testing revealed we get an html file with statistics on ANY url. Meaning /metrics /ANYTHING will return that html file that has an option to CSV export.

Suggestions for improvement: 
Please clearly specify both the url to get the metrics and the actual file time returned, txt or html

@mburke5678
Copy link
Contributor Author

@knobunc PTAL

@mburke5678
Copy link
Contributor Author

@pecameron Can you take a look at this? I believe you wrote the original text that a customer is seeking more information/clarification on.

@mburke5678 mburke5678 requested review from pecameron and knobunc July 6, 2018 23:40
====
----
$ oc adm router --service-account=router
----
====
metrics are automatically available in Prometheus format on the stats-port, default 1936. To suppress metrics collection,

* To get a CSV text file that contains Prometheus format statistics with *metrics* exposed:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not a CSV format. I'd say:

To get the Prometheus statistics for the exposed metrics:


* To get a CSV text file that contains Prometheus format statistics with *metrics* exposed:
+
Launch the stats window use the following URL in a browser:
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't going to work since localhost from a browser is likely the machine they are on. Really they need to tell prometheus to gather from the host:1936 with the appropriate credentials.

Copy link
Contributor Author

@mburke5678 mburke5678 Jul 9, 2018

Choose a reason for hiding this comment

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

@knobunc @pecameron
The customer is looking for:
Please clearly specify both the url to get the metrics and the actual file time returned, txt or html

If I read our docs right, the URL is the STATS_PORT address set in the router env variables and the user can curl <user>:<password>@<router_IP>:<STATS_PORT>/metrics.

  1. oc get po router-1-6tuk1 --template={{.status.hostIP}} for router IP
  2. oc describe po router-1-6tuk1 to get the admin name/password?

Copy link

@pecameron pecameron left a comment

Choose a reason for hiding this comment

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

I think the thrust of the doc should push Prometheus metrics. This plugs into the standard way metrics are collected in Openshift. These metrics include the haproxy native metrics an some controller metrics. The previous way was to use the native metrics from haproxy 1.5 and this should be for backward compatibility only.

Launch the stats window use the following URL in a browser:
+
----
http://admin:<Password>@localhost:1936/metrics

Choose a reason for hiding this comment

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

I think "localhost" should be the router's ip address or FQN. @knobunc is this correct?

Launch the stats window use the following URL in a browser:
+
----
http://admin:<Password>@localhost:1936/metrics

Choose a reason for hiding this comment

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

"/metrics" is not needed here since any path (or no path) will yield the same results. Again, localhost is the IP or FQN of the router.

@@ -1531,10 +1550,10 @@ metadata:
prometheus.openshift.io/username: admin
----
====

+

Choose a reason for hiding this comment

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

@knobunc didn't we change authorization to not pass the username and password as annotations?

Choose a reason for hiding this comment

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

@lihongan Could you review this PR?

@mburke5678
Copy link
Contributor Author

@anpingli PTAL

====
----
$ oc adm router --service-account=router --stats-port=0
----
====

To switch to the HAProxy CSV format metrics, edit the xref:../../architecture/networking/routes.adoc#env-variables[environment variables]
* To switch to the HAProxy CSV format metrics, edit the xref:../../architecture/networking/routes.adoc#env-variables[environment variables]

Choose a reason for hiding this comment

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

Above should be switch to the HTML format metrics instead of CSV format, see https://bugzilla.redhat.com/show_bug.cgi?id=1548500

@mburke5678
Copy link
Contributor Author

@lihongan PTAL

@lihongan
Copy link

LGTM, thank you @mburke5678

@openshift-ci-robot openshift-ci-robot added 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 Jul 17, 2018
@mburke5678
Copy link
Contributor Author

@openshift/team-documentation PTAL

@mburke5678
Copy link
Contributor Author

@knobunc @pecameron How far back should I roll these changes back? All the way to 3.1?

@knobunc
Copy link
Contributor

knobunc commented Jul 17, 2018

The router stats were protected by delegated auth in this PR openshift/origin#16975 (this went into 3.7)

And the new stats were added by openshift/origin#13337 (this went into 3.6)

Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

I have a couple of picks and suggestions for you to consider. :)

link:https://cbonte.github.io/haproxy-dconv/1.5/configuration.html#9[HAProxy router] in its own CSV format.
These metrics include the HAProxy native metrics and some controller metrics.

When you create a router using the following command, {product-title} makes metrics available in Prometheus format on the stats-port, default 1936.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe s/stats-port/stats port
maybe s/default/by default

value: 0.0.0.0:1936
- name: ROUTER_METRICS_TYPE
value: haproxy
oc describe pod <router_pod>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/oc/$ oc


Information needed to access the metrics is found the router service annotations:
* To suppress metrics collection,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/,/:

====
* To extract the raw statistics in Prometheus format use the following.
+
You can get the information you need to access the metrics from the router service annotations:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd make this line an annotation for line 1501 (annotations:)


====
* To extract the raw statistics in Prometheus format use the following.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/format/format,
maybe s/use the following./run the following command and modify the output:

Use the above username and password to access the metrics. The path is "/metrics".

+
The metrics port is the STATS_PORT, default 1936. You might need to configure your firewall to permit access.
Copy link
Contributor

Choose a reason for hiding this comment

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

`STATS_PORT` ?
s/default/by default


+
The metrics port is the STATS_PORT, default 1936. You might need to configure your firewall to permit access.
Use the above user name and password to access the metrics. The path is "/metrics".
Copy link
Contributor

Choose a reason for hiding this comment

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

s/above/previous (or established or <modifier_of_choice>)
s/"/metrics"/`/metrics`

value: haproxy
----
+
Launch the stats window using the following URL in a browser, where 1936 is the `STATS_PORT` value:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the organization of this section could be improved. I think this section is actually three substeps and that you have them presented in 1-3-2 order.
s/Launch the stats window using the following URL in a browser, where 1936 is the STATS_PORT value/Open the stats window at the following URL:
The note about 1936 might be better as an annotation on line 1557.

oc describe pod <router_pod>
----
+
You get a statistics file in HTML format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the statistics display or do they automatically download? I might say one of these instead:
"The statistics display."
"An HTML file that contains the statistics downloads."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knobunc Can you help with this? (I tried to set up an QuickLab instance, but I cannot access the stats page). What output does the user get when here?

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jul 17, 2018
@pecameron
Copy link

pecameron commented Jul 23, 2018 via email

@openshift-docs-preview-bot

The preview will be availble shortly at:

@mburke5678
Copy link
Contributor Author

mburke5678 commented Jul 24, 2018

@lihongan
Can you take another look? I got a question from my peer review that caused me to change things a little.

@lihongan
Copy link

lihongan commented Aug 1, 2018

@mburke5678 Looks good. Thank you.
Maybe the notification from GitHub was missed, sorry for late feedback.

@vikram-redhat
Copy link
Contributor

@mburke5678 I have added the 3.11 branch label and the next milestone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-3.6 branch/enterprise-3.7 branch/enterprise-3.9 branch/enterprise-3.10 branch/enterprise-3.11 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants