request_id_extension: Add noop impl by default#10687
request_id_extension: Add noop impl by default#10687mattklein123 merged 3 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this looks right, thanks. Can you add some tests? Both maybe in the TCP proxy case explicitly and also in stream_info_impl_test to make sure the NOP impl behaves correctly? Thank you!
/wait
| namespace { | ||
|
|
||
| // NoopRequestIDExtension is the implementation used outside of HTTP context. | ||
| class NoopRequestIDExtension : public RequestIDExtension { |
There was a problem hiding this comment.
nit: it would be nice to get coverage of the other methods here in the unit test you added.
| start_time_monotonic_(time_source.monotonicTime()), | ||
| filter_state_(std::make_shared<FilterStateImpl>(FilterState::LifeSpan::FilterChain)) {} | ||
| filter_state_(std::make_shared<FilterStateImpl>(FilterState::LifeSpan::FilterChain)), | ||
| request_id_extension_(Http::RequestIDExtensionFactory::noopInstance()) {} |
There was a problem hiding this comment.
looking at this, I think these constructors can mostly be collapsed with each one invoking the previous with fewer arguments. This would be a nice follow up.
There was a problem hiding this comment.
Sounds good, will send a follow up pr. Does it need a TODO for the time being?
There was a problem hiding this comment.
Created #10691, will rebase once this one is merged.
2 participants
Description: By default use noop
RequestIDExtensionimplementation inStreamInfo. This allows avoiding nullptr checks outside of HTTP context.Risk Level: low
Testing: added unit test, updated integration test (crashes without
StreamInfoImplchanges)Docs Changes: n/a
Release Notes: n/a
Fixes #10686