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

Handlers: Add metrics #2407

Merged

Conversation

lukedirtwalker
Copy link
Collaborator

@lukedirtwalker lukedirtwalker commented Jan 30, 2019

A handler that is registered at the messenger will now have metrics by default.
There are three metrics:
in_calls_total: counts the total received requests.
in_results_total: counts the results of the received requests.
in_calls_latency: a histogram of the latency of the requests.
Analogous to in_* the current messenger metrics (for outgoing requests)
have been renamed to out_*.

Also fixes the registration of messenger metrics (init() doesn't work because
we don't want to register on the default Prometheus registerer).

Note that not all handlers always return the correct result/status,
that will be done in separate commits.

Contributes #2329, #2330, #1819


This change is Reviewable

Copy link
Contributor

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @kormat, @lukedirtwalker, and @sustrik)


go/lib/infra/modules/trust/metrics.go, line 37 at r1 (raw file):

)

func initMetrics() {

I would strongly prefer to have these metrics reported by the messanger itself (i.e. in go/lib/infra/messenger/metrics.go) So instead of current calls_total we would have incoming_calls_total and outgoing_calls_total etc. That would mean having a uniform metrics for all RPC calls, irrespective of which service does the RPC.

As for labelSrc we can log that as a separate metric.

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @kormat and @sustrik)


go/lib/infra/modules/trust/metrics.go, line 37 at r1 (raw file):

Previously, sustrik (Martin Sustrik) wrote…

I would strongly prefer to have these metrics reported by the messanger itself (i.e. in go/lib/infra/messenger/metrics.go) So instead of current calls_total we would have incoming_calls_total and outgoing_calls_total etc. That would mean having a uniform metrics for all RPC calls, irrespective of which service does the RPC.

As for labelSrc we can log that as a separate metric.

Done.

@lukedirtwalker lukedirtwalker changed the title trust handler: Add metrics Handlers: Add metrics Feb 18, 2019
Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1, 14 of 15 files at r2.
Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @kormat, @lukedirtwalker, and @sustrik)


go/lib/infra/metrics.go, line 30 at r2 (raw file):

// HandlerResult contains a result label and a status label.
type HandlerResult struct {

Please add godocs for the fields. It's not entirely clear when writing a handler what each field should contain, and why they are different.

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 15 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kormat, @lukedirtwalker, and @sustrik)

Copy link
Collaborator Author

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 2 unresolved discussions (waiting on @kormat, @scrye, and @sustrik)


go/lib/infra/metrics.go, line 30 at r2 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Please add godocs for the fields. It's not entirely clear when writing a handler what each field should contain, and why they are different.

Done.

Copy link
Contributor

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat and @scrye)

Copy link
Contributor

@sustrik sustrik left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kormat and @scrye)

Copy link
Contributor

@scrye scrye left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @kormat)

@lukedirtwalker lukedirtwalker merged commit 32bb054 into scionproto:master Feb 22, 2019
@lukedirtwalker lukedirtwalker deleted the pubTrustHandlerMetrics branch February 22, 2019 07:21
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.

3 participants