Skip to content
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

api: create OpenRCA service proto file #6497

Merged

Conversation

voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Apr 5, 2019

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Created OpenRCA service proto file based on ORCA design
Risk Level: Low
Testing: None
Docs Changes: None
Release Notes: API: created service proto for Open Request Cost Aggregation
[Optional Fixes #Issue]
[Optional Deprecated:]

@voidzcy voidzcy force-pushed the feature/create_orca_service_proto_file branch 2 times, most recently from 4e89193 to 581ea1f Compare April 7, 2019 23:56
@htuch htuch self-assigned this Apr 8, 2019
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @voidzcy, I'm so excited to see ORCA finally making its way out to the world. I have some minor comments, the only major things to do are (1) decide where to put this in the Envoy API tree and (2) get the design doc published and GH issue opened so we can have others provide input into this. (1) we can discuss in this PR. For (2), I can own this.

api/envoy/service/load_stats/v2alpha/BUILD Outdated Show resolved Hide resolved
api/envoy/service/load_stats/v2alpha/orca.proto Outdated Show resolved Hide resolved
message LoadReportRequest {
// Interval for generating Open RCA core metric responses.
google.protobuf.Duration report_interval = 1;
// Request costs to collect. If this is empty, all known requests costs tracked by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand in this comment on how request cost names are specified? I think they probably should follow reverse DNS, e.g. com.acme.some_app.queue_length.

api/envoy/service/load_stats/v2alpha/orca.proto Outdated Show resolved Hide resolved
api/envoy/service/load_stats/v2alpha/orca.proto Outdated Show resolved Hide resolved

import "validate/validate.proto";

// Out-of-band (OOB) load reporting service for frontends to periodically get
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need a bunch more comments here. Let's discuss tomorrow on IM, but I think the first step is for me to create a public version of the design doc and share this as an Envoy issue, then we can crib from that.

@voidzcy
Copy link
Contributor Author

voidzcy commented Apr 9, 2019

@htuch Thanks for your help! I will fix the PR once we have an officially finalized design published. Look forward to seeing that!

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6614 is now filed, so let's continue the review. My main suggestion is to put this under udpa (universal data plane API), since that's the new location for things (starting in this PR ;) ) that are not Envoy specific. @mattklein123 do you concur?
/wait

api/envoy/data/orca/v1/BUILD Outdated Show resolved Hide resolved
api/envoy/service/orca/v1/BUILD Outdated Show resolved Hide resolved
api/envoy/data/orca/v1/orca_load_report.proto Outdated Show resolved Hide resolved
api/envoy/service/orca/v1/orca.proto Outdated Show resolved Hide resolved
@mattklein123
Copy link
Member

@mattklein123 do you concur?

Yes, +1. I will review once we move the files. Thank you.

@mattklein123 mattklein123 self-assigned this Apr 17, 2019
@voidzcy voidzcy force-pushed the feature/create_orca_service_proto_file branch from c554cad to f51e2bd Compare April 17, 2019 05:15
@voidzcy voidzcy force-pushed the feature/create_orca_service_proto_file branch from f51e2bd to 7431bc3 Compare April 17, 2019 05:17
// during the request.
double mem_utilization = 2 [(validate.rules).double.gte = 0, (validate.rules).double.lte = 1];

// Application specific requests costs. Values may be absolute costs (e.g.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you copy + paste the updated comment from the public ORCA doc here? I made some grammar changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.


import "validate/validate.proto";

// Out-of-band (OOB) load reporting service for the additional load reporting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a pointer and reference to the public design doc and relevant section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.


import "validate/validate.proto";

message OrcaLoadReport {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a pointer and reference to the public design doc and relevant section?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

voidzcy added 10 commits April 17, 2019 13:24
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
@voidzcy voidzcy force-pushed the feature/create_orca_service_proto_file branch 2 times, most recently from 8daf37f to c325921 Compare April 17, 2019 21:10
@voidzcy voidzcy force-pushed the feature/create_orca_service_proto_file branch from c325921 to bcd8516 Compare April 17, 2019 21:11
Signed-off-by: Chengyuan Zhang <chengyuanzhang@google.com>
@voidzcy voidzcy force-pushed the feature/create_orca_service_proto_file branch from bcd8516 to 26c75a6 Compare April 17, 2019 22:05
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@htuch htuch merged commit 5cb2229 into envoyproxy:master Apr 18, 2019
mpuncel added a commit to mpuncel/envoy that referenced this pull request Apr 19, 2019
* master: (26 commits)
  docs: update docs to recommend /retest repokitteh command (envoyproxy#6655)
  http timeout integration test: wait for 15s for upstream reset (envoyproxy#6646)
  access log: add response code details to the access log formatter (envoyproxy#6626)
  build: add ppc build badge to README (envoyproxy#6629)
  Revert dispatcher stats (envoyproxy#6649)
  Batch implementation with timer (envoyproxy#6452)
  fault filter: reset token bucket on data start (envoyproxy#6627)
  event: update libevent dependency to fix race condition (envoyproxy#6637)
  examples: standardize docker-compose version and yaml extension (envoyproxy#6613)
  quiche: Implement SpdyUnsafeArena using SpdySimpleArena (envoyproxy#6612)
  router: support customizable retry back-off intervals (envoyproxy#6568)
  api: create OpenRCA service proto file (envoyproxy#6497)
  ext_authz: option for clearing route cache of authorized requests (envoyproxy#6503)
  build: update jinja to 2.10.1. (envoyproxy#6623)
  tools: check spelling in pre-push hook (envoyproxy#6631)
  security: blameless postmortem template. (envoyproxy#6553)
  Implementing Endpoint lease for ClusterLoadAssigment (envoyproxy#6477)
  add HTTP integration tests exercising timeouts (envoyproxy#6621)
  event: fix DispatcherImplTest::InitializeStats flake (envoyproxy#6619)
  Add tag extractor for RDS route config name (envoyproxy#6618)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants