http: introduce addEncodedMetadata()#7756
Conversation
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
|
cc @birenroy |
Signed-off-by: Yang Song <yasong@google.com>
Signed-off-by: Yang Song <yasong@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looks mostly good (and thankfully parallel to the decode path!)
Just a couple of thoughts from me:
| std::unique_ptr<MetadataMapVector> saved_request_metadata_; | ||
| std::unique_ptr<MetadataMapVector> saved_response_metadata_; | ||
| std::unique_ptr<MetadataMapVector> saved_request_metadata_ = nullptr; | ||
| std::unique_ptr<MetadataMapVector> saved_response_metadata_ = nullptr; |
There was a problem hiding this comment.
Is this necessary? If so let's use the {nullptr} syntax you use for request_medata_map_vector_
| void StreamEncoderImpl::encodeTrailers(const HeaderMap&) { endEncode(); } | ||
|
|
||
| void StreamEncoderImpl::encodeMetadata(const MetadataMapVector&) { | ||
| ENVOY_LOG_MISC(error, "HTTP1 doesn't support metadata"); |
There was a problem hiding this comment.
So if an H2 upstream sends medatadata to an H1 data source, we'll log an error message for every frame? I think we want to handle that in a way that's less painful. I think ideally we'd error log once (or better yet with pow-2 backoff) and increment a stat, but I don't think Envoy has backoff logging yet so maybe just a stat for now?
There was a problem hiding this comment.
Cool. If you want to change this to a no-op I'd be fine with it landing as-is, though it should probably get a Matt Klein pass as well.
There was a problem hiding this comment.
Maybe I can sync to master after #7801 is merged?
There was a problem hiding this comment.
Yeah agreed this should be a NOP + stat, so fine to review the other change first and then come back to this one.
| {"headers", "headers"}, {"duplicate", "duplicate"}, {"remove", "remove"}}; | ||
| Http::MetadataMap metadata_map = {{"headers", "headers"}, {"duplicate", "duplicate"}}; | ||
| Http::MetadataMapPtr metadata_map_ptr = std::make_unique<Http::MetadataMap>(metadata_map); | ||
| decoder_callbacks_->encodeMetadata(std::move(metadata_map_ptr)); |
There was a problem hiding this comment.
Are there HCM unit tests we should update as well?
There was a problem hiding this comment.
I think adding new metadata was only covered in integration test.
Signed-off-by: Yang Song <yasong@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
LGTM modulo the error log. I'd rather have that fixed before merge but given I suspect most folks aren't using metadata I'm fine with a follow-up PR too
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM modulo the error log. I will review the other change.
/wait
| void StreamEncoderImpl::encodeTrailers(const HeaderMap&) { endEncode(); } | ||
|
|
||
| void StreamEncoderImpl::encodeMetadata(const MetadataMapVector&) { | ||
| ENVOY_LOG_MISC(error, "HTTP1 doesn't support metadata"); |
There was a problem hiding this comment.
Yeah agreed this should be a NOP + stat, so fine to review the other change first and then come back to this one.
Signed-off-by: Yang Song <yasong@google.com>
|
Merged h1 stats change. PTAL. Thanks! |
3 participants
Description: introduce addEncodedMetadata(). Newly added metadata by addEncodedMetadata() will only be passed to filters following the filter where the metadata are added.
Risk Level: low.
Testing: integration tests.
Docs Changes: Updated metadata docs.
Release Notes: Introduce addEncodeMetadata() function.