-
Notifications
You must be signed in to change notification settings - Fork 267
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
Metrics Service Support #220
Metrics Service Support #220
Conversation
Signed-off-by: Rama <ramaraochavali@gmail.com>
@mattklein123 Sorry. Had to create a new PR to get around the DCO issues. Here is the original PR #210. Please go through the PR for more details |
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@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.
Great, thanks for making this happen.
bazel/repositories.bzl
Outdated
visibility = ["//visibility:public"], | ||
) | ||
|
||
py_proto_library( |
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.
I would skip the Python rules unless someone is going to use them for now. Bazel really should get native py_proto_library support that builds on proto_library to make this cleaner, see bazelbuild/bazel#2626 and bazelbuild/bazel#3935.
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.
How do I skip? Initially I did not have. Then I got into some py related error. So I thought it is mandatory to have py and added this.
api/metrics_service.proto
Outdated
import "google/api/annotations.proto"; | ||
import "metrics.proto"; | ||
|
||
//Service to fetch metrics from Proxy. This uses Promotheus data model to represent metrics returned. |
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.
Nit: // Service
.
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.
Will change
api/metrics_service.proto
Outdated
service MetricsService { | ||
rpc FetchMetrics (MetricsRequest) returns (MetricsResponse) { | ||
option (google.api.http) = { | ||
post: "/v2/metrics" |
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.
FYI, unlike the other services in data-plane-api
, this is the first to be served by Envoy rather than have Envoy act as client. I'm wondering if we somehow want to distinguish this namespace wise. Also, can you clarify if this is intended to be a gRPC service, REST or combination?
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.
I do not have strong opinion on putting it in different name space. I was originally thinking of it as a gRPC service. For REST, I think the admin end point with ?format=json is sufficient I guess.
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama <ramaraochavali@gmail.com>
@ramaraochavali thanks for bearing with us here. :) I still think think is a major decision point. Is this We should be clear about what we are going after here. If we do b), I would prefer to do this in the context of https://github.com/envoyproxy/data-plane-api/issues/158 and to define a new admin namespace per @htuch where we can start specifying protos for all the admin output. |
Signed-off-by: Rama <ramaraochavali@gmail.com>
@htuch I made the py optional only for metrics and build passes. |
@mattklein123 No issues. I see your point. I think a) is more valuable in data-plane-api context. While b) may be good - it is in the context of admin. I think we should go after a). I am actually modifying the service def accordingly. I am just setting bazel stuff trying to compile with promotheus proto with basic service def. |
Signed-off-by: Rama <ramaraochavali@gmail.com>
@mattklein123 I have changed it to suit the definition for a). Please review. I also think b) might also have some value but we can do it under #158 may be if needed. |
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. I think this is a useful addition. @rshriram et al, does this look ok from Istio side? Anyone else have any thoughts on this?
api/metrics_service.proto
Outdated
// Service for streaming metrics to connected end point. | ||
service MetricsService { | ||
rpc StreamMetrics(stream StreamMetricsMessage) returns (StreamMetricsResponse) { | ||
|
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.
mega nit: I would del newline.
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 is generally useful. Would like some opinion from @douglas-reid or @ZackButcher
api/metrics_service.proto
Outdated
import "metrics.proto"; | ||
|
||
// Service for streaming metrics to connected end point. | ||
service MetricsService { |
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.
Does it make sense to explicitly call out PrometheusMetricsService ? After all the format is for Prometheus. If not this, atleast have a type field or something in streammessage that says what type of metric is coming out of Envoy.
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.
I think we discussed it in the PR #210 that we do not want to call this PrometheusMetricsService. We picked Promotheus model because it is more comprehensive representation of metrics model. We tried to look at OpenMetrics but since it is far off and any way is heavily inspired from Promotheus model. So we decided to go with this model for now.
api/metrics_service.proto
Outdated
message StreamMetricsResponse {} | ||
|
||
message StreamMetricsMessage { | ||
repeated io.prometheus.client.MetricFamily proxy_metrics = 1; |
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.
envoy_metrics. We don’t care if this is proxy or sidecar or LB or anything :).
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.
will change
@rshriram et al. I will look tonight. Am currently OOO.
On Oct 31, 2017 3:09 PM, "Shriram Rajagopalan" <notifications@github.com> wrote:
*@rshriram* approved this pull request.
This is generally useful. Would like some opinion from @douglas-reid
<https://github.com/douglas-reid> or @ZackButcher
<https://github.com/zackbutcher>
------------------------------
In api/metrics_service.proto
<#220 (comment)>
:
@@ -0,0 +1,19 @@
+syntax = "proto3";
+
+package envoy.api.v2;
+
+import "google/api/annotations.proto";
+import "metrics.proto";
+
+// Service for streaming metrics to connected end point.
+service MetricsService {
Does it make sense to explicitly call out PrometheusMetricsService ? After
all the format is for Prometheus. If not this, atleast have a type field or
something in streammessage that says what type of metric is coming out of
Envoy.
------------------------------
In api/metrics_service.proto
<#220 (comment)>
:
+package envoy.api.v2;
+
+import "google/api/annotations.proto"
;
+import "metrics.proto";
+
+// Service for streaming metrics to connected end point.
+service MetricsService {
+ rpc StreamMetrics(stream StreamMetricsMessage) returns
(StreamMetricsResponse) {
+
+ }
+}
+
+message StreamMetricsResponse {}
+
+message StreamMetricsMessage {
+ repeated io.prometheus.client.MetricFamily proxy_metrics = 1;
envoy_metrics. We don’t care if this is proxy or sidecar or LB or anything
:).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#220 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AUKx3c-kWdxHz0ENqXKTT16HQPGZWz9lks5sx5qUgaJpZM4QMMtw>
.
|
Signed-off-by: Rama <ramaraochavali@gmail.com>
@mattklein123 do we need identifier like
|
I enabled go compilation and am running in to the following build problem |
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.
Yes, I think we should Node
as an optional field to be included in the first field, for consistency with other APIs. Looks good other than remaining comments.
api/metrics_service.proto
Outdated
import "google/api/annotations.proto"; | ||
import "metrics.proto"; | ||
|
||
// Service for streaming metrics to connected end point. |
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.
Can you please change the wording here to describe in more detail what the endpoint is, and also update README.md
in the root directory to describe this new service that we are connecting to?
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.
Will change..
Signed-off-by: Rama <ramaraochavali@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.
This looks reasonable for Istio usage.
bazel/repositories.bzl
Outdated
@@ -1,4 +1,5 @@ | |||
GOOGLEAPIS_SHA = "5c6df0cd18c6a429eab739fb711c27f6e1393366" # May 14, 2017 | |||
PROMETHEUS_SHA = "6f3806018612930941127f2a7c6c453ba2c527d2" |
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.
nit: might be worth annotating with date.
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.
will do
api/metrics_service.proto
Outdated
import "google/api/annotations.proto"; | ||
import "metrics.proto"; | ||
|
||
// Service for streaming metrics to server that consumes the metrics data. It uses Prometheus metric data model as a standard to represent metrics information. |
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.
nit: please line break around 100 cols
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.
will do
api/metrics_service.proto
Outdated
|
||
message StreamMetricsMessage { | ||
// The node sending the metric messages over the stream. | ||
Node node = 1; |
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.
Can you take a look at my access log API and copy what we did there with Identifier? We only need to send node once.
api/metrics_service.proto
Outdated
import "metrics.proto"; | ||
|
||
// Service for streaming metrics to server that consumes the metrics data. It uses Prometheus metric data model as a standard to represent metrics information. | ||
service MetricsService { |
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.
You will also need to provide a config struct with at least cluster in it. See access log API.
Signed-off-by: Rama <ramaraochavali@gmail.com>
@mattklein123 I think addressed all comments and should be good to go. Can you please look at it? |
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. Thank you! Any final comments from anyone else?
Lgtm.
…On Thu, Nov 2, 2017 at 12:29 PM Matt Klein ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM. Thank you! Any final comments from anyone else?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#220 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AH0qd4H1zGcTE0rv6SLGoOnacK6hBtHRks5sye34gaJpZM4QMMtw>
.
|
@@ -0,0 +1,39 @@ | |||
syntax = "proto3"; | |||
|
|||
package envoy.api.v2; |
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.
Nit: blank line after package.
api/metrics_service.proto
Outdated
} | ||
|
||
// Identifier data that will only be sent in the first message on the stream. This is effectively | ||
// structured metadata and is a performance optimization. |
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.
Nit: this should probably be two sentences, the conjunction of the description of metadata and then the performance optimization note doesn't make sense.
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.
Changed
Signed-off-by: Rama <ramaraochavali@gmail.com>
Signed-off-by: Rama ramaraochavali@gmail.com