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

Port prometheus to opencensus wrapper #92

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

bigs
Copy link
Contributor

@bigs bigs commented Mar 20, 2019

This change also introduces a very naive metricsModules map that
collects, by name, all of the modules present in the daemon.

Sorry for the confusion here, @vyzo. @anacrolix and @lanzafame made a compelling argument to switch to OpenCensus and I decided to make the jump before it's too late. Still exposing a prometheus-friendly collection endpoint.

@bigs bigs requested a review from vyzo March 20, 2019 22:03
@ghost ghost assigned bigs Mar 20, 2019
@ghost ghost added the in progress label Mar 20, 2019
@anacrolix
Copy link
Contributor

Why not just expose all metrics, all the time? They're suitably name-spaced that choosing which ones to use can be done further down the line by tools better suited for it.

@bigs
Copy link
Contributor Author

bigs commented Mar 21, 2019

maybe i misunderstood but doesn’t registering them cause them to impact performance? idea was to allow users to selectively not register metrics. definitely down to instrument everything if we don’t mind!

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

looks fine.
Btw, what's with all the junk in go.sum? Maybe we need to go mod tidy.

@vyzo
Copy link
Collaborator

vyzo commented Mar 21, 2019

Note CI is failing again.

p2pd/metrics.go Outdated
}
view.RegisterExporter(pe)

if err = psmetrics.Register(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Per comments on the other PR, this would look like this: https://github.com/anacrolix/go-libp2p-dht-tool/blob/feat/oc-metrics/main.go#L377. What do you think?

mux := http.NewServeMux()
mux.Handle("/metrics", pe)
if err := http.ListenAndServe(*metricsAddr, mux); err != nil {
log.Printf("setting up metrics endpoint (%s): %s", *metricsAddr, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Try setting up metrics endpoint %q: %v :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k!

Cole Brown added 5 commits April 26, 2019 16:36
This change also introduces a very naive metricsModules map that
collects, by name, all of the modules present in the daemon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants