codec: add metadata_not_supported_error to HTTP/1 codec stats

docs/root/configuration/http_conn_man/stats.rst

@@ -98,7 +98,18 @@ following statistics:
 Per codec statistics
 -----------------------
 
-Each codec has the option of adding per-codec statistics. Currently only http2 has codec stats.
+Each codec has the option of adding per-codec statistics. Both http1 and http2 have codec stats.
+
+Http1 codec statistics
+~~~~~~~~~~~~~~~~~~~~~~
+
+All http1 statistics are rooted at *http1.*
+
+.. csv-table::
+ :header: Name, Type, Description
+ :widths: 1, 1, 2
+
+ metadata_not_supported_error, Counter, Total number of metadata dropped during HTTP/1 encoding
 
 Http2 codec statistics
 ~~~~~~~~~~~~~~~~~~~~~~

source/common/http/codec_client.cc

@@ -140,7 +140,8 @@ CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& conne
  : CodecClient(type, std::move(connection), host, dispatcher) {
  switch (type) {
  case Type::HTTP1: {
- codec_ = std::make_unique<Http1::ClientConnectionImpl>(*connection_, *this);
+ codec_ = std::make_unique<Http1::ClientConnectionImpl>(*connection_,
+ host->cluster().statsScope(), *this);
  break;
  }
  case Type::HTTP2: {

source/common/http/conn_manager_utility.cc

@@ -46,8 +46,8 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(
  return std::make_unique<Http2::ServerConnectionImpl>(connection, callbacks, scope,
  http2_settings, max_request_headers_kb);
  } else {
- return std::make_unique<Http1::ServerConnectionImpl>(connection, callbacks, http1_settings,
- max_request_headers_kb);
+ return std::make_unique<Http1::ServerConnectionImpl>(connection, scope, callbacks,
+ http1_settings, max_request_headers_kb);
  }
 }
 

source/common/http/http1/BUILD

@@ -18,6 +18,8 @@ envoy_cc_library(
  "//include/envoy/http:codec_interface",
  "//include/envoy/http:header_map_interface",
  "//include/envoy/network:connection_interface",
+ "//include/envoy/stats:stats_interface",
+ "//include/envoy/stats:stats_macros",
  "//source/common/buffer:buffer_lib",
  "//source/common/buffer:watermark_buffer_lib",
  "//source/common/common:assert_lib",

source/common/http/http1/codec_impl.cc

@@ -164,6 +164,10 @@ void StreamEncoderImpl::encodeData(Buffer::Instance& data, bool end_stream) {
 
 void StreamEncoderImpl::encodeTrailers(const HeaderMap&) { endEncode(); }
 
+void StreamEncoderImpl::encodeMetadata(const MetadataMapVector&) {
+ connection_.stats().metadata_not_supported_error_.inc();
+}
+
 void StreamEncoderImpl::endEncode() {
  if (chunk_encoding_) {
  connection_.buffer().add(LAST_CHUNK);
@@ -321,10 +325,11 @@ const ToLowerTable& ConnectionImpl::toLowerTable() {
  return *table;
 }
 
-ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type,
- uint32_t max_headers_kb)
- : connection_(connection), output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
- [&]() -> void { this->onAboveHighWatermark(); }),
+ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& stats,
+ http_parser_type type, uint32_t max_headers_kb)
+ : connection_(connection), stats_{ALL_HTTP1_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http1."))},
+ output_buffer_([&]() -> void { this->onBelowLowWatermark(); },
+ [&]() -> void { this->onAboveHighWatermark(); }),
  max_headers_kb_(max_headers_kb), strict_header_validation_(Runtime::runtimeFeatureEnabled(
  "envoy.reloadable_features.strict_header_validation")) {
  output_buffer_.setWatermarks(connection.bufferLimit());
@@ -506,11 +511,11 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) {
  onResetStream(reason);
 }
 
-ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection,
+ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Stats::Scope& stats,
  ServerConnectionCallbacks& callbacks,
  Http1Settings settings, uint32_t max_request_headers_kb)
- : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb), callbacks_(callbacks),
- codec_settings_(settings) {}
+ : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb),
+ callbacks_(callbacks), codec_settings_(settings) {}
 
 void ServerConnectionImpl::onEncodeComplete() {
  ASSERT(active_request_);
@@ -681,8 +686,9 @@ void ServerConnectionImpl::onBelowLowWatermark() {
  }
 }
 
-ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&)
- : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {}
+ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Stats::Scope& stats,
+ ConnectionCallbacks&)
+ : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {}
 
 bool ClientConnectionImpl::cannotHaveBody() {
  if ((!pending_responses_.empty() && pending_responses_.front().head_request_) ||

source/common/http/http1/codec_impl.h

@@ -10,6 +10,7 @@
 
 #include "envoy/http/codec.h"
 #include "envoy/network/connection.h"
+#include "envoy/stats/scope.h"
 
 #include "common/buffer/watermark_buffer.h"
 #include "common/common/assert.h"
@@ -22,6 +23,21 @@ namespace Envoy {
 namespace Http {
 namespace Http1 {
 
+/**
+ * All stats for the HTTP/1 codec. @see stats_macros.h
+ */
+// clang-format off
+#define ALL_HTTP1_CODEC_STATS(COUNTER) \
+ COUNTER(metadata_not_supported_error) \
+// clang-format on
+
+/**
+ * Wrapper struct for the HTTP/1 codec stats. @see stats_macros.h
+ */
+struct CodecStats {
+ ALL_HTTP1_CODEC_STATS(GENERATE_COUNTER_STRUCT)
+};
+
 class ConnectionImpl;
 
 /**
@@ -37,7 +53,7 @@ class StreamEncoderImpl : public StreamEncoder,
  void encodeHeaders(const HeaderMap& headers, bool end_stream) override;
  void encodeData(Buffer::Instance& data, bool end_stream) override;
  void encodeTrailers(const HeaderMap& trailers) override;
- void encodeMetadata(const MetadataMapVector&) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; }
+ void encodeMetadata(const MetadataMapVector&) override;
  Stream& getStream() override { return *this; }
 
  // Http::Stream
@@ -171,13 +187,16 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
 
  bool maybeDirectDispatch(Buffer::Instance& data);
 
+ CodecStats& stats() { return stats_; }
+
 protected:
- ConnectionImpl(Network::Connection& connection, http_parser_type type,
+ ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, http_parser_type type,
  uint32_t max_request_headers_kb);
 
  bool resetStreamCalled() { return reset_stream_called_; }
 
  Network::Connection& connection_;
+ CodecStats stats_;
  http_parser parser_;
  HeaderMapPtr deferred_end_stream_headers_;
  Http::Code error_code_{Http::Code::BadRequest};
@@ -291,8 +310,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
  */
 class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
 public:
- ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
- Http1Settings settings, uint32_t max_request_headers_kb);
+ ServerConnectionImpl(Network::Connection& connection, Stats::Scope& stats,
+ ServerConnectionCallbacks& callbacks, Http1Settings settings,
+ uint32_t max_request_headers_kb);
 
  ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks,
  Http1Settings settings, uint32_t max_request_headers_kb,
@@ -346,7 +366,8 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl {
  */
 class ClientConnectionImpl : public ClientConnection, public ConnectionImpl {
 public:
- ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks& callbacks);
+ ClientConnectionImpl(Network::Connection& connection, Stats::Scope& stats,
+ ConnectionCallbacks& callbacks);
 
  // Http::ClientConnection
  StreamEncoder& newStream(StreamDecoder& response_decoder) override;

source/extensions/filters/network/http_connection_manager/config.cc

@@ -368,7 +368,7 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection,
  switch (codec_type_) {
  case CodecType::HTTP1:
  return std::make_unique<Http::Http1::ServerConnectionImpl>(
- connection, callbacks, http1_settings_, maxRequestHeadersKb());
+ connection, context_.scope(), callbacks, http1_settings_, maxRequestHeadersKb());
  case CodecType::HTTP2:
  return std::make_unique<Http::Http2::ServerConnectionImpl>(
  connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb());

test/common/http/codec_impl_fuzz_test.cc

@@ -359,7 +359,8 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi
  stats_store, client_http2settings,
  max_request_headers_kb);
  } else {
- client = std::make_unique<Http1::ClientConnectionImpl>(client_connection, client_callbacks);
+ client = std::make_unique<Http1::ClientConnectionImpl>(client_connection, stats_store,
+ client_callbacks);
  }
 
  NiceMock<Network::MockConnection> server_connection;
@@ -371,8 +372,9 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi
  max_request_headers_kb);
  } else {
  const Http1Settings server_http1settings{fromHttp1Settings(input.h1_settings().server())};
- server = std::make_unique<Http1::ServerConnectionImpl>(
- server_connection, server_callbacks, server_http1settings, max_request_headers_kb);
+ server = std::make_unique<Http1::ServerConnectionImpl>(server_connection, stats_store,
+ server_callbacks, server_http1settings,
+ max_request_headers_kb);
  }
 
  ReorderBuffer client_write_buf{*server};

test/common/http/http1/codec_impl_test.cc

@@ -49,8 +49,8 @@ class Http1ServerConnectionImplTest : public testing::Test {
  }
 
  void initialize() {
- codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_,
- max_request_headers_kb_);
+ codec_ = std::make_unique<ServerConnectionImpl>(connection_, store_, callbacks_,
+ codec_settings_, max_request_headers_kb_);
  }
 
  NiceMock<Network::MockConnection> connection_;
@@ -84,8 +84,8 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur
 
  if (allow_absolute_url) {
  codec_settings_.allow_absolute_url_ = allow_absolute_url;
- codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_,
- max_request_headers_kb_);
+ codec_ = std::make_unique<ServerConnectionImpl>(connection_, store_, callbacks_,
+ codec_settings_, max_request_headers_kb_);
  }
 
  Http::MockStreamDecoder decoder;
@@ -104,8 +104,8 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs
  // Make a new 'codec' with the right settings
  if (allow_absolute_url) {
  codec_settings_.allow_absolute_url_ = allow_absolute_url;
- codec_ = std::make_unique<ServerConnectionImpl>(connection_, callbacks_, codec_settings_,
- max_request_headers_kb_);
+ codec_ = std::make_unique<ServerConnectionImpl>(connection_, store_, callbacks_,
+ codec_settings_, max_request_headers_kb_);
  }
 
  Http::MockStreamDecoder decoder;
@@ -578,9 +578,7 @@ TEST_F(Http1ServerConnectionImplTest, HeaderOnlyResponseWith100Then200) {
  EXPECT_EQ("HTTP/1.1 200 OK\r\ncontent-length: 0\r\n\r\n", output);
 }
 
-class Http1ServerConnectionImplDeathTest : public Http1ServerConnectionImplTest {};
-
-TEST_F(Http1ServerConnectionImplDeathTest, MetadataTest) {
+TEST_F(Http1ServerConnectionImplTest, MetadataTest) {
  initialize();
 
  NiceMock<Http::MockStreamDecoder> decoder;
@@ -598,7 +596,8 @@ TEST_F(Http1ServerConnectionImplDeathTest, MetadataTest) {
  MetadataMapPtr metadata_map_ptr = std::make_unique<MetadataMap>(metadata_map);
  MetadataMapVector metadata_map_vector;
  metadata_map_vector.push_back(std::move(metadata_map_ptr));
- EXPECT_DEATH_LOG_TO_STDERR(response_encoder->encodeMetadata(metadata_map_vector), "");
+ response_encoder->encodeMetadata(metadata_map_vector);
+ EXPECT_EQ(1, store_.counter("http1.metadata_not_supported_error").value());
 }
 
 TEST_F(Http1ServerConnectionImplTest, ChunkedResponse) {
@@ -860,7 +859,9 @@ class Http1ClientConnectionImplTest : public testing::Test {
  generator_, validation_visitor_, *api_)});
  }
 
- void initialize() { codec_ = std::make_unique<ClientConnectionImpl>(connection_, callbacks_); }
+ void initialize() {
+ codec_ = std::make_unique<ClientConnectionImpl>(connection_, store_, callbacks_);
+ }
 
  NiceMock<Network::MockConnection> connection_;
  NiceMock<Http::MockConnectionCallbacks> callbacks_;

test/integration/fake_upstream.cc

@@ -221,7 +221,8 @@ FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connectio
  : FakeConnectionBase(shared_connection, time_system) {
  if (type == Type::HTTP1) {
  codec_ = std::make_unique<Http::Http1::ServerConnectionImpl>(
- shared_connection_.connection(), *this, Http::Http1Settings(), max_request_headers_kb);
+ shared_connection_.connection(), store, *this, Http::Http1Settings(),
+ max_request_headers_kb);
  } else {
  auto settings = Http::Http2Settings();
  settings.allow_connect_ = true;