HTTP2: Add DumpState support for HTTP2#14923
Conversation
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/assign @antoniovicente |
antoniovicente
left a comment
There was a problem hiding this comment.
I think it looks good otherwise.
| case '\t': | ||
| os << "\\t"; | ||
| break; | ||
| case '\v': |
There was a problem hiding this comment.
Please also escape '\0'
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
KBaichoo
left a comment
There was a problem hiding this comment.
Thanks for the review @antoniovicente !
| case '\t': | ||
| os << "\\t"; | ||
| break; | ||
| case '\v': |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
|
||
| void ConnectionImpl::StreamImpl::dumpState(std::ostream& os, int indent_level) const { | ||
| const char* spaces = spacesForLevel(indent_level); | ||
| os << "ConnectionImpl::StreamImpl " << this << DUMP_MEMBER(stream_id_) |
There was a problem hiding this comment.
nit: I think you're missing spaces in this line
os << spaces << "ConnectionImpl::StreamImpl "
| OutputBufferStream ostream{buffer.data(), buffer.size()}; | ||
|
|
||
| Stats::TestUtil::MemoryTest memory_test; | ||
| server_->dumpState(ostream, 0); |
There was a problem hiding this comment.
nit: passing in a non-zero number of spaces here may make testing easier/more comprehensive. suggestion: set to 2 instead of 0.
There was a problem hiding this comment.
Good idea. It now generates 2 spaces for all of these tests. Note: the spacesForLevel generates min(12, 2*i) spaces for i in [0, inf).
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
@antoniovicente since you're the maintainer on-call can you assign someone from @envoyproxy/senior-maintainers who is a non-googler? Thanks! |
|
/retest |
|
Retrying Azure Pipelines: |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
PTAL @snowp . @antoniovicente suggested you'd be a good senior-maintainer to look at this. Thanks! /assign @snowp |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
|
/retest Windows seems like a flake |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
It is a C++17 feature: https://en.cppreference.com/w/cpp/algorithm/for_each_n Edit: What's the procedure then? I'm fine with just replacing it. |
|
This appears to be the first introduction of a C++17 feature and it breaks Clang 10. The current build requirements are outlined at https://github.com/envoyproxy/envoy/blob/main/docs/root/start/building.rst. Will likely need the @envoyproxy/senior-maintainers to chime in |
|
|
|
Yeah I think the simplest thing to do here is to just avoid the use of std::for_each_n since it seems to be poorly supported. @lizan do you have any thoughts here? Is this just due to std lib implementations poorly supporting C++17 features? |
|
IIRC there are other places where the Envoy codebase depends on C++ 17. Is there some compiler flag that can be used to disallow for_each_n in c++ 17 if we're not ready to support it yet? |
|
We use structured bindings in envoy/source/common/upstream/subset_lb.cc Line 137 in de8c505 |
|
Language/library support in compilers tend to be on a per feature basis, so I can totally believe that some of the older standard library versions don't support this feature while still supporting structured bindings. I think if we want to maintain support for GCC-7 we would probably want to avoid using this, similar to how we avoid using std::filesystem due to spotty support on some platforms (e.g. mobile).
|
|
See: #15093 for a lint of this as you suggested. |
|
It must be due to older libstdc++, libstdc++-9 has support for that. @moderation can you try with gcc-toolset-9? |
|
@lizan I have gcc-toolset-10 in my RHEL 8 environment and will try that but I recall moving to the clang 10 for some reason. Will try and report back |
|
@lizan @KBaichoo With no config flag and therefore gcc 10 + libstdc++ and a very limited set of extensions enabled the first failure was in the quic code (I forgot to comment that out). I commented that extension out and the next problem I hit was So with the addition of the single C++17 feature it doesn't look like there is a way to build on RHEL 8.3 with either GCC 10 or Clang 10 |
|
You can use libstdc++ >= 9 with clang, no need use gcc, but that gcc 10 issue is a one issue that we should address. What's your output of |
|
This broke for me too in ubuntu 18 fwiw. Are we planning to revert or document/fix fwd? |
|
Ah nvm #15093 is the fix, i'll wait for that (or add it to our repo in the meanwhile). |
|
Looking forward to #15093 too as we are still blocked with Clang 10 on RHEL8.3 (and GCC is broken too but we'd rather stick with Clang) |
|
PR #15093 seems stuck in review. It would be good to unblock folks using ubuntu 18 and RHEL8.3. What is the recommended path forward? |
This reverts commit 3f740bc. Because it breaks builds on RHEL 8 and Ubuntu 18. See: envoyproxy#15093 Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
|
I'd kindly suggest we revert (PR here #15153) for now, so we can unblock people who need to track master closely. |
|
PR #15093 was merged, ubuntu 18 and RHEL8.3 builds should work again. |
Signed-off-by: Kevin Baichoo kbaichoo@google.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message: HTTP2: Add DumpState support for HTTP2
Additional Description: While dispatching, the HTTP2 Connection object registers itself as a scopetrackedobject. If we crash during parsing pipeline, we'll dump information such as the current streams, current buffer, etc.
Risk Level: medium
Testing:unit tests
Docs Changes: N/A
Release Notes: Included
Platform Specific Features: N/A
Related Issue #14249
This is a follow up to PR #14812.
Originally got some feedback on this in KBaichoo#2.