decompressor library: Add stats to zlib library (#11782)

This adds decompressor error stats to the zlib decompressor library. This introduces a lot of boilerplate but will make it easier to continue to add stats in the future. Additionally, the compressor library can use the same pattern. Risk Level: Low. Testing: Updated. Docs Changes: Added. Signed-off-by: Jose Nino <[email protected]>
envoyproxy · Jul 12, 2020 · 84affaa · 84affaa
1 parent cb03985
commit 84affaa
Show file tree

Hide file tree

Showing 19 changed files with 343 additions and 72 deletions.
diff --git a/docs/root/configuration/http/http_filters/decompressor_filter.rst b/docs/root/configuration/http/http_filters/decompressor_filter.rst
@@ -110,3 +110,6 @@ with the following:
   not_decompressed, Counter, Number of request/responses not compressed.
   total_uncompressed_bytes, Counter, The total uncompressed bytes of all the request/responses that were marked for decompression.
   total_compressed_bytes, Counter, The total compressed bytes of all the request/responses that were marked for decompression.
+
+Additional stats for the decompressor library are rooted at
+<stat_prefix>.decompressor.<decompressor_library.name>.<decompressor_library_stat_prefix>.decompressor_library.
diff --git a/include/envoy/compression/decompressor/factory.h b/include/envoy/compression/decompressor/factory.h
@@ -10,7 +10,7 @@ class DecompressorFactory {
 public:
   virtual ~DecompressorFactory() = default;
 
-  virtual DecompressorPtr createDecompressor() PURE;
+  virtual DecompressorPtr createDecompressor(const std::string& stats_prefix) PURE;
   virtual const std::string& statsPrefix() const PURE;
   // TODO(junr03): this method assumes that decompressors are used on http messages.
   // A more generic method might be `hint()` which gives the user of the decompressor a hint about

diff --git a/source/extensions/compression/common/decompressor/factory_base.h b/source/extensions/compression/common/decompressor/factory_base.h
@@ -17,7 +17,8 @@ class DecompressorLibraryFactoryBase
                                      Server::Configuration::FactoryContext& context) override {
     return createDecompressorFactoryFromProtoTyped(
         MessageUtil::downcastAndValidate<const ConfigProto&>(proto_config,
-                                                             context.messageValidationVisitor()));
+                                                             context.messageValidationVisitor()),
+        context);
   }
 
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {
@@ -31,7 +32,8 @@ class DecompressorLibraryFactoryBase
 
 private:
   virtual Envoy::Compression::Decompressor::DecompressorFactoryPtr
-  createDecompressorFactoryFromProtoTyped(const ConfigProto&) PURE;
+  createDecompressorFactoryFromProtoTyped(const ConfigProto& proto_config,
+                                          Server::Configuration::FactoryContext& context) PURE;
 
   const std::string name_;
 };

diff --git a/source/extensions/compression/gzip/decompressor/BUILD b/source/extensions/compression/gzip/decompressor/BUILD
@@ -16,6 +16,8 @@ envoy_cc_library(
     external_deps = ["zlib"],
     deps = [
         "//include/envoy/compression/decompressor:decompressor_interface",
+        "//include/envoy/stats:stats_interface",
+        "//include/envoy/stats:stats_macros",
         "//source/common/buffer:buffer_lib",
         "//source/common/common:assert_lib",
         "//source/common/common:minimal_logger_lib",

diff --git a/source/extensions/compression/gzip/decompressor/config.cc b/source/extensions/compression/gzip/decompressor/config.cc
@@ -15,21 +15,24 @@ const uint32_t GzipHeaderValue = 16;
 } // namespace
 
 GzipDecompressorFactory::GzipDecompressorFactory(
-    const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip)
-    : window_bits_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(gzip, window_bits, DefaultWindowBits) |
+    const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip, Stats::Scope& scope)
+    : scope_(scope),
+      window_bits_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(gzip, window_bits, DefaultWindowBits) |
                    GzipHeaderValue),
       chunk_size_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(gzip, chunk_size, DefaultChunkSize)) {}
 
-Envoy::Compression::Decompressor::DecompressorPtr GzipDecompressorFactory::createDecompressor() {
-  auto decompressor = std::make_unique<ZlibDecompressorImpl>(chunk_size_);
+Envoy::Compression::Decompressor::DecompressorPtr
+GzipDecompressorFactory::createDecompressor(const std::string& stats_prefix) {
+  auto decompressor = std::make_unique<ZlibDecompressorImpl>(scope_, stats_prefix, chunk_size_);
   decompressor->init(window_bits_);
   return decompressor;
 }
 
 Envoy::Compression::Decompressor::DecompressorFactoryPtr
 GzipDecompressorLibraryFactory::createDecompressorFactoryFromProtoTyped(
-    const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config) {
-  return std::make_unique<GzipDecompressorFactory>(proto_config);
+    const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config,
+    Server::Configuration::FactoryContext& context) {
+  return std::make_unique<GzipDecompressorFactory>(proto_config, context.scope());
 }
 
 /**

diff --git a/source/extensions/compression/gzip/decompressor/config.h b/source/extensions/compression/gzip/decompressor/config.h
@@ -25,16 +25,19 @@ const std::string& gzipExtensionName() {
 
 class GzipDecompressorFactory : public Envoy::Compression::Decompressor::DecompressorFactory {
 public:
-  GzipDecompressorFactory(const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip);
+  GzipDecompressorFactory(const envoy::extensions::compression::gzip::decompressor::v3::Gzip& gzip,
+                          Stats::Scope& scope);
 
   // Envoy::Compression::Decompressor::DecompressorFactory
-  Envoy::Compression::Decompressor::DecompressorPtr createDecompressor() override;
+  Envoy::Compression::Decompressor::DecompressorPtr
+  createDecompressor(const std::string& stats_prefix) override;
   const std::string& statsPrefix() const override { return gzipStatsPrefix(); }
   const std::string& contentEncoding() const override {
     return Http::CustomHeaders::get().ContentEncodingValues.Gzip;
   }
 
 private:
+  Stats::Scope& scope_;
   const int32_t window_bits_;
   const uint32_t chunk_size_;
 };
@@ -47,7 +50,8 @@ class GzipDecompressorLibraryFactory
 
 private:
   Envoy::Compression::Decompressor::DecompressorFactoryPtr createDecompressorFactoryFromProtoTyped(
-      const envoy::extensions::compression::gzip::decompressor::v3::Gzip& config) override;
+      const envoy::extensions::compression::gzip::decompressor::v3::Gzip& proto_config,
+      Server::Configuration::FactoryContext& context) override;
 };
 
 DECLARE_FACTORY(GzipDecompressorLibraryFactory);

diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc
@@ -1,5 +1,7 @@
 #include "extensions/compression/gzip/decompressor/zlib_decompressor_impl.h"
 
+#include <zlib.h>
+
 #include <memory>
 
 #include "envoy/common/exception.h"
@@ -14,13 +16,17 @@ namespace Compression {
 namespace Gzip {
 namespace Decompressor {
 
-ZlibDecompressorImpl::ZlibDecompressorImpl() : ZlibDecompressorImpl(4096) {}
+ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix)
+    : ZlibDecompressorImpl(scope, stats_prefix, 4096) {}
 
-ZlibDecompressorImpl::ZlibDecompressorImpl(uint64_t chunk_size)
-    : Zlib::Base(chunk_size, [](z_stream* z) {
-        inflateEnd(z);
-        delete z;
-      }) {
+ZlibDecompressorImpl::ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix,
+                                           uint64_t chunk_size)
+    : Zlib::Base(chunk_size,
+                 [](z_stream* z) {
+                   inflateEnd(z);
+                   delete z;
+                 }),
+      stats_(generateStats(stats_prefix, scope)) {
   zstream_ptr_->zalloc = Z_NULL;
   zstream_ptr_->zfree = Z_NULL;
   zstream_ptr_->opaque = Z_NULL;
@@ -67,16 +73,40 @@ bool ZlibDecompressorImpl::inflateNext() {
 
   if (result < 0) {
     decompression_error_ = result;
-    ENVOY_LOG(
-        trace,
-        "zlib decompression error: {}. Error codes are defined in https://www.zlib.net/manual.html",
-        result);
+    ENVOY_LOG(trace,
+              "zlib decompression error: {}, msg: {}. Error codes are defined in "
+              "https://www.zlib.net/manual.html",
+              result, zstream_ptr_->msg);
+    chargeErrorStats(result);
     return false;
   }
 
   return true;
 }
 
+void ZlibDecompressorImpl::chargeErrorStats(const int result) {
+  switch (result) {
+  case Z_ERRNO:
+    stats_.zlib_errno_.inc();
+    break;
+  case Z_STREAM_ERROR:
+    stats_.zlib_stream_error_.inc();
+    break;
+  case Z_DATA_ERROR:
+    stats_.zlib_data_error_.inc();
+    break;
+  case Z_MEM_ERROR:
+    stats_.zlib_mem_error_.inc();
+    break;
+  case Z_BUF_ERROR:
+    stats_.zlib_buf_error_.inc();
+    break;
+  case Z_VERSION_ERROR:
+    stats_.zlib_version_error_.inc();
+    break;
+  }
+}
+
 } // namespace Decompressor
 } // namespace Gzip
 } // namespace Compression

diff --git a/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h b/source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h
@@ -1,6 +1,8 @@
 #pragma once
 
 #include "envoy/compression/decompressor/decompressor.h"
+#include "envoy/stats/scope.h"
+#include "envoy/stats/stats_macros.h"
 
 #include "common/common/logger.h"
 
@@ -14,14 +16,32 @@ namespace Compression {
 namespace Gzip {
 namespace Decompressor {
 
+/**
+ * All zlib decompressor stats. @see stats_macros.h
+ */
+#define ALL_ZLIB_DECOMPRESSOR_STATS(COUNTER)                                                       \
+  COUNTER(zlib_errno)                                                                              \
+  COUNTER(zlib_stream_error)                                                                       \
+  COUNTER(zlib_data_error)                                                                         \
+  COUNTER(zlib_mem_error)                                                                          \
+  COUNTER(zlib_buf_error)                                                                          \
+  COUNTER(zlib_version_error)
+
+/**
+ * Struct definition for zlib decompressor stats. @see stats_macros.h
+ */
+struct ZlibDecompressorStats {
+  ALL_ZLIB_DECOMPRESSOR_STATS(GENERATE_COUNTER_STRUCT)
+};
+
 /**
  * Implementation of decompressor's interface.
  */
 class ZlibDecompressorImpl : public Zlib::Base,
                              public Envoy::Compression::Decompressor::Decompressor,
                              public Logger::Loggable<Logger::Id::decompression> {
 public:
-  ZlibDecompressorImpl();
+  ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix);
 
   /**
    * Constructor that allows setting the size of decompressor's output buffer. It
@@ -31,7 +51,7 @@ class ZlibDecompressorImpl : public Zlib::Base,
    * 256K bytes. @see http://zlib.net/zlib_how.html
    * @param chunk_size amount of memory reserved for the decompressor output.
    */
-  ZlibDecompressorImpl(uint64_t chunk_size);
+  ZlibDecompressorImpl(Stats::Scope& scope, const std::string& stats_prefix, uint64_t chunk_size);
 
   /**
    * Init must be called in order to initialize the decompressor. Once decompressor is initialized,
@@ -49,7 +69,19 @@ class ZlibDecompressorImpl : public Zlib::Base,
   int decompression_error_{0};
 
 private:
+  // TODO: clean up friend class. This is here to allow coverage of chargeErrorStats as it isn't
+  // completely straightforward
+  // to cause zlib's inflate function to return all the error codes necessary to hit all the cases
+  // in the switch statement.
+  friend class ZlibDecompressorStatsTest;
+  static ZlibDecompressorStats generateStats(const std::string& prefix, Stats::Scope& scope) {
+    return ZlibDecompressorStats{ALL_ZLIB_DECOMPRESSOR_STATS(POOL_COUNTER_PREFIX(scope, prefix))};
+  }
+
   bool inflateNext();
+  void chargeErrorStats(const int result);
+
+  const ZlibDecompressorStats stats_;
 };
 
 } // namespace Decompressor

diff --git a/source/extensions/filters/http/decompressor/decompressor_filter.cc b/source/extensions/filters/http/decompressor/decompressor_filter.cc
@@ -27,6 +27,7 @@ DecompressorFilterConfig::DecompressorFilterConfig(
     : stats_prefix_(fmt::format("{}decompressor.{}.{}", stats_prefix,
                                 proto_config.decompressor_library().name(),
                                 decompressor_factory->statsPrefix())),
+      decompressor_stats_prefix_(stats_prefix_ + "decompressor_library"),
       decompressor_factory_(std::move(decompressor_factory)),
       request_direction_config_(proto_config.request_direction_config(), stats_prefix_, scope,
                                 runtime),

diff --git a/source/extensions/filters/http/decompressor/decompressor_filter.h b/source/extensions/filters/http/decompressor/decompressor_filter.h
@@ -95,14 +95,15 @@ class DecompressorFilterConfig {
       Compression::Decompressor::DecompressorFactoryPtr decompressor_factory);
 
   Compression::Decompressor::DecompressorPtr makeDecompressor() {
-    return decompressor_factory_->createDecompressor();
+    return decompressor_factory_->createDecompressor(decompressor_stats_prefix_);
   }
   const std::string& contentEncoding() { return decompressor_factory_->contentEncoding(); }
   const RequestDirectionConfig& requestDirectionConfig() { return request_direction_config_; }
   const ResponseDirectionConfig& responseDirectionConfig() { return response_direction_config_; }
 
 private:
   const std::string stats_prefix_;
+  const std::string decompressor_stats_prefix_;
   const Compression::Decompressor::DecompressorFactoryPtr decompressor_factory_;
   const RequestDirectionConfig request_direction_config_;
   const ResponseDirectionConfig response_direction_config_;

diff --git a/test/extensions/compression/gzip/compressor_fuzz_test.cc b/test/extensions/compression/gzip/compressor_fuzz_test.cc
@@ -1,5 +1,6 @@
 #include "common/buffer/buffer_impl.h"
 #include "common/common/assert.h"
+#include "common/stats/isolated_store_impl.h"
 
 #include "extensions/compression/gzip/compressor/zlib_compressor_impl.h"
 #include "extensions/compression/gzip/decompressor/zlib_decompressor_impl.h"
@@ -19,9 +20,11 @@ namespace Fuzz {
 // trip compress-decompress pair; the decompressor itself is not fuzzed beyond
 // whatever the compressor emits, as it exists only as a test utility today.
 DEFINE_FUZZER(const uint8_t* buf, size_t len) {
+
   FuzzedDataProvider provider(buf, len);
   ZlibCompressorImpl compressor;
-  Decompressor::ZlibDecompressorImpl decompressor;
+  Stats::IsolatedStoreImpl stats_store;
+  Decompressor::ZlibDecompressorImpl decompressor{stats_store, "test"};
 
   // Select target compression level. We can't use ConsumeEnum() since the range
   // is non-contiguous.

diff --git a/test/extensions/compression/gzip/decompressor/BUILD b/test/extensions/compression/gzip/decompressor/BUILD
@@ -18,6 +18,7 @@ envoy_extension_cc_test(
     deps = [
         "//source/common/common:assert_lib",
         "//source/common/common:hex_lib",
+        "//source/common/stats:isolated_store_lib",
         "//source/extensions/compression/gzip/compressor:compressor_lib",
         "//source/extensions/compression/gzip/decompressor:zlib_decompressor_impl_lib",
         "//test/test_common:utility_lib",