-
Notifications
You must be signed in to change notification settings - Fork 683
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
Add OpenMetrics(Prometheus) in the provider module #379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- 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)
- 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)
0434dfb
to
60b2d64
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
d476f2f
to
caa8e3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Going in good direction but still a few major things:
- We agreed on
openmetrics
I think? - 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 🤗
de7565b
to
aff09f4
Compare
I haven't tested the middleware locally, so I will have more questions when done 😛 |
Can we make it not a draft when ready for review? (: |
I haven't added the options, so technically isn't completed, will make it ready for review once I add the options. |
aff09f4
to
a49b4ff
Compare
@bwplotka |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a few comments. :)
a49b4ff
to
191948f
Compare
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 🎉 |
Let's leave the examples to be added in the next PR, would be nice to separate this from this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (:
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>
.
|
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
… reporter to utilise the timer call function of open metrics Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
…incorrect, also added some nits Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
…penmetrics Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
7c06b8e
to
82b77a4
Compare
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
Current Update: DefaultClientMetrics = openmetrics.NewClientMetrics(prometheus.DefaultRegisterer)
if err := DefaultClientMetrics.EnableClientStreamSendTimeHistogram; err != nil {
// Handle Error
}
|
Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
…rver/client metrics object with some default metrics initialisation Signed-off-by: Yash Sharma <yashrsharma44@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, one suggestion (:
// ClientMetrics represents a collection of metrics to be registered on a | ||
// Prometheus metrics registry for a gRPC client. | ||
type ClientMetrics struct { | ||
clientRegister openmetrics.Registerer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using clientRegister
in Enable...
functions. Should I modify the Enable..
functions to accept clientRegister?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm
need to look on this, happy to propose something on top of your PR (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally register is only needed on start!
func NewClientMetrics(clientRegistry prometheus.Registerer, counterOpts ...CounterOption) *ClientMetrics { | ||
opts := counterOptions(counterOpts) | ||
return &ClientMetrics{ | ||
clientRegister: clientRegistry, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern can be further simplified.
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, | |
.... | |
... | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds fair 👍
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! |
@yashrsharma44 @bwplotka
Please see add "metadata.NewOutgoingContext(" test case, you will see. refer:
|
All the prometheus metrics, or any specific one?
Thanks for reporting this, would be nice if you could propose a fix for this. I can add the fix in another PR. |
Fixes #346
Some questions:
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
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: