examples: add example for ORCA load reporting #6114
Conversation
examples/features/orca/README.md
Outdated
| pieces are optional and work independently. For per-RPC metrics to be | ||
| reported, two things are important: 1. using `orca.CallMetricsServerOption()` | ||
| when creating the server and 2. setting metrics in the method handlers by using | ||
| `orca.CallMetricRecorderFromContext()`. For out-of-band metrics, one simply | ||
| needs to create and register the reporter by using `orca.Register()` and set | ||
| metrics on the returned `orca.Service` using its methods. |
There was a problem hiding this comment.
Could you do bulleted lists for this to make it easier to read.
There was a problem hiding this comment.
Rewrote as part of above to not have #s anymore.
examples/features/orca/README.md
Outdated
|
|
||
| ## Explanation | ||
|
|
||
| The server is set up to report query cost metrics in its RPC handler. It also |
There was a problem hiding this comment.
I feel that it might be easier to read if you split this section into per-rpc metrics and oob metrics. And then talk about the APIs, what the server does and what the client does per section.
| pb.RegisterEchoServer(s, &server{}) | ||
|
|
||
| // Register the orca service for out-of-band metric reporting, and set the | ||
| // maximum reporting frequency to once every 3 seconds. Note that, by |
There was a problem hiding this comment.
This is the only place where maximum is used. Should this also be minimum?
There was a problem hiding this comment.
Higher frequency = faster = shorter interval. Confusing. I'll just remove any mention of "frequency".
| sc.Connect() | ||
|
|
||
| // Register a simple ORCA OOB listener on the SubConn. We request a 1 | ||
| // second report interval, but in this example the minimum interval is 3 |
There was a problem hiding this comment.
A small clarification to say that the 3s interval is set by the server would be useful I feel.
| conn, err := grpc.Dial(*addr, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"orca_example":{}}]}`), | ||
| ) |
There was a problem hiding this comment.
Nit: Even though this is just an example, it would be nice if we don't split up lines this way. We should either have everything on a single line, or define a dial options slice to initialize the dial options.
There was a problem hiding this comment.
Why? This seems like purely personal preference. We do this in other places in our examples, too, like:
grpc-go/examples/features/load_balancing/client/main.go
Lines 75 to 79 in 5796c40
There was a problem hiding this comment.
It is part of the style guide: https://g3doc.corp.google.com/go/g3doc/style/guide.md?cl=head#line-length
There was a problem hiding this comment.
Discussed offline & agreed to leave this.
| pb.RegisterEchoServer(s, &server{}) | ||
|
|
||
| // Register the orca service for out-of-band metric reporting, and set the | ||
| // maximum reporting frequency to once every 3 seconds. Note that, by |
There was a problem hiding this comment.
Higher frequency = faster = shorter interval. Confusing. I'll just remove any mention of "frequency".
examples/features/orca/README.md
Outdated
|
|
||
| ## Explanation | ||
|
|
||
| The server is set up to report query cost metrics in its RPC handler. It also |
examples/features/orca/README.md
Outdated
| pieces are optional and work independently. For per-RPC metrics to be | ||
| reported, two things are important: 1. using `orca.CallMetricsServerOption()` | ||
| when creating the server and 2. setting metrics in the method handlers by using | ||
| `orca.CallMetricRecorderFromContext()`. For out-of-band metrics, one simply | ||
| needs to create and register the reporter by using `orca.Register()` and set | ||
| metrics on the returned `orca.Service` using its methods. |
There was a problem hiding this comment.
Rewrote as part of above to not have #s anymore.
| conn, err := grpc.Dial(*addr, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"orca_example":{}}]}`), | ||
| ) |
There was a problem hiding this comment.
Why? This seems like purely personal preference. We do this in other places in our examples, too, like:
grpc-go/examples/features/load_balancing/client/main.go
Lines 75 to 79 in 5796c40
| sc.Connect() | ||
|
|
||
| // Register a simple ORCA OOB listener on the SubConn. We request a 1 | ||
| // second report interval, but in this example the minimum interval is 3 |
| The client performs one RPC per second. Per-RPC metrics are available for each | ||
| call via the `Done()` callback returned from the LB policy's picker. |
There was a problem hiding this comment.
Most of the other discussion here is about ORCA in general, the two types of reporting metrics, and the API in general. This line The client performs one RPC per second seems to be out of place. It seems to suddenly talk about the example.
There was a problem hiding this comment.
I don't understand. The whole section is about the example.
"The server is set up to report query cost metrics in its RPC handler. <explanation of how>"
"The client performs one RPC per second. <explanation of how it receives metrics related to those RPCs>"
Can you be more specific about how you would reword this?
There was a problem hiding this comment.
Actually looks good. Not sure what I was confused about.
| conn, err := grpc.Dial(*addr, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"orca_example":{}}]}`), | ||
| ) |
There was a problem hiding this comment.
It is part of the style guide: https://g3doc.corp.google.com/go/g3doc/style/guide.md?cl=head#line-length
cc @temawi
RELEASE NOTES: