runtime: Add getDouble functionality to snapshot by tonya11en · Pull Request #8265 · envoyproxy/envoy

tonya11en · 2019-09-17T08:17:15Z

This patch adds the getDouble function to the runtime snapshot. This allows for parsing of double values in addition to the existing integer and fractions.

Risk Level: Low
Testing: New unit tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Tony Allen <tony@allen.gg>

zuercher

Thanks, this seems useful for non-integer, non-percentage values.

I think we need to think about the interaction between integer and double values, however. It seems to me that the current behavior ignores values like "1.5" (if I understand absl::SimpleAtoi correctly it will treat that as a parsing error). I don't think this PR changes that, as long as code calls Snapshot::getInteger. However, if some code is using Snapshot::getDouble and the input is changed from 1.5 to just 1, getDouble will suddenly start returning 0.0 because "1" is parsed as a uint and SnapshotImpl::parseEntryDoubleValue won't be invoked. Perhaps we should store numeric values as both uint64_t and doubles and allow getDouble and getInteger to be used interchangeably?

tonya11en · 2019-09-18T18:59:44Z

Editing this comment since I realized the logic was miguided. I'll see about changing this to store both uint and double values.

Great catch, I hadn't considered that scenario.

Signed-off-by: Tony Allen <tallen@lyft.com>

zuercher

Thanks! There's still a potential weirdness where, say "5.0" isn't handled if the code uses getInteger. But I don't know the right way to reconcile numbers with fractional parts and getInteger. For example, I'm not sure it would be good for getInteger to return 5 for an input of "5.9" but that might be better than having it flip to default values because these they didn't realize a config point only accepted integers vs. floats.

Curious if @mattklein123 has an opinion.

mattklein123 · 2019-09-19T03:22:45Z

For example, I'm not sure it would be good for getInteger to return 5 for an input of "5.9" but that might be better than having it flip to default values because these they didn't realize a config point only accepted integers vs. floats.

FWIW I think we should probably just floor it and always store as both a double and an integer. I think it will be less surprising. If we decide not to do this can we clearly document that case?

Let's do what Matt suggested.

Signed-off-by: Tony Allen <tony@allen.gg>

zuercher · 2019-09-19T22:50:13Z

source/common/runtime/runtime_impl.h

+      // Valid uint values will always be parseable as doubles, so we assign the value to both the
+      // uint and double fields. In cases where the value is something like "3.1", we will floor the
+      // number by casting it to a uint and assigning the uint value.
+      entry.uint_value_ = entry.double_value_;


Let's add a test for negative double values and positive double values >= 2^64. I think we'll want to handle those specially. Probably also need a warning on getInteger that values greater than approximately 2^53 will not be accurately converted to integers.

source/common/runtime/runtime_impl.h

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en · 2019-09-20T21:07:41Z

/wait

test/common/runtime/runtime_impl_test.cc

Signed-off-by: Tony Allen <tallen@lyft.com>

zuercher · 2019-09-23T16:59:30Z

Cool. ~~Let's add a comment on getInteger for values > 2^53 and then I think this is good to go.~~

mattklein123

Thanks, looks great. Can you add a release note about the new behavior for doubles/integers in runtime. Also, would it be worth to actually document this behavior somewhere in the runtime docs?

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>

mattklein123

LGTM with small nit. Thank you!

/wait

mattklein123 · 2019-09-23T20:02:02Z

docs/root/intro/version_history.rst

 * upstream: added new :ref:`failure-percentage based outlier detection<arch_overview_outlier_detection_failure_percentage>` mode.
 * upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1.
 * upstream: added :ref:`fail_traffic_on_panic <envoy_api_field_Cluster.CommonLbConfig.ZoneAwareLbConfig.fail_traffic_on_panic>` to allow failing all requests to a cluster during panic state.
+* runtime: allow for the ability to parse integers as double values and vice-versa.


nit: alpha order.

Signed-off-by: Tony Allen <tallen@lyft.com>

mattklein123 · 2019-09-23T20:19:44Z

Sorry needs a master merge.

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>

mattklein123 · 2019-09-23T21:35:08Z

/retest

repokitteh-read-only · 2019-09-23T21:35:12Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #8265 (comment) was created by @mattklein123.

see: more, trace.

mattklein123 · 2019-09-23T21:48:57Z

docs/root/intro/version_history.rst

Sorry needs another merge fix.

/wait

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en · 2019-09-23T22:59:58Z

/retest

repokitteh-read-only · 2019-09-23T23:00:00Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #8265 (comment) was created by @tonya11en.

see: more, trace.

mattklein123

Thanks!

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en added 2 commits September 17, 2019 00:36

runtime: Add getDouble functionality to snapshot

343b013

Signed-off-by: Tony Allen <tony@allen.gg>

Fix test

a3e0d15

Signed-off-by: Tony Allen <tony@allen.gg>

zuercher reviewed Sep 17, 2019

View reviewed changes

zuercher self-assigned this Sep 17, 2019

Store both double and uint

a4ae5d2

Signed-off-by: Tony Allen <tallen@lyft.com>

zuercher previously approved these changes Sep 18, 2019

View reviewed changes

Always parse uints/doubles together

372ff42

Signed-off-by: Tony Allen <tony@allen.gg>

zuercher reviewed Sep 19, 2019

View reviewed changes

Address negative doubles

a31daf3

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only bot added the waiting label Sep 20, 2019

zuercher reviewed Sep 20, 2019

View reviewed changes

test/common/runtime/runtime_impl_test.cc Outdated Show resolved Hide resolved

Tony Allen added 2 commits September 20, 2019 15:09

add large int test

df5dcb7

Signed-off-by: Tony Allen <tallen@lyft.com>

format

4022659

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only bot removed the waiting label Sep 20, 2019

zuercher previously approved these changes Sep 23, 2019

View reviewed changes

zuercher assigned mattklein123 Sep 23, 2019

mattklein123 requested changes Sep 23, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 23, 2019

docs

c5cfcc6

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en dismissed zuercher’s stale review via c5cfcc6 September 23, 2019 19:19

repokitteh-read-only bot removed the waiting label Sep 23, 2019

zuercher previously approved these changes Sep 23, 2019

View reviewed changes

mattklein123 requested changes Sep 23, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 23, 2019

alpha order

a6000b7

Signed-off-by: Tony Allen <tallen@lyft.com>

tonya11en dismissed zuercher’s stale review via a6000b7 September 23, 2019 20:05

repokitteh-read-only bot removed the waiting label Sep 23, 2019

repokitteh-read-only bot added the waiting label Sep 23, 2019

Merge remote-tracking branch 'upstream/master' into get_double

5f80c13

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only bot removed the waiting label Sep 23, 2019

mattklein123 reviewed Sep 23, 2019

View reviewed changes

docs/root/intro/version_history.rst

Copy link
Copy Markdown

Member

mattklein123 Sep 23, 2019

Choose a reason for hiding this comment

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

Sorry needs another merge fix.

/wait

repokitteh-read-only bot added the waiting label Sep 23, 2019

ordering of release note

590b542

Signed-off-by: Tony Allen <tallen@lyft.com>

repokitteh-read-only bot removed the waiting label Sep 23, 2019

mattklein123 approved these changes Sep 24, 2019

View reviewed changes

mattklein123 merged commit 7bd7c46 into envoyproxy:master Sep 24, 2019

tonya11en deleted the get_double branch September 24, 2019 18:15

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

runtime: Add getDouble functionality to snapshot (envoyproxy#8265)

0f4ffe3

Signed-off-by: Tony Allen <tallen@lyft.com>

Conversation

tonya11en commented Sep 17, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 19, 2019

Uh oh!

zuercher Sep 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tonya11en commented Sep 20, 2019

Uh oh!

Uh oh!

zuercher commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Sep 23, 2019

Uh oh!

mattklein123 commented Sep 23, 2019

Uh oh!

repokitteh-read-only bot commented Sep 23, 2019

Uh oh!

mattklein123 Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

tonya11en commented Sep 23, 2019

Uh oh!

repokitteh-read-only bot commented Sep 23, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonya11en commented Sep 18, 2019 •

edited

Loading

zuercher commented Sep 23, 2019 •

edited

Loading