Skip to content

Conversation

SvenDowideit
Copy link

Or if that's too verbose, can we just add the http.Handle("/metrics/inner", promhttp.HandlerFor(reg, promhttp.HandlerOpts{})) line to help the new user?

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

The example is not really meant to handle the promhttp part, but I'm not opposed to making this anyway quite long example a bit more comprehensive. But see my comments for details...


// Because we've registered these metrics to a new non-default Registry
// we need to tell it to servce them separately
http.Handle("/metrics/inner", promhttp.HandlerFor(reg, promhttp.HandlerOpts{}))
Copy link
Member

Choose a reason for hiding this comment

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

I would not recommend to serve those metrics separately.

If you would like to see the standard process and Go metrics, it's better to register the respective collectors with the custom registry. It might actually be a good idea to demonstrate that here, turning this into a very comprehensive example.

Copy link
Author

Choose a reason for hiding this comment

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

aha, another thing i haven't worked out how to do - will do some more reading :)

Copy link
Member

Choose a reason for hiding this comment

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

The code is the following:

reg.MustRegister(
	prometheus.NewProcessCollector(prometheus.ProcessCollectorOpts{}),
	prometheus.NewGoCollector(),
)

Copy link
Member

Choose a reason for hiding this comment

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

ping Would you like to add the above and then remove the exposition of the default registry?

Copy link
Author

Choose a reason for hiding this comment

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

yup, sorry, $work is distracting :)

http.Handle("/metrics/inner", promhttp.HandlerFor(reg, promhttp.HandlerOpts{}))

// The default golang metrics
http.Handle("/metrics", prometheus.Handler())
Copy link
Member

Choose a reason for hiding this comment

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

In the spirit of the above, I would drop this line.

@SvenDowideit SvenDowideit force-pushed the add-extra-hints-to-clustermanager-example branch from 913cd43 to 218f588 Compare November 14, 2018 00:59
@beorn7 beorn7 assigned beorn7 and unassigned beorn7 Nov 15, 2018
@beorn7 beorn7 self-requested a review November 15, 2018 14:20
…therer fits together

Signed-off-by: Dowideit, Sven (O&A, St. Lucia) <Sven.Dowideit@csiro.au>
@SvenDowideit SvenDowideit force-pushed the add-extra-hints-to-clustermanager-example branch from 218f588 to 8324e65 Compare November 20, 2018 05:46
@SvenDowideit
Copy link
Author

aha, and that simplifies the code i'm using too - worth iterating - thanks for taking the time :)

@beorn7
Copy link
Member

beorn7 commented Nov 20, 2018

Thank you, too.

@beorn7 beorn7 merged commit f6443e7 into prometheus:master Nov 20, 2018
@SvenDowideit SvenDowideit deleted the add-extra-hints-to-clustermanager-example branch November 22, 2018 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants