-
Notifications
You must be signed in to change notification settings - Fork 5.5k
decompressor filter: add bytes reporting to trailers #12477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
7612d8f
85d63e8
2101e24
5f444d0
89e3f0a
5b0430c
d799bcc
4daaa11
1d461d0
60fe4c4
540a5ae
c91d96d
2bf797f
2c04883
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,9 @@ DecompressorFilterConfig::DecompressorFilterConfig( | |
| : stats_prefix_(fmt::format("{}decompressor.{}.{}", stats_prefix, | ||
| proto_config.decompressor_library().name(), | ||
| decompressor_factory->statsPrefix())), | ||
| trailers_prefix_(fmt::format("{}-decompressor-{}", | ||
| ThreadSafeSingleton<Http::PrefixValue>::get().prefix(), | ||
| proto_config.decompressor_library().name())), | ||
| decompressor_stats_prefix_(stats_prefix_ + "decompressor_library"), | ||
| decompressor_factory_(std::move(decompressor_factory)), | ||
| request_direction_config_(proto_config.request_direction_config(), stats_prefix_, scope, | ||
|
|
@@ -56,7 +59,9 @@ DecompressorFilterConfig::ResponseDirectionConfig::ResponseDirectionConfig( | |
| : DirectionConfig(proto_config.common_config(), stats_prefix + "response.", scope, runtime) {} | ||
|
|
||
| DecompressorFilter::DecompressorFilter(DecompressorFilterConfigSharedPtr config) | ||
| : config_(std::move(config)) {} | ||
| : config_(std::move(config)), | ||
| request_byte_tracker_(config_->trailersStrings()[0], config_->trailersStrings()[1]), | ||
| response_byte_tracker_(config_->trailersStrings()[0], config_->trailersStrings()[1]) {} | ||
|
|
||
| Http::FilterHeadersStatus DecompressorFilter::decodeHeaders(Http::RequestHeaderMap& headers, | ||
| bool end_stream) { | ||
|
|
@@ -82,9 +87,24 @@ Http::FilterHeadersStatus DecompressorFilter::decodeHeaders(Http::RequestHeaderM | |
| *decoder_callbacks_, headers); | ||
| }; | ||
|
|
||
| Http::FilterDataStatus DecompressorFilter::decodeData(Buffer::Instance& data, bool) { | ||
| return maybeDecompress(config_->requestDirectionConfig(), request_decompressor_, | ||
| *decoder_callbacks_, data); | ||
| Http::FilterDataStatus DecompressorFilter::decodeData(Buffer::Instance& data, bool end_stream) { | ||
| if (request_decompressor_) { | ||
| HeaderMapOptRef trailers; | ||
| if (end_stream) { | ||
| trailers = HeaderMapOptRef(std::ref(decoder_callbacks_->addDecodedTrailers())); | ||
|
junr03 marked this conversation as resolved.
|
||
| } | ||
| decompress(config_->requestDirectionConfig(), request_decompressor_, *decoder_callbacks_, data, | ||
| request_byte_tracker_, trailers); | ||
| } | ||
| return Http::FilterDataStatus::Continue; | ||
| } | ||
|
|
||
| Http::FilterTrailersStatus DecompressorFilter::decodeTrailers(Http::RequestTrailerMap& trailers) { | ||
| // Only report if the filter has actually decompressed. | ||
| if (request_decompressor_) { | ||
| request_byte_tracker_.reportTotalBytes(trailers); | ||
| } | ||
| return Http::FilterTrailersStatus::Continue; | ||
| } | ||
|
|
||
| Http::FilterHeadersStatus DecompressorFilter::encodeHeaders(Http::ResponseHeaderMap& headers, | ||
|
|
@@ -99,29 +119,48 @@ Http::FilterHeadersStatus DecompressorFilter::encodeHeaders(Http::ResponseHeader | |
| *encoder_callbacks_, headers); | ||
| } | ||
|
|
||
| Http::FilterDataStatus DecompressorFilter::encodeData(Buffer::Instance& data, bool) { | ||
| return maybeDecompress(config_->responseDirectionConfig(), response_decompressor_, | ||
| *encoder_callbacks_, data); | ||
| Http::FilterDataStatus DecompressorFilter::encodeData(Buffer::Instance& data, bool end_stream) { | ||
| if (response_decompressor_) { | ||
| HeaderMapOptRef trailers; | ||
| if (end_stream) { | ||
| trailers = HeaderMapOptRef(std::ref(encoder_callbacks_->addEncodedTrailers())); | ||
| } | ||
| decompress(config_->responseDirectionConfig(), response_decompressor_, *encoder_callbacks_, | ||
| data, response_byte_tracker_, trailers); | ||
| } | ||
| return Http::FilterDataStatus::Continue; | ||
| } | ||
|
|
||
| Http::FilterDataStatus DecompressorFilter::maybeDecompress( | ||
| Http::FilterTrailersStatus DecompressorFilter::encodeTrailers(Http::ResponseTrailerMap& trailers) { | ||
| // Only report if the filter has actually decompressed. | ||
| if (response_decompressor_) { | ||
| response_byte_tracker_.reportTotalBytes(trailers); | ||
| } | ||
| return Http::FilterTrailersStatus::Continue; | ||
| } | ||
|
|
||
| void DecompressorFilter::decompress( | ||
| const DecompressorFilterConfig::DirectionConfig& direction_config, | ||
| const Compression::Decompressor::DecompressorPtr& decompressor, | ||
| Http::StreamFilterCallbacks& callbacks, Buffer::Instance& input_buffer) const { | ||
| if (decompressor) { | ||
| Buffer::OwnedImpl output_buffer; | ||
| decompressor->decompress(input_buffer, output_buffer); | ||
|
|
||
| // Report decompression via stats and logging before modifying the input buffer. | ||
| direction_config.stats().total_compressed_bytes_.add(input_buffer.length()); | ||
| direction_config.stats().total_uncompressed_bytes_.add(output_buffer.length()); | ||
| ENVOY_STREAM_LOG(debug, "{} data decompressed from {} bytes to {} bytes", callbacks, | ||
| direction_config.logString(), input_buffer.length(), output_buffer.length()); | ||
|
|
||
| input_buffer.drain(input_buffer.length()); | ||
| input_buffer.add(output_buffer); | ||
| Http::StreamFilterCallbacks& callbacks, Buffer::Instance& input_buffer, | ||
| ByteTracker& byte_tracker, HeaderMapOptRef trailers) const { | ||
| ASSERT(decompressor); | ||
| Buffer::OwnedImpl output_buffer; | ||
| decompressor->decompress(input_buffer, output_buffer); | ||
|
|
||
| // Report decompression via stats and logging before modifying the input buffer. | ||
| byte_tracker.chargeBytes(input_buffer.length(), output_buffer.length()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking desired behavior: Do we want to only increment byte counts if we actually do decompression (versus regardless of if we do decompression)?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean if
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The latter (i.e., if the response is not compressed)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My opinion is that the trailers should only be added if the stream was actually decompressed. So this is correct. But you did make me realize that the decode/encodeTrailers calls were not gated and that was wrong. Fixed in: 540a5ae |
||
| direction_config.stats().total_compressed_bytes_.add(input_buffer.length()); | ||
| direction_config.stats().total_uncompressed_bytes_.add(output_buffer.length()); | ||
| ENVOY_STREAM_LOG(debug, "{} data decompressed from {} bytes to {} bytes", callbacks, | ||
| direction_config.logString(), input_buffer.length(), output_buffer.length()); | ||
|
|
||
| input_buffer.drain(input_buffer.length()); | ||
| input_buffer.add(output_buffer); | ||
|
|
||
| if (trailers.has_value()) { | ||
| byte_tracker.reportTotalBytes(trailers.value().get()); | ||
| } | ||
| return Http::FilterDataStatus::Continue; | ||
| } | ||
|
|
||
| template <> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,9 +100,18 @@ class DecompressorFilterConfig { | |
| const std::string& contentEncoding() { return decompressor_factory_->contentEncoding(); } | ||
| const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; } | ||
| const ResponseDirectionConfig& responseDirectionConfig() { return response_direction_config_; } | ||
| // TODO: replace std::vector for std::pair. For some reason the macro is failing to build with a | ||
| // multivariate templated type. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| const std::vector<Http::LowerCaseString>& trailersStrings() const { | ||
| CONSTRUCT_ON_FIRST_USE( | ||
| std::vector<Http::LowerCaseString>, | ||
| {Http::LowerCaseString(fmt::format("{}-compressed-bytes", trailers_prefix_)), | ||
| Http::LowerCaseString(fmt::format("{}-uncompressed-bytes", trailers_prefix_))}); | ||
| } | ||
|
junr03 marked this conversation as resolved.
Outdated
junr03 marked this conversation as resolved.
Outdated
|
||
|
|
||
| private: | ||
| const std::string stats_prefix_; | ||
| const std::string trailers_prefix_; | ||
| const std::string decompressor_stats_prefix_; | ||
| const Compression::Decompressor::DecompressorFactoryPtr decompressor_factory_; | ||
| const RequestDirectionConfig request_direction_config_; | ||
|
|
@@ -122,12 +131,36 @@ class DecompressorFilter : public Http::PassThroughFilter, | |
| // Http::StreamDecoderFilter | ||
| Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override; | ||
| Http::FilterDataStatus decodeData(Buffer::Instance&, bool) override; | ||
| Http::FilterTrailersStatus decodeTrailers(Http::RequestTrailerMap&) override; | ||
|
|
||
| // Http::StreamEncoderFilter | ||
| Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool) override; | ||
| Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override; | ||
| Http::FilterTrailersStatus encodeTrailers(Http::ResponseTrailerMap&) override; | ||
|
|
||
| private: | ||
| struct ByteTracker { | ||
| ByteTracker(const Http::LowerCaseString& compressed_bytes_trailer, | ||
| const Http::LowerCaseString& uncompressed_bytes_trailer) | ||
| : compressed_bytes_trailer_(compressed_bytes_trailer), | ||
| uncompressed_bytes_trailer_(uncompressed_bytes_trailer) {} | ||
| void chargeBytes(uint64_t compressed_bytes, uint64_t uncompressed_bytes) { | ||
|
junr03 marked this conversation as resolved.
|
||
| total_compressed_bytes_ += compressed_bytes; | ||
| total_uncompressed_bytes_ += uncompressed_bytes; | ||
| } | ||
| void reportTotalBytes(Http::HeaderMap& trailers) const { | ||
| trailers.addReferenceKey(compressed_bytes_trailer_, total_compressed_bytes_); | ||
| trailers.addReferenceKey(uncompressed_bytes_trailer_, total_uncompressed_bytes_); | ||
| } | ||
|
|
||
| private: | ||
| const Http::LowerCaseString& compressed_bytes_trailer_; | ||
| const Http::LowerCaseString& uncompressed_bytes_trailer_; | ||
| uint64_t total_compressed_bytes_{}; | ||
| uint64_t total_uncompressed_bytes_{}; | ||
| }; | ||
| using ByteTrackerOptConstRef = absl::optional<std::reference_wrapper<const ByteTracker>>; | ||
|
|
||
| template <class HeaderType> | ||
| Http::FilterHeadersStatus | ||
| maybeInitDecompress(const DecompressorFilterConfig::DirectionConfig& direction_config, | ||
|
|
@@ -153,10 +186,11 @@ class DecompressorFilter : public Http::PassThroughFilter, | |
| return Http::FilterHeadersStatus::Continue; | ||
| } | ||
|
|
||
| Http::FilterDataStatus | ||
| maybeDecompress(const DecompressorFilterConfig::DirectionConfig& direction_config, | ||
| using HeaderMapOptRef = absl::optional<std::reference_wrapper<Http::HeaderMap>>; | ||
| void decompress(const DecompressorFilterConfig::DirectionConfig& direction_config, | ||
| const Compression::Decompressor::DecompressorPtr& decompressor, | ||
| Http::StreamFilterCallbacks& callbacks, Buffer::Instance& input_buffer) const; | ||
| Http::StreamFilterCallbacks& callbacks, Buffer::Instance& input_buffer, | ||
| ByteTracker& byte_tracker, HeaderMapOptRef trailers) const; | ||
|
|
||
| // TODO(junr03): These can be shared between compressor and decompressor. | ||
| template <Http::CustomInlineHeaderRegistry::Type Type> | ||
|
|
@@ -201,6 +235,8 @@ class DecompressorFilter : public Http::PassThroughFilter, | |
| DecompressorFilterConfigSharedPtr config_; | ||
| Compression::Decompressor::DecompressorPtr request_decompressor_{}; | ||
| Compression::Decompressor::DecompressorPtr response_decompressor_{}; | ||
| ByteTracker request_byte_tracker_; | ||
| ByteTracker response_byte_tracker_; | ||
| }; | ||
|
|
||
| } // namespace Decompressor | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.