add variadic counterOpts funcs to server and client metrics constructor #25

zwopir · 2017-09-21T14:00:23Z

we have the use case that we need to add const label pairs to the metrics. We are exposing a gRPC service w/ and w/o TLS and we want to partition the metrics on a label "schema". So I added a variadic option func to the metric constructors. As the options don't have many fields, I added option funcs for Namespace, Subsystem and Namespace as well.

If it makes sense to you, I would add some documentation for the option functions.

codecov-io · 2017-09-21T14:04:31Z

Codecov Report

Merging #25 into master will decrease coverage by 2.04%.
The diff coverage is 68.75%.

@@            Coverage Diff            @@
##           master     #25      +/-   ##
=========================================
- Coverage   79.45%   77.4%   -2.05%     
=========================================
  Files           7       8       +1     
  Lines         258     270      +12     
=========================================
+ Hits          205     209       +4     
- Misses         45      52       +7     
- Partials        8       9       +1

Impacted Files	Coverage Δ
client_metrics.go	`74.44% <100%> (+0.28%)`	⬆️
server_metrics.go	`76.69% <100%> (+1.69%)`	⬆️
metric_options.go	`16.66% <16.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dafe0d...d35d21b. Read the comment docs.

zwopir · 2017-09-29T14:57:53Z

Hi grpc-ecosystem-team,

is there a way to get this PR reviewed/merged? I'd be glad to receive feedback - ideally a PR approval.

Best, Christoph

googlebot · 2017-09-29T14:57:56Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

zwopir · 2017-09-29T15:00:35Z

I signed it

…

Am 29.09.2017 um 16:57 schrieb googlebot ***@***.***>: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign. Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#25 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFz_9rGJw3NRwnCDsP3QBaF3F0I-_Zoiks5snQV0gaJpZM4PfXVt>.

googlebot · 2017-09-29T15:00:38Z

CLAs look good, thanks!

atetubou · 2017-10-31T14:27:02Z

server_metrics.go

@@ -61,6 +62,24 @@ func WithHistogramBuckets(buckets []float64) HistogramOption {
 	return func(o *prom.HistogramOpts) { o.Buckets = buckets }
 }

+func WithHistogramConstLabels(labels prom.Labels) HistogramOption {


Why this function is not in metric_options.go and not used?

I found the other WithXXX() HistogramOption functions in server_metrics.go, so I decided to put my one here too. You are right, they are all not specific to server instrumentation, so I would move all funcs returning a
HistogramOption to metric_options.go if you agree.

Why they're not used? I hope I get your question right: all of the WithXXX() functions are not used. They are functions a package user can use to configure the metrics.

I see. I prefer to move all WithXXX() option to metric_options.go.
Writing these functions in this file feels confusing.

I asked you why not used this function because I only saw New{Sever,Client}Metrics function. I got now, it is wrong comment. Sorry.

zwopir · 2017-11-01T09:29:01Z

@atetubou I moved the HistogramOption type and option setters (WithXXX) to metric_options.go. Thanks for reviewing this PR!

mwitkow · 2017-11-04T08:53:08Z

Can't you achieve it with a separate Registry that will add your namespace? @brancz is probably better placed for this

brancz · 2017-11-04T09:51:01Z

Unfortunately the Prometheus golang client doesn't implement metric.WithPrefix yet, but it's currently being worked on, so it's just a matter of time to land.

Aside from that, I wonder why you want to prefix these metrics additionally, it would mean that dashboards, alerting rules etc. all have to be duplicated because of that prefix, while the queries are actually the same.

dominikschulz · 2017-11-04T10:01:38Z

Actually this PR is more about adding const labels. As stated in the original PR adding namespace and subsystem opts was done as a convenience. We can remove this if they concern you. We only care about the const labels.

brancz · 2017-11-04T10:44:35Z

In that case I think those should be removed as proper support in the golang client is already being worked on, the additional labels make sense though (although this could also be something that could be addressed by the golang client but haven’t heard it being discussed so far).

zwopir · 2017-11-04T11:18:30Z

I removed the WithSubsystem and WithNamespace options as they will be obsolete if the client supports it directly.

zwopir · 2017-11-17T16:23:55Z

@mwitkow can I do something to close (preferably with a merge ;) ) this issue? anything missing?

brancz · 2017-11-17T18:24:31Z

lgtm 👍

mwitkow · 2018-03-19T09:30:13Z

Thanks! Sorry it took so long, I went a little dark on my open source contrib work.

zwopir · 2018-03-19T09:44:58Z

thanks @mwitkow!

Christoph Oelmüller added 2 commits September 21, 2017 15:41

add variadic counterOpts funcs to server and client metrics constructor

687b090

add const label option to histogram

1c7ed06

atetubou reviewed Oct 31, 2017

View reviewed changes

move HistogramOption type and WithXXX funcs to metric_options

b7836d8

remove WithSubsystem and WithNamespace options

d35d21b

mwitkow merged commit 031b1d4 into grpc-ecosystem:master Mar 19, 2018

brancz mentioned this pull request Apr 17, 2018

New release #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add variadic counterOpts funcs to server and client metrics constructor #25

add variadic counterOpts funcs to server and client metrics constructor #25

zwopir commented Sep 21, 2017

codecov-io commented Sep 21, 2017 •

edited

Loading

zwopir commented Sep 29, 2017

googlebot commented Sep 29, 2017

zwopir commented Sep 29, 2017 via email

googlebot commented Sep 29, 2017

atetubou Oct 31, 2017

zwopir Oct 31, 2017 •

edited

Loading

atetubou Nov 1, 2017

zwopir commented Nov 1, 2017

mwitkow commented Nov 4, 2017

brancz commented Nov 4, 2017

dominikschulz commented Nov 4, 2017

brancz commented Nov 4, 2017

zwopir commented Nov 4, 2017

zwopir commented Nov 17, 2017

brancz commented Nov 17, 2017

mwitkow commented Mar 19, 2018

zwopir commented Mar 19, 2018

add variadic counterOpts funcs to server and client metrics constructor #25

add variadic counterOpts funcs to server and client metrics constructor #25

Conversation

zwopir commented Sep 21, 2017

codecov-io commented Sep 21, 2017 • edited Loading

Codecov Report

zwopir commented Sep 29, 2017

googlebot commented Sep 29, 2017

zwopir commented Sep 29, 2017 via email

googlebot commented Sep 29, 2017

atetubou Oct 31, 2017

Choose a reason for hiding this comment

zwopir Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

atetubou Nov 1, 2017

Choose a reason for hiding this comment

zwopir commented Nov 1, 2017

mwitkow commented Nov 4, 2017

brancz commented Nov 4, 2017

dominikschulz commented Nov 4, 2017

brancz commented Nov 4, 2017

zwopir commented Nov 4, 2017

zwopir commented Nov 17, 2017

brancz commented Nov 17, 2017

mwitkow commented Mar 19, 2018

zwopir commented Mar 19, 2018

codecov-io commented Sep 21, 2017 •

edited

Loading

zwopir Oct 31, 2017 •

edited

Loading