tracing: Add config support for a Tracing Decorator to enable operati…#170
tracing: Add config support for a Tracing Decorator to enable operati…#170objectiser wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
…on name to be specified for matched criteria
|
|
||
| // If specified, this is a regular expression match on the :path header | ||
| // once the query string is removed. | ||
| string regex = 3; |
There was a problem hiding this comment.
This is currently a placeholder, as in RouteMatch.
| oneof path_specifier { | ||
| // If specified, this is a prefix rule meaning that the prefix must | ||
| // match the beginning of the :path header. | ||
| string prefix = 1; |
There was a problem hiding this comment.
I like the idea, but does this need to be independent to the existing https://github.com/lyft/envoy-api/blob/master/api/rds.proto#L67 RouteMatch message? Can we attach a Decorator to a Route https://github.com/lyft/envoy-api/blob/master/api/rds.proto#L288 instead?
There was a problem hiding this comment.
So for example?
"routes": [
{
"timeout_ms": 0,
"prefix": "/trace",
"cluster": "service2",
"method": "GET",
"operation": "getTrace"
}
]
Then the method would become part of the routing criteria and potentially a route would need to be defined for each REST endpoint.
If that is the preference, then I could look at changing - but would be good to get opinions from others?
There was a problem hiding this comment.
If the user is going to have to basically build a route table inside the tracing config, why not just build a more specific route table as you demonstrate above, and just allow specifying an operation name on the route. (This could be used in the future for stats, logging, and other things potentially).
That seems better to me and more future proof. Any reason not to do that?
If we don't want to do that, I think we could probably move RouteMatch message out and use it in both places. (Note that you don't need to have a method specific match. You can do a header match on the :method header).
There was a problem hiding this comment.
Ok will rework to add to Route - will close this PR and reopen with new version.
…on name to be specified for matched criteria
Definitions associated with envoyproxy/envoy#1527