API definitions for health check & outlier detection event services by baranov1ch · Pull Request #10407 · envoyproxy/envoy

baranov1ch · 2020-03-16T20:40:10Z

Description: this PR introduces gRPC services interfaces for outlier detection and healthcheck events services.
Risk Level: low
Testing: n.a.
Docs Changes: not yet
Release Notes: not yet
Relates to #8970

repokitteh-read-only · 2020-03-16T20:40:16Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10407 was opened by baranov1ch.

see: more, trace.

junr03

These two look very aligned with Access Log Service, and Metrics Service, so it generally looks good to me. Left a few comments.

I do wonder if there is anything we could be doing to reduce the boiler plate in this "reporting sink" services we have.

@htuch or @lizan do you want to take a look on the API front?

junr03 · 2020-03-16T23:23:37Z

@baranov1ch also please fix DCO, thanks!

junr03

lgtm, thanks for updating.

I still wonder about

I do wonder if there is anything we could be doing to reduce the boiler plate in this "reporting sink" services we have.

Given the increase in number of this type of services. Lets see what @htuch has to say.

htuch · 2020-03-17T18:52:57Z

How does this compare/contrast with HDS? Would be ideal to have a single way to tackle this.

I'll put in my 2 cents. In my mind this is different. This is an event reporting stream vs HDS is asking for information to change data plane behavior. So this is closer in nature to the other 3 event reporting services (metrics, logs, outlier) than it does with HDS. And it fits more nicely into the roll up we are describing in #10407 (comment)

htuch · 2020-03-17T18:53:42Z

Should we have a single Envoy event streaming service? It seems less boiler plate and implementation work.

+1, the events service could take a repeated oneof of events, or even could take typed Any objects?

As long as we allow each event source to specify an independent service it would remain flexible on how the deployment implements it.

Yeah, I wonder if we want a model like ADS where it's possible to have multiple event sources share a service. I'm thinking of a use case like Envoy anomaly detection, where everything gets fed in-sequence into a giant ML soup.

Yes exactly.

aggregated service seems good, but I wonder what would be the best way to configure such an endpoint. Should it be some sort of a global bootstrap-level endpoint, or per cluster?

I would still make it per source like you have now and just use a common service definition. IMO in the future we could add aggregation support?

Yep, this is what I was thinking it would look like. My other thought is that in V4 we could fold metrics service sink and the grpc log sink into the "event sink" service definition as well.

I've changed this to a single AggregatedEventService, PTAL

htuch · 2020-03-17T18:56:22Z

@junr03 yeah, I'm thinking that if we're going to add some event triggered streaming service in Envoy, that we have a generic protocol for this and adding a new event message/implementation is low overhead, without having to implement a new streaming service in Envoy or the API.

My request here would be for either more discussion in-PR or a short design doc. I think this could complement metrics/logging as a new service, but we shouldn't have N of these.

@envoyproxy/api-shepherds please weigh in on this one.

htuch · 2020-03-19T00:22:16Z

For future proofing, I would add a EventServiceConfig message to the API and use it everywhere that GrpcService appears right now. Then put a GrpcService in there. In the future, this will give you the ability to add aggregated streaming without breaking any of the config sites.

+1 good idea.

htuch · 2020-03-19T00:24:05Z

My $0.02 is for the naming EventReportingService (analogos to Load Reporting Service (LRS)). The reason AggregatedEventService might not be great is that it might be used in non-aggregated (today) and aggregated (future) ways on the same stream. If that sounds awful, ProxyEventService? IDK.

EventReportingService sgtm +1

htuch · 2020-03-19T00:29:13Z

Have you considered now or in the future adopting a pattern similar to LRS/HDS, where the event sink might want to negotiate the events of interest after the client (Envoy) advertises them in a request? If this makes sense, I'd make this fully bidi as a future proofing, even if we don't implement right now.

+1 for this. I think it would be useful to have the event sink decide that events it is interested in

mattklein123

In general this LGTM modulo remaining comments, thank you!

mattklein123 · 2020-03-19T21:45:38Z

+1 good idea.

mattklein123 · 2020-03-19T21:46:52Z

EventReportingService sgtm +1

htuch

Thanks, looks really generic and extensible now. A few more naming/package comments..
/wait

htuch · 2020-03-23T14:46:41Z

Nit: s/reporing/reporting/

htuch · 2020-03-23T14:47:15Z

Arguably could be in its own event_service_config.proto but I don't feel that strongly (it's easy enough to change later with breaking things).

sure, why not:)

htuch · 2020-03-23T14:48:37Z

I think this should have its own tree in the envoy.service hierarchy, e.g. envoy.service.event_reporting.v2alpha.

yep, good idea

htuch

Needs to pass CI check_format, but otherwise the API LGTM, thanks for iterating on this.

htuch

Thanks!

htuch · 2020-03-26T19:30:15Z

@baranov1ch also needs to merge master, to pickup change of location of api/docs/BUILD

Signed-off-by: Alexey Baranov <me@kotiki.cc>

repokitteh-read-only Bot added the api label Mar 16, 2020

baranov1ch force-pushed the remote-hc-outlier-events-api branch from e96377a to 10ef01f Compare March 16, 2020 20:42

mattklein123 assigned junr03 Mar 16, 2020

junr03 suggested changes Mar 16, 2020

View reviewed changes

baranov1ch force-pushed the remote-hc-outlier-events-api branch from e9a48d3 to 702467f Compare March 17, 2020 06:48

junr03 previously approved these changes Mar 17, 2020

View reviewed changes

htuch reviewed Mar 17, 2020

View reviewed changes

htuch self-assigned this Mar 17, 2020

baranov1ch dismissed junr03’s stale review via 7dab1f5 March 18, 2020 19:38

baranov1ch force-pushed the remote-hc-outlier-events-api branch 2 times, most recently from 7dab1f5 to e7282d1 Compare March 18, 2020 19:43

htuch reviewed Mar 19, 2020

View reviewed changes

mattklein123 requested changes Mar 19, 2020

View reviewed changes

mattklein123 added the waiting label Mar 19, 2020

baranov1ch force-pushed the remote-hc-outlier-events-api branch from e7282d1 to d56b4bd Compare March 22, 2020 07:43

repokitteh-read-only Bot removed the waiting label Mar 22, 2020

htuch suggested changes Mar 23, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Mar 23, 2020

baranov1ch force-pushed the remote-hc-outlier-events-api branch from d56b4bd to 4c28baf Compare March 23, 2020 23:17

repokitteh-read-only Bot removed the waiting label Mar 23, 2020

baranov1ch force-pushed the remote-hc-outlier-events-api branch from 4c28baf to 402e6df Compare March 24, 2020 07:09

htuch reviewed Mar 24, 2020

View reviewed changes

Comment thread api/envoy/config/bootstrap/v3/bootstrap.proto Outdated

baranov1ch force-pushed the remote-hc-outlier-events-api branch 3 times, most recently from 98a3fe1 to 6393fc6 Compare March 25, 2020 10:17

baranov1ch requested a review from mattklein123 March 25, 2020 15:02

htuch previously approved these changes Mar 26, 2020

View reviewed changes

Comment thread api/envoy/service/event_reporting/v2alpha/event_reporting_service.proto Outdated

repokitteh-read-only Bot removed the api label Mar 26, 2020

htuch added the waiting label Mar 27, 2020

API definitions for health check & outlier detection event services

15e607d

Signed-off-by: Alexey Baranov <me@kotiki.cc>

baranov1ch dismissed htuch’s stale review via 15e607d March 28, 2020 07:54

baranov1ch force-pushed the remote-hc-outlier-events-api branch from 6393fc6 to 15e607d Compare March 28, 2020 07:54

repokitteh-read-only Bot added api and removed waiting labels Mar 28, 2020

htuch approved these changes Mar 30, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Mar 30, 2020

htuch merged commit 34fcdef into envoyproxy:master Mar 30, 2020

This was referenced Apr 23, 2020

metrics service sink: add config option to report counters as deltas #10889

Merged

event emission: implement generic event reporting gRPC client #10966

Open

observability: allow "event sink" services to update configuration in-band #10968

Open

Conversation

baranov1ch commented Mar 16, 2020

Uh oh!

repokitteh-read-only Bot commented Mar 16, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

junr03 commented Mar 16, 2020

Uh oh!

junr03 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranov1ch Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Mar 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranov1ch Mar 18, 2020 •

edited

Loading