Add quic_expect_bug_impl.h, (spdy|http2)_logging_impl.h, (spdy|http2)_bug_tracker_impl.h QUICHE platform implementation by wu-bin · Pull Request #6339 · envoyproxy/envoy

wu-bin · 2019-03-21T04:08:22Z

Description:

Add quic_expect_bug_impl.h, (spdy|http2)_logging_impl.h, (spdy|http2)_bug_tracker_impl.h QUICHE platform implementation.

All of them depends on quic_logging_impl.h.

Risk Level: minimum, code not used yet.
Testing:

bazel test test/extensions/quic_listeners/quiche/platform:spdy_platform_test --test_output=all --define quiche=enabled
bazel test test/extensions/quic_listeners/quiche/platform:http2_platform_test --test_output=all --define quiche=enabled
bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled
bazel test @com_googlesource_quiche//:spdy_platform_test --test_output=all --define quiche=enabled
bazel test @com_googlesource_quiche//:http2_platform_test --test_output=all --define quiche=enabled
bazel test @com_googlesource_quiche//:quic_platform_test --test_output=all --define quiche=enabled

Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Bin Wu <[email protected]>

wu-bin · 2019-03-21T13:28:46Z

/assign @danzh2010

wu-bin · 2019-03-21T15:11:55Z

/retest

repokitteh-read-only · 2019-03-21T15:12:04Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6339 (comment) was created by @wu-bin.

see: more, trace.

danzh2010 · 2019-03-21T14:52:19Z

bazel/external/quiche.BUILD

    name = "http2_platform",
    hdrs = [
        "quiche/http2/platform/api/http2_arraysize.h",
+        "quiche/http2/platform/api/http2_bug_tracker.h",


Q please: do these API files need to be wrapped in envoy_select_quiche if their impl's are wrapping with it?

It's not necessary but I moved some to envoy_select_quiche.

IIUC, this (and some other) library is not a "real" library - it does not have a "srcs" section, so "bazel build" this library "succeeds" without doing anything. But

if a dependency(like http2_platform_test) uses this library, that dependency will fail to build.

the library will fail to build as soon as we add a .cc to the "srcs" section.
In my past PRs, I add envoy_select_quiche when 1) ci fails on the files I'm adding or 2) I think ci will fail on the files I'm adding. It is really a bandaid to allow us to continue working on platform impls before the root of the build problem is fixed.

danzh2010 · 2019-03-21T14:55:41Z

source/extensions/quic_listeners/quiche/platform/BUILD


+envoy_cc_library(
+    name = "quic_platform_logging_impl_lib",
+    srcs = ["quic_logging_impl.cc"],


does this need to be wrapped in envoy_select_quiche?

It's not necessary, libraries depend on this is using envoy_select_quiche.

danzh2010 · 2019-03-21T15:06:54Z

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.cc

+
+    if (!(is_dfatal && g_dfatal_exit_disabled)) {
+      abort();
+    }


Can you combine this if block with the #ifdef NDEBUG block above?

danzh2010 · 2019-03-21T15:14:55Z

test/extensions/quic_listeners/quiche/platform/spdy_platform_test.cc

@@ -26,6 +30,14 @@ TEST(SpdyPlatformTest, SpdyArraysize) {
  EXPECT_EQ(5, SPDY_ARRAYSIZE(array));
 }



Can we move these tests under namespace spdy? ditto to Http2PlatformTest

Move their namespaces SGTM, but it is unrelated to this PR, let's do it separately.

Signed-off-by: Bin Wu <[email protected]>

danzh2010

LGTM other than a request to add corresponding namespace to spdy|http2_platform_test.cc

…ng_impl Signed-off-by: Bin Wu <[email protected]>

wu-bin · 2019-03-22T19:14:54Z

/assign @mattklein123 @alyssawilk @htuch

alyssawilk

LGTM pending one question

alyssawilk · 2019-03-25T15:06:53Z

source/extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h

-#define QUIC_BUG_IF_IMPL(condition) QUIC_LOG_IF(DFATAL, condition)
-#define QUIC_PEER_BUG_IMPL QUIC_LOG(ERROR)
-#define QUIC_PEER_BUG_IF_IMPL(condition) QUIC_LOG_IF(ERROR, condition)
+#define QUIC_BUG_IMPL QUIC_LOG_IMPL(DFATAL)


preexisted, but doesn't QUIC_BUG work based on the exponential back-off, and could cause serious perf problems if we don't do that?
Should we get a TODO to get around to that work?

QUIC_BUG has the exponential back off in google3 but not Chromium. It, like other error logs, could cause serious perf problems if it happens too frequently.

TODO added to implement exponential back off in envoy.

yeah, per-packet logging server side scales less well than client side :-) thanks for the TODO

htuch

LGTM modulo comments.
/wait

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.cc

htuch · 2019-03-25T15:18:47Z

test/extensions/quic_listeners/quiche/platform/spdy_platform_test.cc

+  SPDY_DLOG(INFO) << "DLOG(INFO)";
+  SPDY_DLOG(ERROR) << "DLOG(ERROR)";
+
+  SPDY_DLOG_IF(ERROR, true) << "DLOG_IF(ERROR, true)";


Just as an FYI; when we do the Envoy-side QUIC implementation, we should avoid using any of these macros, ideally we restrict access to them using package visibility to just the QUICHE code and PAL.

Ack, totally agree.

Signed-off-by: Bin Wu <[email protected]>

wu-bin

Thanks Alyssa and Harvey. I added a TODO for the exponential back off.

wu-bin · 2019-03-25T15:38:20Z

source/extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h

-#define QUIC_BUG_IF_IMPL(condition) QUIC_LOG_IF(DFATAL, condition)
-#define QUIC_PEER_BUG_IMPL QUIC_LOG(ERROR)
-#define QUIC_PEER_BUG_IF_IMPL(condition) QUIC_LOG_IF(ERROR, condition)
+#define QUIC_BUG_IMPL QUIC_LOG_IMPL(DFATAL)


QUIC_BUG has the exponential back off in google3 but not Chromium. It, like other error logs, could cause serious perf problems if it happens too frequently.

TODO added to implement exponential back off in envoy.

source/extensions/quic_listeners/quiche/platform/quic_logging_impl.cc

wu-bin · 2019-03-25T15:48:44Z

test/extensions/quic_listeners/quiche/platform/spdy_platform_test.cc

+  SPDY_DLOG(INFO) << "DLOG(INFO)";
+  SPDY_DLOG(ERROR) << "DLOG(ERROR)";
+
+  SPDY_DLOG_IF(ERROR, true) << "DLOG_IF(ERROR, true)";


Ack, totally agree.

alyssawilk

LGTM - I'll let Harvey sign off on his requests and merge.

alyssawilk · 2019-03-25T17:09:07Z

source/extensions/quic_listeners/quiche/platform/quic_bug_tracker_impl.h

-#define QUIC_BUG_IF_IMPL(condition) QUIC_LOG_IF(DFATAL, condition)
-#define QUIC_PEER_BUG_IMPL QUIC_LOG(ERROR)
-#define QUIC_PEER_BUG_IF_IMPL(condition) QUIC_LOG_IF(ERROR, condition)
+#define QUIC_BUG_IMPL QUIC_LOG_IMPL(DFATAL)


yeah, per-packet logging server side scales less well than client side :-) thanks for the TODO

htuch

Thanks!

wu-bin added 3 commits March 20, 2019 22:02

Add quic_expect_bug_impl.h.

7d3314a

Signed-off-by: Bin Wu <[email protected]>

Add spdy_logging_impl.h and spdy_bug_tracker_impl.h.

113f47c

Signed-off-by: Bin Wu <[email protected]>

Add http2_logging_impl.h and http2_bug_tracker_impl.h.

9bf4b26

Signed-off-by: Bin Wu <[email protected]>

wu-bin requested review from alyssawilk and mattklein123 as code owners March 21, 2019 04:08

s/VLOGs/VLOG/ to make spelling checker happy.

c338d2f

Signed-off-by: Bin Wu <[email protected]>

repokitteh-read-only bot assigned danzh2010 Mar 21, 2019

danzh2010 reviewed Mar 21, 2019

View reviewed changes

Update per review comment.

105d171

Signed-off-by: Bin Wu <[email protected]>

danzh2010 reviewed Mar 21, 2019

View reviewed changes

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

751d882

…ng_impl Signed-off-by: Bin Wu <[email protected]>

danzh2010 previously approved these changes Mar 22, 2019

View reviewed changes

repokitteh-read-only bot assigned alyssawilk, htuch and mattklein123 Mar 22, 2019

alyssawilk reviewed Mar 25, 2019

View reviewed changes

htuch suggested changes Mar 25, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 25, 2019

Add a TODO for QUIC_BUG to implement exponential back off.

f9f7cde

Signed-off-by: Bin Wu <[email protected]>

wu-bin dismissed danzh2010’s stale review via f9f7cde March 25, 2019 15:32

repokitteh-read-only bot removed the waiting label Mar 25, 2019

wu-bin commented Mar 25, 2019

View reviewed changes

alyssawilk approved these changes Mar 25, 2019

View reviewed changes

htuch added the waiting:any label Mar 26, 2019

repokitteh-read-only bot removed the waiting:any label Mar 26, 2019

htuch approved these changes Mar 26, 2019

View reviewed changes

htuch merged commit 8fb00ba into envoyproxy:master Mar 26, 2019

		@@ -26,6 +30,14 @@ TEST(SpdyPlatformTest, SpdyArraysize) {
		EXPECT_EQ(5, SPDY_ARRAYSIZE(array));
		}

Conversation

wu-bin commented Mar 21, 2019

Uh oh!

wu-bin commented Mar 21, 2019

Uh oh!

wu-bin commented Mar 21, 2019

Uh oh!

repokitteh-read-only bot commented Mar 21, 2019

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!

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!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

wu-bin commented Mar 22, 2019

Uh oh!

alyssawilk 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!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants