Deprecate int percent in favor of double percentage #609

venilnoronha · 2018-08-14T23:40:01Z

This PR deprecates the integer percent field in Delay and Abort types in favor of the new double percentage field which allows finer control.

This PR serves as a prereq for the changes necessary for istio/istio#7903.

Signed-off-by: Venil Noronha veniln@vmware.com

/cc @rshriram

This commit deprecates the integer percent field in Delay and Abort types in favor of the new FractionalPercent type which allows finer control. Signed-off-by: Venil Noronha <veniln@vmware.com>

istio-testing · 2018-08-14T23:40:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kyessenov

If they are not already assigned, you can assign the PR to them by writing /assign @kyessenov in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

venilnoronha · 2018-08-14T23:41:23Z

/assign @kyessenov

rshriram · 2018-08-15T16:37:05Z

networking/v1alpha3/virtual_service.proto

@@ -1001,6 +1011,9 @@ message HTTPFaultInjection {
      // $hide_from_docs
      string http2_error = 4;
    }
+
+    // Percentage of requests to be aborted with the error code provided.
+    FractionalPercent percentage = 5;


we shouldn't use the envoy model as it is here. Please use double or float and then derive the envoy level stuff automatically from the user specified value.

Addressed in fe10f32.

rshriram

change fractional percent to double and derive the fractional percent from it.

Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram · 2018-08-15T16:57:05Z

networking/v1alpha3/virtual_service.proto

@@ -938,17 +938,18 @@ message HTTPFaultInjection {
  //         subset: v1
  //     fault:
  //       delay:
-  //         percent: 10
+  //         percentage: 0.000001


Update the explanation above this example that says delay 10% of requests. It should say delay 1 out of every 1000 requests (and change the percentage to 0.001 everywhere). Do the same for all example texts that say 10%.

Addressed in 9628157.

kyessenov · 2018-08-15T17:05:35Z

Floats make me a bit uncomfortable. Note that these protos go through JSON translation that loses all number precision (since Javascript has only one number datatype), so that's some loss of precision during internal processing. I'd hate to find out that some small number like 0.000001 gets approximated to 0 internally in istio.

Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram · 2018-08-15T17:23:59Z

Do we know the bounds for this? I am happy supporting precision upto 3 digits or 4 digits. the clunky numerator/denominator thing is not helpful.

wora · 2018-08-15T19:36:36Z

Please consider using standard type defined by `envoy.type.Percent` package. It was created for this use case. While it may not be perfect, it is still better for Istio to align with Envoy.

* The "type" in envoy.type.Percent is renamed to "types" in istio.envoy.types.Percent to avoid a keyword conflict in Golang. * The Makefile splits the *.pb.go file generation for v2alpha1/*.proto and types/*.proto in order to avoid the "inconsistent package names" error in protoc-gen-go. Signed-off-by: Venil Noronha <veniln@vmware.com>

Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha · 2018-08-16T22:48:01Z

I've addressed the changes via 35280a4 and aee3bd2.

wora · 2018-08-17T04:51:46Z

Is there any reason we cannot use Envoy definition directly? I assume Istio has to trust Envoy anyway.

venilnoronha · 2018-08-17T05:42:51Z

As per the readme, this project should be independent of others.

rshriram · 2018-08-17T13:03:48Z

Makefile

+envoy_types_pb_pythons := $(envoy_types_protos:.proto=_pb2.py)
+envoy_v2alpha1_protos := $(shell find $(envoy_path) -type f -path '*/v2alpha1/*' -name '*.proto' | sort)
+envoy_v2alpha1_pb_gos := $(envoy_v2alpha1_protos:.proto=.pb.go)
+envoy_v2alpha1_pb_pythons := $(envoy_v2alpha1_protos:.proto=_pb2.py)


See golang/protobuf#39.

Attempting to generate *.pb.go for protos under different packages causes an error currently.

Revert all of this please

rshriram · 2018-08-17T14:08:00Z

envoy/types/percent.proto

+// Identifies a percentage, in the range [0.0, 100.0].
+message Percent {
+  double value = 1;
+}


rshriram · 2018-08-17T14:08:46Z

networking/v1alpha3/virtual_service.proto

@@ -958,11 +961,14 @@ message HTTPFaultInjection {
      // $hide_from_docs
      google.protobuf.Duration exponential_delay = 3 ;
    }
+
+    // Percentage of requests on which the delay will be injected.
+    istio.envoy.types.Percent percentage = 5;


define a Percent message in this API and use it. Don't want to import from another packages. As such proto generated docs are not that friendly.

Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha · 2018-08-17T16:38:48Z

@rshriram all comments have been addressed via 81320ce and 6418b55.

rshriram · 2018-08-17T19:23:26Z

/ok-to-test

Deprecate int percent in favor of FractionalPercent

1b2a492

This commit deprecates the integer percent field in Delay and Abort types in favor of the new FractionalPercent type which allows finer control. Signed-off-by: Venil Noronha <veniln@vmware.com>

istio-testing requested review from mandarjog and sebastienvas August 14, 2018 23:40

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 14, 2018

istio-testing added the needs-ok-to-test label Aug 14, 2018

istio-testing assigned kyessenov Aug 14, 2018

rshriram reviewed Aug 15, 2018

View reviewed changes

rshriram suggested changes Aug 15, 2018

View reviewed changes

Remove FractionalPercent and use double for percentage

fe10f32

Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha changed the title ~~Deprecate int percent in favor of FractionalPercent~~ Deprecate int percent in favor of double percentage Aug 15, 2018

rshriram reviewed Aug 15, 2018

View reviewed changes

Fix docs related to the percentage field

9628157

Signed-off-by: Venil Noronha <veniln@vmware.com>

venilnoronha added 2 commits August 16, 2018 13:44

Use istio.envoy.types.Percent for percentage

aee3bd2

Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram reviewed Aug 17, 2018

View reviewed changes

venilnoronha added 2 commits August 17, 2018 09:30

Revert copy envoy.type.Percent and its usage

81320ce

Signed-off-by: Venil Noronha <veniln@vmware.com>

Add and use Percent type in virtual_service.proto

6418b55

Signed-off-by: Venil Noronha <veniln@vmware.com>

rshriram approved these changes Aug 17, 2018

View reviewed changes

istio-testing removed the needs-ok-to-test label Aug 17, 2018

rshriram merged commit 214c759 into istio:master Aug 17, 2018

venilnoronha mentioned this pull request Aug 23, 2018

Membership? istio/istio#8193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate int percent in favor of double percentage #609

Deprecate int percent in favor of double percentage #609

venilnoronha commented Aug 14, 2018 •

edited

istio-testing commented Aug 14, 2018

venilnoronha commented Aug 14, 2018

rshriram Aug 15, 2018

venilnoronha Aug 15, 2018

rshriram left a comment

rshriram Aug 15, 2018

venilnoronha Aug 15, 2018

kyessenov commented Aug 15, 2018

rshriram commented Aug 15, 2018

wora commented Aug 15, 2018 via email

venilnoronha commented Aug 16, 2018

wora commented Aug 17, 2018 via email

venilnoronha commented Aug 17, 2018

rshriram Aug 17, 2018

venilnoronha Aug 17, 2018 •

edited

rshriram Aug 17, 2018

rshriram Aug 17, 2018

rshriram Aug 17, 2018

venilnoronha commented Aug 17, 2018

rshriram commented Aug 17, 2018

Deprecate int percent in favor of double percentage #609

Deprecate int percent in favor of double percentage #609

Conversation

venilnoronha commented Aug 14, 2018 • edited

istio-testing commented Aug 14, 2018

venilnoronha commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshriram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Aug 15, 2018

rshriram commented Aug 15, 2018

wora commented Aug 15, 2018 via email

venilnoronha commented Aug 16, 2018

wora commented Aug 17, 2018 via email

venilnoronha commented Aug 17, 2018

Choose a reason for hiding this comment

venilnoronha Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venilnoronha commented Aug 17, 2018

rshriram commented Aug 17, 2018

venilnoronha commented Aug 14, 2018 •

edited

venilnoronha Aug 17, 2018 •

edited