Add grpc metrics #414

antgubarev · 2023-04-28T09:03:41Z

fix the issue #648

srenatus

Thanks for your contribution.

The code changes looks good to me -- but adding just any sort of test would be good.

internal/internal.go

antgubarev · 2023-04-28T12:24:07Z

Thanks for your contribution.

The code changes looks good to me -- but adding just any sort of test would be good.

@srenatus Done

srenatus · 2023-04-28T12:28:13Z

internal/internal_test.go

+	}
+	if output.Status.Code != int32(code.Code_OK) {
+		t.Fatal("Expected request to be allowed but got:", output)
+	}


I don't know if the test scaffolding is capable enough -- but could we check that the desired prometheus metric is actually exported, and has a non-zero value after this incoming request?

There seems to be a panic in this test https://github.com/open-policy-agent/opa-envoy-plugin/actions/runs/5271008838/jobs/9531286549?pr=414#step:3:262

antgubarev · 2023-05-02T06:14:05Z

@srenatus @ashutosh-narkar Could you please review PR and my last fixes

ashutosh-narkar

Thanks for working on this @antgubarev. Few comments inline. Also these metrics should be part of the status update and decision log. It would be great if we have a test to verify that.

ashutosh-narkar · 2023-05-02T18:37:47Z

internal/internal.go

+	GRPCMaxRecvMsgSize      int  `json:"grpc-max-recv-msg-size"`
+	GRPCMaxSendMsgSize      int  `json:"grpc-max-send-msg-size"`
+	SkipRequestBodyParse    bool `json:"skip-request-body-parse"`
+	EnablePrometheusMetrics bool `json:"enable-prometheus-metrics"`


Nit: Can we call this something like EnablePerformanceMetrics

ashutosh-narkar · 2023-05-02T19:29:40Z

internal/internal.go

@@ -237,6 +240,15 @@ func (p *envoyExtAuthzGrpcServer) listen() {
 		addr = "grpc://" + addr
 	}

+	if p.cfg.EnablePrometheusMetrics {
+		summaryAuthzDuration := prometheus.NewSummary(prometheus.SummaryOpts{
+			Name: "grpc_authz_request_duration_seconds",


Can you please help me understand the reason to use NewSummary v/s something like SummaryVec or HistogramVec? The latter would be useful if we want aggregates afaik. Similar to this.

Nit: Can we change grpc_authz_request_duration_seconds to grpc_request_duration_seconds

We have only one value. Vec is for multiple values. It's a single reason. I will change to SummaryVec if you want.

If we have multiple instances of the plugin and we need to aggregate the results, will we be able to do this?

Also is there any impact from the change in default behavior of Summary as indicated in this comment? It would be helpful if we have more tests in any case.

// A Summary captures individual observations from an event or sample stream and // summarizes them in a manner similar to traditional summary statistics: 1. sum // of observations, 2. observation count, 3. rank estimations. // // A typical use-case is the observation of request latencies. By default, a // Summary provides the median, the 90th and the 99th percentile of the latency // as rank estimations. However, the default behavior will change in the // upcoming v1.0.0 of the library. There will be no rank estimations at all by // default. For a sane transition, it is recommended to set the desired rank // estimations explicitly. // // Note that the rank estimations cannot be aggregated in a meaningful way with // the Prometheus query language (i.e. you cannot average or add them). If you // need aggregatable quantiles (e.g. you want the 99th percentile latency of all // queries served across all instances of a service), consider the Histogram // metric type. See the Prometheus documentation for more details. // // To create Summary instances, use NewSummary.

Yes, you're right. I fixed it. Thanks!

ashutosh-narkar · 2023-05-17T22:06:08Z

@antgubarev please let us know if you need any help with this. This would be a good one to get in.

antgubarev · 2023-05-18T05:54:40Z

@antgubarev please let us know if you need any help with this. This would be a good one to get in.

Thanks! I don't need any help but I need free time ) I'm going to finish it in the next few days.

antgubarev · 2023-05-25T19:23:17Z

@antgubarev please let us know if you need any help with this. This would be a good one to get in.

@ashutosh-narkar I can't find a way to test status plugin :( . Could you help me?

antgubarev · 2023-05-31T05:44:20Z

@ashutosh-narkar @srenatus Could you give your opinion about this PR

ashutosh-narkar · 2023-06-03T00:48:02Z

Thanks for working on this @antgubarev. I'll take a look next week.

ashutosh-narkar · 2023-06-07T20:39:04Z

README.md

+    grpc-max-recv-msg-size: 40194304 # default: 1024 * 1024 * 4
+    grpc-max-send-msg-size: 2147483647 # default: max Int
+    skip-request-body-parse: false # default: false
+    enable-performance-metrics: false # default: false. Adds `grpc_request_duration_seconds` prometheus histogram metric 


@antgubarev here we are stating that by enabling this we'll get a metric called grpc_request_duration_seconds which is a histogram metric. This looks good. But below we've defined grpc_request_duration_seconds as a prometheus.Summary which does not seem right. I would have imagined something like a prometheus.HistogramVec similar to what OPA does. This would be a more useful metric than a summary as users typically want to understand the latency distribution of their requests. WDYT?

@ashutosh-narkar We've already used prometheus.HistogramVec not Summary :)

https://github.com/open-policy-agent/opa-envoy-plugin/pull/414/files#diff-67213f39b84a013e7db04664cd3ad943b7a237d97b9c6e862d10806ac27def28R244 here we seem to be using Summary.

ashutosh-narkar

Thanks for making the changes @antgubarev. Can you please look into the test failure. Also these metrics should be part of the status update and decision log. It would be great if we have a test to verify that.

antgubarev · 2023-06-17T09:30:19Z

Thanks for making the changes @antgubarev. Can you please look into the test failure. Also these metrics should be part of the status update and decision log. It would be great if we have a test to verify that.

@ashutosh-narkar I'm sorry it's my fault. I accidentally reverted more commits than I needed. I fixed it!

ashutosh-narkar · 2023-06-29T00:18:27Z

@antgubarev thanks so much for working on this. The changes look good. Can you please squash your commits and rebase? Thanks.

Signed-off-by: Anton Gubarev <[email protected]>

antgubarev · 2023-06-30T12:52:37Z

@ashutosh-narkar Done. Thanks for your review.

ashutosh-narkar

Thanks for the contribution @antgubarev!

srenatus reviewed Apr 28, 2023

View reviewed changes

internal/internal.go Outdated Show resolved Hide resolved

internal/internal.go Outdated Show resolved Hide resolved

srenatus reviewed Apr 28, 2023

View reviewed changes

antgubarev requested review from srenatus and ashutosh-narkar May 1, 2023 11:27

ashutosh-narkar reviewed May 2, 2023

View reviewed changes

antgubarev requested a review from ashutosh-narkar May 29, 2023 19:56

ashutosh-narkar reviewed Jun 7, 2023

View reviewed changes

ashutosh-narkar reviewed Jun 16, 2023

View reviewed changes

antgubarev requested a review from ashutosh-narkar June 23, 2023 13:35

add grpc metrics

8bcacad

Signed-off-by: Anton Gubarev <[email protected]>

ashutosh-narkar approved these changes Jun 30, 2023

View reviewed changes

ashutosh-narkar merged commit 761f94d into open-policy-agent:main Jun 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add grpc metrics #414

Add grpc metrics #414

antgubarev commented Apr 28, 2023 •

edited

Loading

srenatus left a comment

antgubarev commented Apr 28, 2023 •

edited

Loading

srenatus Apr 28, 2023

antgubarev May 1, 2023

ashutosh-narkar Jun 16, 2023

antgubarev commented May 2, 2023 •

edited

Loading

ashutosh-narkar left a comment

ashutosh-narkar May 2, 2023

antgubarev May 3, 2023

ashutosh-narkar May 2, 2023

antgubarev May 3, 2023 •

edited

Loading

ashutosh-narkar May 4, 2023

antgubarev May 25, 2023

ashutosh-narkar commented May 17, 2023

antgubarev commented May 18, 2023

antgubarev commented May 25, 2023 •

edited

Loading

antgubarev commented May 31, 2023 •

edited

Loading

ashutosh-narkar commented Jun 3, 2023

ashutosh-narkar Jun 7, 2023

antgubarev Jun 9, 2023

ashutosh-narkar Jun 9, 2023

ashutosh-narkar left a comment

antgubarev commented Jun 17, 2023 •

edited

Loading

ashutosh-narkar commented Jun 29, 2023

antgubarev commented Jun 30, 2023

ashutosh-narkar left a comment

Add grpc metrics #414

Add grpc metrics #414

Conversation

antgubarev commented Apr 28, 2023 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

antgubarev commented Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgubarev commented May 2, 2023 • edited Loading

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgubarev May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented May 17, 2023

antgubarev commented May 18, 2023

antgubarev commented May 25, 2023 • edited Loading

antgubarev commented May 31, 2023 • edited Loading

ashutosh-narkar commented Jun 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

antgubarev commented Jun 17, 2023 • edited Loading

ashutosh-narkar commented Jun 29, 2023

antgubarev commented Jun 30, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

antgubarev commented Apr 28, 2023 •

edited

Loading

antgubarev commented Apr 28, 2023 •

edited

Loading

antgubarev commented May 2, 2023 •

edited

Loading

antgubarev May 3, 2023 •

edited

Loading

antgubarev commented May 25, 2023 •

edited

Loading

antgubarev commented May 31, 2023 •

edited

Loading

antgubarev commented Jun 17, 2023 •

edited

Loading