Skip to content

tracing: Add config support for a Tracing Decorator to enable operati… #170

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.

Sign up for GitHub

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

Closed
objectiser wants to merge 1 commit into from
Closed

tracing: Add config support for a Tracing Decorator to enable operati… #170

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions api/filter/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,50 @@ message HttpConnectionManager {
// used to populate the tag value. The tag is created if the specified
// header name is present in the request’s headers.
repeated string request_headers_for_tags = 2;

// Criteria for identifying a request to be decorated
message DecoratorMatch {
// A path specifier must be present.
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;
Copy link
Copy Markdown
Member

@htuch htuch Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

@objectiser objectiser Sep 12, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 12, 2017

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

@objectiser objectiser Sep 12, 2017

Choose a reason for hiding this comment

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

Ok will rework to add to Route - will close this PR and reopen with new version.


// If specified, this is an exact path rule meaning that the path must
// exactly match the :path header once the query string is removed.
string path = 2;

// If specified, this is a regular expression match on the :path header
// once the query string is removed.
string regex = 3;
Copy link
Copy Markdown
Contributor Author

@objectiser objectiser Sep 12, 2017

Choose a reason for hiding this comment

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

This is currently a placeholder, as in RouteMatch.

}
// Indicates that prefix/path matching should be case insensitive. The default
// is true.
google.protobuf.BoolValue case_sensitive = 4;

// The HTTP method to match. If not specified, then method won't be considered
// as part of the matching.
enum HttpMethod {
UNDEFINED = 0;
OPTIONS = 1;
GET = 2;
HEAD = 3;
POST = 4;
PUT = 5;
DELETE = 6;
TRACE = 7;
CONNECT = 8;
}
HttpMethod method = 5;
}

message Decorator {
DecoratorMatch match = 1;

// The tracing operation (or span name) to be used for the matched criteria.
string operation = 2;
}
repeated Decorator decorators = 3;
}
Tracing tracing = 7;

Expand Down