Conversation
Adds a new body send mode for gRPC traffic. Also adds a safe way for the ext_proc server to return OK status without losing data in FULL_DUPLEX_STREAMED and GRPC modes. See grpc/proposal#484 for context. Risk Level: Low Testing: N/A Docs Changes: Included in PR Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Mark D. Roth <roth@google.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Adds a new body send mode for gRPC traffic. Also adds a safe way for the ext_proc server to return OK status without losing data in FULL_DUPLEX_STREAMED and GRPC modes. See grpc/proposal#484 for context. Risk Level: Low Testing: N/A Docs Changes: Included in PR Release Notes: N/A Platform Specific Features: N/A --------- Signed-off-by: Mark D. Roth <roth@google.com> Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Mirrored from https://github.com/envoyproxy/envoy @ 7b3a632333b587c784aff65e72ff618ff034f331
| - [request_headers](https://github.com/envoyproxy/envoy/blob/cdd19052348f7f6d85910605d957ba4fe0538aec/api/envoy/service/ext_proc/v3/external_processor.proto#L76) | ||
| and | ||
| [response_headers](https://github.com/envoyproxy/envoy/blob/cdd19052348f7f6d85910605d957ba4fe0538aec/api/envoy/service/ext_proc/v3/external_processor.proto#L81). | ||
| Populated when sending client headers or server headers, respectively. |
There was a problem hiding this comment.
I know we talked about this, but what's the story around what headers should be included vs. not? Are we going to define which headers specifically? It would be good to call out exactly whether method/scheme/path/te/user-agent/message-type/etc, etc are supposed to be included or not. In Go many of these things are added by the transport on the way out or are removed by the transport on the way in. It would be great if we can specify: "only the things set by the application [plus X, Y, and Z, which some libraries may need to manually synthesize before sending to the ext_proc server]"
There was a problem hiding this comment.
Yeah, we need to add this for both ext_authz and ext_proc. I think @easwars was compiling a list of what headers we should document.
|
|
||
| #### Server-Side Metrics | ||
|
|
||
| The following server-side metrics will be exported: |
There was a problem hiding this comment.
Are there no labels that could be used on the server?
There was a problem hiding this comment.
Not that I can think of. If you have any suggestions of labels that you think might be useful here, I'm open to them.
There was a problem hiding this comment.
Aren't these actually per-call/attempt metrics, so should they have all the same labels as the default per-call metrics?
There was a problem hiding this comment.
We have the following labels on per-call metrics today:
- method: This seems reasonable to add if we think it will be useful.
- target (client only): This also seems reasonable to add if it will be useful (client only).
- status: I don't think this one makes sense, because we don't know the status of the data plane RPC at the point at which we're updating these metrics.
- the new per-RPC label being introduced in A108: I'd be okay with this one too.
I'm open to adding all but status, as long we think they'll be useful and are not worried about performance issues in Java or Go.
A93-xds-ext-proc.md
Outdated
| The ext_authz filter will export metrics using the non-per-call metrics | ||
| architecture defined in [A79]. There will be a separate set of metrics |
There was a problem hiding this comment.
Oh maybe the implication is that it will get all of those labels already? If so we should call that out, probably?
There was a problem hiding this comment.
Not sure what you mean by "all of those labels". A79 doesn't define any labels that are intended to be applied to all metrics.
Maybe you meant to refer to the per-call metric labels? If so, I've answered that below.
3 participants
No description provided.