Skip to content

Expose trace sampling controls in the public API #375

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
hausdorff wants to merge 6 commits into from
Closed

Expose trace sampling controls in the public API #375

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2c4373d
Expose trace sampling controls in the public API
hausdorff Dec 29, 2017
33e1cea
Move the trace sampling controls to HttpConnectionManager
hausdorff Dec 30, 2017
2ebd52a
Use dedicated Percentage message type for percentages
hausdorff Dec 30, 2017
5f4defe
Small documentation fixes
hausdorff Dec 30, 2017
6938a37
Docs fixes
hausdorff Dec 30, 2017
bec964a
Update documentation and percent data types
hausdorff Jan 3, 2018
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
16 changes: 16 additions & 0 deletions api/filter/network/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,22 @@ message HttpConnectionManager {
// populate the tag name, and the header value is 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;

// Global target percentage of requests that will be force traced if the
Copy link
Copy Markdown
Member

@htuch htuch Dec 30, 2017

Choose a reason for hiding this comment

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

Consider linking back and forth between these docs and https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/runtime.html?highlight=sample.

Copy link
Copy Markdown
Author

@hausdorff hausdorff Dec 30, 2017

Choose a reason for hiding this comment

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

Originally I didn't because this link went stale in the original issue, but we can add it if you like.

// x-client-trace-id header is set. Must be an integer number between 0 and
Copy link
Copy Markdown
Member

@htuch htuch Dec 30, 2017

Choose a reason for hiding this comment

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

The convention we have for headers in docs is *x-client-trace-id*.

Copy link
Copy Markdown
Author

@hausdorff hausdorff Dec 30, 2017

Choose a reason for hiding this comment

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

Fixed.

// 100.
uint32 client_enabled = 3 [(validate.rules).uint32.lte = 100];
Copy link
Copy Markdown
Member

@htuch htuch Dec 30, 2017

Choose a reason for hiding this comment

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

You will need to use wrapped types, i.e. google.protobuf.UInt32Value, to obtain the defaulting behavior specified in https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/runtime.html?highlight=sample. This is because proto3 can't distinguish between non-specified scalar values and their null values (0 in the case of uint32).

Copy link
Copy Markdown
Author

@hausdorff hausdorff Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so just so we're all clear, you're saying they need to be nullable so that we can detect when they're not set when we plumb it through to the runtime? (EDIT: I now see that they're not nullable, but they're null-check-able. Heh.)

Makes sense. Fixed.

Copy link
Copy Markdown
Member

@htuch htuch Dec 30, 2017

Choose a reason for hiding this comment

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

Yep.


// Global target percentage of requests that will be traced after all other
// checks have been applied (force tracing, sampling, etc.). Must be a
// number between 0 and
// 100.
Copy link
Copy Markdown
Member

@htuch htuch Dec 30, 2017

Choose a reason for hiding this comment

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

Please specify the default as done in https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/runtime.html?highlight=sample.

Copy link
Copy Markdown
Author

@hausdorff hausdorff Dec 30, 2017

Choose a reason for hiding this comment

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

Fixed.

uint32 global_enabled = 4 [(validate.rules).uint32.lte = 100];

// Global target percentage of requests that will be randomly traced.
// Specified as ten-thousandths of a percent (i.e., in 0.01% increments),
// using integer numbers in the range 0-10000.
Copy link
Copy Markdown
Member

@htuch htuch Dec 30, 2017

Choose a reason for hiding this comment

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

This is a problem in runtime as well, but I can't help but feel that we should have a standard practice for specifying percentages in the Envoy proto API. In some places, we use double, other places uint32, with different degrees of granularity. Ideally we would have a Percent message type in the API that would also capture the lte constraint.

@mattklein123 @wora for comment and possible addition to STYLE.md. I don't think there is anything actionable for this PR @hausdorff.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

+1. I'm sad we didn't do this in the original v2 conversion. Oh well. Might as well start now. Can we just add a common Percent message per @htuch? I would probably just use a double and clamp it between 0.0 and 1.0. Internally, we can convert to integer as needed (e.g., a number out of 10,000 or 100,000 or whatever).

Copy link
Copy Markdown
Author

@hausdorff hausdorff Dec 30, 2017

Choose a reason for hiding this comment

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

I didn't want to cause waves by having the API not match up with the implementation, but I'd also prefer the wrapping a double in a Percent message type. So let's do that.

Fixed.

uint32 random_sampling = 5 [(validate.rules).uint32.lte = 10000];
}

// Presence of the object defines whether the connection manager
Expand Down