Add OpenMetrics(Prometheus) in the provider module

[yashrsharma44]

Fixes #346

Some questions:

• Should we integrate Prometheus in the metrics package, or provide it as another provider?

Have Prometheus as a separate provider, which follows Open Metrics convention, we need to decide what do we want to name the package though since the repo is Open Metrics compliant

• How should we add in options for enabling the histograms for different purposes?

This PR is a WIP PR, however suggestions regarding the implementation details are welcome because most of the structure of the code is final. The interface for the reporter has been tweaked a little for enabling the Timer feature from Prometheus.

cc @bwplotka @brancz

TODO:

• Write tests for the new provider
• Write docs for the same

[bwplotka]

Amazing work! I think it's literally what we need in the first iteration, but before full v2 let's address all pain points @brancz suggested in https://gophers.slack.com/archives/CNJL30P4P/p1610112810036200?thread_ts=1610112652.036000&cid=CNJL30P4P

You first question should be probably 2 questions to answer:

1. Do we want Prometheus as core vs provider?

I don't think we can put it in core, because Prometheus client might be not what user will use and what if user don't want to use metric interceptor at all? Then having in core will dictate them to have Prometheus dependency in code.

(Discussed in Thanos Contributor Hours)

2. IF we want Prometheus as a provider do we need any "metrics" interceptor in the core.

We have some similarities... like recording specific things.

There is a good rule which is YAGNI - unless there is already 3 or more specific "users" of the vendor-agnostic metric interceptor, there is no point creating one, because we don't know even if there will be any other "user" than Prometheus, and we don't know what they want.

(Discussed in Thanos Contributor Hours)

3 .Dario: Maybe open telemetry is a common thing that allows bridge.

My understanding is that the current state is that Open Telemetry does not support Open Metrics or actually anything without extra bridges (sidecars, agent), so open metrics via the most popular Prometheus client sounds like a good choice. Since it will separate module, this project will work with open telemetry just fine (: (in or out of this repo)

[codecov-io]

Codecov Report

Merging #379 (7b8ea9d) into v2 (a79558a) will decrease coverage by 27.83%.
The diff coverage is 37.48%.

@@             Coverage Diff             @@
##               v2     #379       +/-   ##
===========================================
- Coverage   83.58%   55.74%   -27.84%     
===========================================
  Files          30       37        +7     
  Lines         932     1322      +390     
===========================================
- Hits          779      737       -42     
- Misses        114      514      +400     
- Partials       39       71       +32     

Impacted Files	Coverage Δ
chain.go	0.00% <ø> (-90.91%)	⬇️
interceptors/auth/auth.go	100.00% <ø> (ø)
interceptors/auth/metadata.go	100.00% <ø> (ø)
interceptors/ratelimit/ratelimit.go	50.00% <0.00%> (-50.00%)	⬇️
interceptors/tags/fieldextractor.go	13.79% <0.00%> (-71.51%)	⬇️
testing/testpb/interceptor_suite.go	0.00% <0.00%> (ø)
testing/testpb/test.manual_extractfields.pb.go	0.00% <0.00%> (ø)
testing/testpb/test.manual_validator.pb.go	0.00% <0.00%> (ø)
testing/testpb/test.pb.go	36.92% <36.92%> (ø)
interceptors/logging/payload.go	67.18% <42.30%> (-15.43%)	⬇️
... and 45 more

---

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 3ff779b...7b8ea9d. Read the comment docs.

[bwplotka]

Nice! Going in good direction but still a few major things:

1. We agreed on openmetrics I think?
2. I removed initially StartTimeCall reporter method for a reason (: We can have much simpler code if (less methods in in terface = better!). See comments below.

Thanks for helping 🤗

[yashrsharma44]

I haven't tested the middleware locally, so I will have more questions when done 😛

[bwplotka]

Can we make it not a draft when ready for review? (:

[yashrsharma44]

I haven't added the options, so technically isn't completed, will make it ready for review once I add the options.

[yashrsharma44]

@bwplotka
I have added the options for enabling the histogram. PTAL whenever you are free 🤗

[onprem]

Overall LGTM, just a few comments. :)

[yashrsharma44]

I have added the tests for open metrics. The lint makes some suggestions, let me know if I need to address them.

PTAL whenever free @johanbrandhorst @bwplotka 🎉

[yashrsharma44]

Let's leave the examples to be added in the next PR, would be nice to separate this from this PR.

[bwplotka]

LGTM, but we need to fix module to point to newest github.com/grpc-ecosystem/go-grpc-middleware/v2 version (somehow). There many different ways, but build has to pass (:

[bwplotka]

That's one option. You can use replace comment to pin to commit from your own fork as well. Kind Regards, Bartek Płotka (@bwplotka)

…

On Thu, 28 Jan 2021 at 07:24, Yash Sharma ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In providers/openmetrics/go.mod <#379 (comment)> : > @@ -0,0 +1,13 @@ +module github.com/grpc-ecosystem/go-grpc-middleware/providers/openmetrics/v2 + +go 1.15 + +require ( + github.com/golang/protobuf v1.4.3 + github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.0.0-rc.2 I think I need to add the interceptor change in a different PR, and then tag that commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#379 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O5H662JIB6T5VGZJRTS4EGMNANCNFSM4WCDDM2Q> .

[yashrsharma44]

Current Update:
Thanks to @ash2k for reviewing and asking to remove the global state.
I have written the current setup which uses registry as a variable, and other functions associated with it.

DefaultClientMetrics = openmetrics.NewClientMetrics(prometheus.DefaultRegisterer)
if err := DefaultClientMetrics.EnableClientStreamSendTimeHistogram; err != nil {
   // Handle Error
}

• All the options for enabling the histogram is connected with the client metrics instance, while for using it in the interceptor, pass the registry as a variable.
• UnaryClientInterceptor takes in registry as a variable, because I didn't wanted to integrate the metrics object with reporter. So taking the registry as a variable.

[bwplotka]

Amazing, one suggestion (:

[bwplotka]

We can remove this (:

[yashrsharma44]

We are using clientRegister in Enable... functions. Should I modify the Enable.. functions to accept clientRegister?

[bwplotka]

Hmmm
need to look on this, happy to propose something on top of your PR (:

[bwplotka]

Ideally register is only needed on start!

[bwplotka]

This pattern can be further simplified.

Suggested change

	func NewClientMetrics(clientRegistry prometheus.Registerer, counterOpts ...CounterOption) *ClientMetrics {
	opts := counterOptions(counterOpts)
	return &ClientMetrics{
	clientRegister: clientRegistry,
	func NewClientMetrics(r prometheus.Registerer, counterOpts ...CounterOption) *ClientMetrics {
	opts := counterOptions(counterOpts)
	c := &ClientMetrics{
	r.MustRegister(
	c.clientStartedCounter,
	....
	...

[yashrsharma44]

Sounds fair 👍

[bwplotka]

Actually you know what. Let's merge. I think we are trying to do exactly what we wanted to avoid -> adding thousands of improvements to porting PR (: 🤗

Thanks @yashrsharma44 for your work

I will try to apply some fixes for registration on next PR if that's ok!

[ti]

@yashrsharma44 @bwplotka
There are two Fatal bug in This mr：

1. Lost server context when client call server with: metadata.NewOutgoingContext(ctx, md)
2. Metrics counter does not work

Please see add "metadata.NewOutgoingContext(" test case, you will see.

refer:

go-grpc-middleware/providers/openmetrics/reporter.go

Line 113 in 215af81

return r, context.Background()

[yashrsharma44]

@ti

Metrics counter does not work

All the prometheus metrics, or any specific one?

Lost server context when client call server with: metadata.NewOutgoingContext(ctx, md)

Thanks for reporting this, would be nice if you could propose a fix for this. I can add the fix in another PR.