runtime: throw exception in debug mode that condition is always true when fractionalPercent numerator > denominator by belyalov · Pull Request #12068 · envoyproxy/envoy

belyalov · 2020-07-14T03:02:58Z

Commit Message: Add debug log message when numerator > denominator
Additional Description:
Risk Level: No Risk
Testing: No new tests added, was tested as part of troubleshooting of real issue
Docs Changes: Not Changed
Release Notes: Not added, maybe need to add?

Fixes #11874

…ent numerator > denominator Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov · 2020-07-14T19:47:33Z

source/common/runtime/runtime_impl.cc

+              "WARNING runtime key '{}': numerator ({}) > denominator ({}), condition always "
+              "evaluates to true",
+              key, percent.numerator(), denominator_value);
+    NOT_REACHED_GCOVR_EXCL_LINE;


Is it ok have this as gcov exception or you'd prefer me to add super simple unittest?

This change should have a unit test associated with it. Should be trivial.

NP, added :)

tonya11en · 2020-07-14T20:18:30Z

source/common/runtime/runtime_impl.cc

+  if (percent.numerator() > denominator_value) {
+    ENVOY_LOG(debug,
+              "WARNING runtime key '{}': numerator ({}) > denominator ({}), condition always "
+              "evaluates to true",
+              key, percent.numerator(), denominator_value);
+    NOT_REACHED_GCOVR_EXCL_LINE;
+  }


I think this check should only occur in debug builds and just throw an exception. Something like:

#ifndef NDEBUG if (percent.numerator() > denominator_value) { throw EnvoyException(.....); } #endif

Makes sense.

tonya11en

Thanks! I left 2 comments on the patch. The changes should be easy.

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov · 2020-07-14T23:57:19Z

Thanks! I left 2 comments on the patch. The changes should be easy.

Thanks for feedback!
Addresses all 2 comments.

tonya11en · 2020-07-15T00:47:23Z

source/common/runtime/runtime_impl.cc

+  uint64_t denominator_value =
+      ProtobufPercentHelper::fractionalPercentDenominatorToInt(percent.denominator());
+  if (percent.numerator() > denominator_value) {
+    throw EnvoyException(fmt::format("WARNING runtime key '{}': numerator ({}) > denominator ({}), "


I'd remove the WARNING part of this. You're halting the show with that exception, so it's not really a warning :)

Oh yeah, sure, makes sense (I've just copy pasted it)

tonya11en

Just a nit regarding the error message. I'll approve once that's fixed and then a maintainer can take a look for official approval.

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

tonya11en

LGTM. Thanks!

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

tonya11en · 2020-07-16T21:47:14Z

.bazelversion

@@ -1 +0,0 @@
-3.3.1


Should this be part of the diff?

oops, reverting it :)
Was testing it on several bazel versions

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

tonya11en

Looks like CI is failing. Can you look into that?

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov · 2020-07-22T17:11:37Z

@tonya11en ping? seems CI is now passing.. :)

tonya11en

One last adjustment and we should be good to go after that.

test/common/runtime/runtime_impl_test.cc

ggreenway · 2020-07-22T18:30:01Z

I don't think having different error handling in debug vs release is a good idea. Can it just log instead, and do it regardless of compilation mode?

belyalov · 2020-07-27T15:31:27Z

I don't think having different error handling in debug vs release is a good idea. Can it just log instead, and do it regardless of compilation mode?

This is what originally has been done.
@ggreenway @tonya11en let's finalize what's preferred way? :)

ggreenway · 2020-07-27T19:30:40Z

I don't think having different error handling in debug vs release is a good idea. Can it just log instead, and do it regardless of compilation mode?

This is what originally has been done.
@ggreenway @tonya11en let's finalize what's preferred way? :)

Sorry about that; I didn't look at all the history when I reviewed it. I talked to @tonya11en and we agreed it makes most sense to log at debug level, in both debug and release builds. Again, sorry for the back-and-forth on this.

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

…_for_wrong_runtime_value Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov · 2020-07-29T00:04:09Z

@ggreenway ok, thanks for clarification.
Switched back to debug log message.

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

test/common/runtime/runtime_impl_test.cc

ggreenway · 2020-07-29T15:37:09Z

/wait

stale · 2020-08-08T18:29:41Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

…_for_wrong_runtime_value Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

ggreenway

LGTM

ggreenway · 2020-08-13T16:42:48Z

/retest

repokitteh-read-only · 2020-08-13T16:42:52Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12068 (comment) was created by @ggreenway.

see: more, trace.

ggreenway · 2020-08-13T17:25:29Z

Windows failure looks like an unrelated test flake.

belyalov · 2020-08-13T17:37:47Z

Windows failure looks like an unrelated test flake.

:(
Do you want me to merge it with latest master or we can just re-start build?

ggreenway · 2020-08-13T17:43:09Z

Windows failure looks like an unrelated test flake.

:(
Do you want me to merge it with latest master or we can just re-start build?

No need; merging as-is since the failure is unrelated.

* master: (67 commits) logger: support log control in admin interface and command line option for Fancy Logger (envoyproxy#12369) test: fix http_timeout_integration_test flake (envoyproxy#12654) [fuzz]added an input check in writefilter fuzzer and added test cases (envoyproxy#12628) add 'explicit' restriction. (envoyproxy#12643) scoped_rds_integration_test migrate from api v2 to api v3. (envoyproxy#12633) fuzz: added fuzz test for listener filter tls_inspector (envoyproxy#12617) testing: fix multiple race conditions in simulated time tests (envoyproxy#12527) [tls] Move handshaking behavior into SslSocketInfo. (envoyproxy#12571) header: getting rid of exception-throwing behaviors in header files [the rest] (envoyproxy#12611) router: add new ratelimited retry backoff strategy (envoyproxy#12202) [redis_proxy] added a constraint for route.prefix().size() (envoyproxy#12637) network: add tcp listener backlog config (envoyproxy#12625) runtime: debug log that condition is always true when fractionalPercent numerator > denominator (envoyproxy#12068) WatchDog Extension hook (envoyproxy#12416) router: add dynamic metadata header formatter (envoyproxy#11858) statsd: revert visibility to public (envoyproxy#12621) Fix regression of /build_* in gitignore (envoyproxy#12630) Added a missing extension point to documentation. (envoyproxy#12620) Reverts proxy protocol test on windows (envoyproxy#12619) caching: Improved the tests and coverage of the CacheFilter tree (envoyproxy#12544) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

belyalov added 2 commits July 13, 2020 21:00

[runtime] debug log that condition is always true when fractionalPerc…

1b31cad

…ent numerator > denominator Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Fix format

472b32c

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

junr03 assigned tonya11en Jul 14, 2020

Exclude debug log from code coverage

2108e16

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov commented Jul 14, 2020

View reviewed changes

tonya11en reviewed Jul 14, 2020

View reviewed changes

tonya11en requested changes Jul 14, 2020

View reviewed changes

Address review feedback: throw expection / add unittest

c0db266

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov marked this pull request as ready for review July 14, 2020 23:57

tonya11en reviewed Jul 15, 2020

View reviewed changes

Remove "warning" from exception message

1d49bda

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov changed the title ~~[runtime] debug log that condition is always true when fractionalPercent numerator > denominator~~ runtime: throw exception in debug mode that condition is always true when fractionalPercent numerator > denominator Jul 15, 2020

tonya11en previously approved these changes Jul 15, 2020

View reviewed changes

belyalov added 3 commits July 15, 2020 09:18

Merge with upstream master

ddca74d

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Merge upstream/master

0442492

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Assert for exception only in debug mode

e828795

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov dismissed tonya11en’s stale review via e828795 July 16, 2020 21:07

tonya11en reviewed Jul 16, 2020

View reviewed changes

brint bazelversion back

628dbd3

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

tonya11en approved these changes Jul 16, 2020

View reviewed changes

tonya11en requested changes Jul 16, 2020

View reviewed changes

belyalov added 3 commits July 16, 2020 20:04

call featureEnabled() on release test config.

6a1408d

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Merge with upstream/master

54290b8

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Merge with master

7824bb8

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov requested a review from tonya11en July 22, 2020 17:10

tonya11en requested changes Jul 22, 2020

View reviewed changes

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

belyalov added 2 commits July 28, 2020 17:58

Switch back to debug log instead of expection

bf4b498

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Merge remote-tracking branch 'upstream/master' into add_debug_message…

a9453ec

…_for_wrong_runtime_value Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Fix format

140db72

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

belyalov requested a review from tonya11en July 29, 2020 03:37

ggreenway requested changes Jul 29, 2020

View reviewed changes

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

ggreenway self-assigned this Jul 29, 2020

repokitteh-read-only bot added the waiting label Jul 29, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 8, 2020

belyalov added 2 commits August 12, 2020 12:45

Merge remote-tracking branch 'upstream/master' into add_debug_message…

382770b

…_for_wrong_runtime_value Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

Assert for log message

29e13b2

Signed-off-by: Konstantin Belyalov <arsenicus@mail.ru>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 12, 2020

repokitteh-read-only bot removed the waiting label Aug 12, 2020

belyalov requested a review from ggreenway August 12, 2020 22:56

ggreenway approved these changes Aug 12, 2020

View reviewed changes

ggreenway merged commit 466245b into envoyproxy:master Aug 13, 2020

		@@ -1 +0,0 @@
		3.3.1

Conversation

belyalov commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

belyalov commented Jul 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

belyalov commented Jul 22, 2020

Uh oh!

tonya11en left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ggreenway commented Jul 22, 2020

Uh oh!

belyalov commented Jul 27, 2020

Uh oh!

ggreenway commented Jul 27, 2020

Uh oh!

belyalov commented Jul 29, 2020

Uh oh!

Uh oh!

ggreenway commented Jul 29, 2020

Uh oh!

stale bot commented Aug 8, 2020

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Aug 13, 2020

Uh oh!

repokitteh-read-only bot commented Aug 13, 2020

Uh oh!

ggreenway commented Aug 13, 2020

Uh oh!

belyalov commented Aug 13, 2020

Uh oh!

ggreenway commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants