add a helper class for runtime-derived uint32 #16398

WeavingGao · 2021-05-08T15:48:17Z

Signed-off-by: gaoweiwen [email protected]

Envoy already has several runtime protobuf messages, such as RuntimeUInt32, RuntimePercent and RuntimeDouble. Each protobuf message has a corresponding C++ helper class to easily and safely obtain the runtime value, except for RuntimeUInt32. So this PR tries to add the missing helper class.

TODO(WeavingGao): use for #16392

Commit Message: helper class for RuntimeUInt32
Additional Description:
Risk Level: Low
Testing: Unit Test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: gaoweiwen <[email protected]>

WeavingGao · 2021-05-10T11:55:54Z

ready to be reviewed.

antoniovicente

cc @tonya11en

Hey Tony, could you do a review pass on this code since you were involved in the implementation of the wrapper for doubles?

antoniovicente · 2021-05-11T08:23:59Z

test/common/runtime/runtime_protos_test.cc

+  EXPECT_EQ(1024, test_uint32.value());
+
+  EXPECT_CALL(runtime_.snapshot_, getInteger("foo.bar", 99)).WillOnce(Return(1ull << 33));
+  EXPECT_EQ(0, test_uint32.value());


In cases where getInteger returns a value larger than uint32 max, should this mask the returned value down to 32 bits or return the default value? Consistency between the behavior of getDouble and getInteger for values larger than uint64 max suggest that we should return the default in this case.

https://github.com/envoyproxy/envoy/blob/main/source/common/runtime/runtime_impl.h#L104

The default value is indeed more reasonable, I'll submit a commit to fix it.

Fixed in the latest commit. @antoniovicente

Signed-off-by: gaoweiwen <[email protected]>

tonya11en

Just a minor comment, LG otherwise. Thanks!

tonya11en · 2021-05-11T16:45:02Z

source/common/runtime/runtime_protos.h

+
+  uint32_t value() const {
+    uint64_t raw_value = runtime_.snapshot().getInteger(runtime_key_, default_value_);
+    return raw_value > std::numeric_limits<uint32_t>::max() ? default_value_


Can you please add an error or warning log (every ~1000 times or something along those lines) for when there is an invalid value parsed that causes a revert to the default value?

This may introduce additional complexity. If we introduce additional variables to record the number of invalid parsing, this class will become thread-unsafe, making it either complicated or poor in performance.
I'm not sure if there are other solutions, I look forward to your suggestions.

I mean just put ENVOY_LOG_EVERY_POW_2() or ENVOY_LOG_EVERY_N() in a conditional instead of using that ternary operator. You don't need to get too clever here.

I would be genuinely surprised if that caused an appreciable decline in performance.

I misunderstood what you meant before (embarrassing >_<). Now I understand your suggestion and submit a new commit.

Signed-off-by: gaoweiwen <[email protected]>

tonya11en · 2021-05-12T06:50:55Z

source/common/runtime/runtime_protos.h

+    uint64_t raw_value = runtime_.snapshot().getInteger(runtime_key_, default_value_);
+    if (raw_value > std::numeric_limits<uint32_t>::max()) {
+      ENVOY_LOG_EVERY_POW_2(warn,
+                            "value:{} of {} is larger than uint32 max, return default instead",


Sorry one more-- can you mention in the message that it's a parsed runtime value and swap "return" for "returning".

It'll be good to go after that.

Signed-off-by: gaoweiwen <[email protected]>

tonya11en

Thanks!

WeavingGao · 2021-05-12T11:19:33Z

Could you please take a look since you had implemented a similar wrapper class and also have write access to merge. @alyssawilk

alyssawilk

I'm happy to take a look, but I'm unclear on why we're landing this upstream if it's not being used. Maybe if you filled out the Pr description fields it would explain? I think largely if its not used in Envoy it's going to end up being cleaned up at some point.

WeavingGao · 2021-05-13T02:09:27Z

I'm happy to take a look, but I'm unclear on why we're landing this upstream if it's not being used. Maybe if you filled out the Pr description fields it would explain? I think largely if its not used in Envoy it's going to end up being cleaned up at some point.

When I tried to implement another feature (see #16392 ), I found that I need a RuntimeUInt32 helper class just like RuntimeDouble. I was going to implement the helper class in that PR, but I thought it was very general, so I submitted it in a separate PR. The helper class is not used currently, but the RuntimeUInt32 protobuf message does exists and any other place that uses this protobuf message can benefit from the helper class.

tonya11en · 2021-05-15T20:41:19Z

@WeavingGao can you please update the description to include those details. This way other's don't have to fish through the comments to gather context.

WeavingGao · 2021-05-16T06:42:55Z

@WeavingGao can you please update the description to include those details. This way other's don't have to fish through the comments to gather context.

I added some details in the PR description. @alyssawilk @tonya11en

alyssawilk

Gotcha, thanks for the context.
I'd mildly prefer this land with the actual use in #16392, but if you'd prefer to land it separately how about a TODO(WeavingGao): use for #16392 just to make it clear it's not used yet, but what the plan is?

alyssawilk · 2021-05-17T14:05:14Z

source/common/runtime/runtime_protos.h

+      : runtime_key_(uint32_proto.runtime_key()), default_value_(uint32_proto.default_value()),
+        runtime_(runtime) {}
+
+  const std::string& runtimeKey() const { return runtime_key_; }


Mind adding a unit test for this?

Signed-off-by: gaoweiwen <[email protected]>

alyssawilk

lgtm modulo one nit - could you put the TODO in the code, rather than the PR description? Sorry that wasn't clear!

Signed-off-by: gaoweiwen <[email protected]>

Adding runtime helper protobuf messages for RuntimeUInt32. TODO(WeavingGao): use for Risk Level: Low Testing: Unit Test Docs Changes: N/A Release Notes: N/A part of envoyproxy#16392 Signed-off-by: gaoweiwen <[email protected]>

add a helper class for runtime-derived uint32 (envoyproxy#16397)

0a34c9b

Signed-off-by: gaoweiwen <[email protected]>

WeavingGao changed the title ~~add a helper class for runtime-derived uint32 (#16397)~~ add a helper class for runtime-derived uint32 May 8, 2021

change protected to private

f553f65

Signed-off-by: gaoweiwen <[email protected]>

antoniovicente self-assigned this May 10, 2021

antoniovicente mentioned this pull request May 10, 2021

Runtime protos: add UInt32 #16397

Closed

antoniovicente assigned tonya11en May 11, 2021

antoniovicente reviewed May 11, 2021

View reviewed changes

WeavingGao added 2 commits May 11, 2021 17:10

return default if the runtime value is larger then uint32 max

eed421c

Signed-off-by: gaoweiwen <[email protected]>

fix format

682f4c1

Signed-off-by: gaoweiwen <[email protected]>

tonya11en suggested changes May 11, 2021

View reviewed changes

add warnings when the value is overflow

a1cffc4

Signed-off-by: gaoweiwen <[email protected]>

tonya11en suggested changes May 12, 2021

View reviewed changes

modify the warning message

0d86df8

Signed-off-by: gaoweiwen <[email protected]>

tonya11en previously approved these changes May 12, 2021

View reviewed changes

alyssawilk reviewed May 12, 2021

View reviewed changes

alyssawilk self-assigned this May 12, 2021

alyssawilk reviewed May 17, 2021

View reviewed changes

WeavingGao dismissed tonya11en’s stale review via d5bc8ee May 19, 2021 02:03

additional unit test

9f39b77

Signed-off-by: gaoweiwen <[email protected]>

WeavingGao force-pushed the runtime_proto branch from d5bc8ee to 9f39b77 Compare May 19, 2021 02:40

Merge remote-tracking branch 'upstream/main' into runtime_proto

9fd4ead

Signed-off-by: gaoweiwen <[email protected]>

alyssawilk reviewed May 19, 2021

View reviewed changes

WeavingGao added 2 commits May 20, 2021 10:21

add a TODO

0610dcf

Signed-off-by: gaoweiwen <[email protected]>

Merge remote-tracking branch 'upstream/main' into runtime_proto

4e22777

Signed-off-by: gaoweiwen <[email protected]>

alyssawilk approved these changes May 20, 2021

View reviewed changes

alyssawilk merged commit 2174fd0 into envoyproxy:main May 20, 2021

WeavingGao deleted the runtime_proto branch October 18, 2021 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a helper class for runtime-derived uint32 #16398

add a helper class for runtime-derived uint32 #16398

WeavingGao commented May 8, 2021 •

edited

Loading

WeavingGao commented May 10, 2021

antoniovicente left a comment

antoniovicente May 11, 2021

WeavingGao May 11, 2021

WeavingGao May 11, 2021

tonya11en left a comment

tonya11en May 11, 2021

WeavingGao May 12, 2021

tonya11en May 12, 2021

WeavingGao May 12, 2021

tonya11en May 12, 2021

WeavingGao May 12, 2021

tonya11en left a comment

WeavingGao commented May 12, 2021

alyssawilk left a comment

WeavingGao commented May 13, 2021

tonya11en commented May 15, 2021

WeavingGao commented May 16, 2021 •

edited

Loading

alyssawilk left a comment

alyssawilk May 17, 2021

WeavingGao May 19, 2021

alyssawilk left a comment

add a helper class for runtime-derived uint32 #16398

add a helper class for runtime-derived uint32 #16398

Conversation

WeavingGao commented May 8, 2021 • edited Loading

WeavingGao commented May 10, 2021

antoniovicente left a comment

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

tonya11en left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonya11en left a comment

Choose a reason for hiding this comment

WeavingGao commented May 12, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

WeavingGao commented May 13, 2021

tonya11en commented May 15, 2021

WeavingGao commented May 16, 2021 • edited Loading

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

WeavingGao commented May 8, 2021 •

edited

Loading

WeavingGao commented May 16, 2021 •

edited

Loading