This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 2119ac95f4 Compact response HTTP version log fields (#13568)
2119ac95f4 is described below
commit 2119ac95f4e745e716aace2fdb105743344c2189
Author: Brian Neradt <[email protected]>
AuthorDate: Wed Aug 19 12:44:31 2026 -0500
Compact response HTTP version log fields (#13568)
The sshv and csshv access log fields store two one-byte version
components as separate 64-bit integers, consuming 16 bytes per entry and
publishing dINT framing in binary log schemas.
This patch addresses the overhead by marshaling major.minor strings in
one 8-byte-aligned slot while preserving the existing HTTP/major.minor
ASCII form. It updates v3 schema expectations and regression coverage
for both fields.
Fixes: #12891
---
.../binary-log-v3-format.en.rst | 16 ++---
include/proxy/logging/LogAccess.h | 4 +-
include/proxy/logging/LogField.h | 2 +-
src/proxy/logging/Log.cc | 4 +-
src/proxy/logging/LogAccess.cc | 80 ++++++++--------------
src/proxy/logging/unit-tests/test_LogAccess.cc | 30 +++++---
src/proxy/logging/unit-tests/test_LogBuffer.cc | 11 +--
src/traffic_logcat/unit-tests/test_LogEntryJson.cc | 6 +-
tests/gold_tests/logging/binary_log_v3.test.py | 4 +-
.../logging/gold/binary_log_v3_json.gold | 6 +-
10 files changed, 74 insertions(+), 89 deletions(-)
diff --git
a/doc/developer-guide/logging-architecture/binary-log-v3-format.en.rst
b/doc/developer-guide/logging-architecture/binary-log-v3-format.en.rst
index 7deb532ae1..fca1247ca8 100644
--- a/doc/developer-guide/logging-architecture/binary-log-v3-format.en.rst
+++ b/doc/developer-guide/logging-architecture/binary-log-v3-format.en.rst
@@ -104,9 +104,8 @@ Code Name Wire encoding
meets it -- or any code it does not recognize -- cannot
determine the field length and must stop decoding the entry.
1 sINT A single ``int64_t``, fixed 8 bytes, **host byte order**.
-2 dINT Two ``int64_t`` (16 bytes), host byte order. Used for
- values stored as two integers, e.g. HTTP version
- major/minor.
+2 dINT Two ``int64_t`` (16 bytes), host byte order. Used for values
+ stored as two integers.
3 STRING NUL-terminated bytes, then padded to an 8-byte boundary.
4 IP ``uint16_t`` address family followed by a family-sized
address, then padded to an 8-byte boundary (see below).
@@ -116,8 +115,8 @@ The code reflects how the value is *framed* on disk, i.e.
how a reader walks
(or skips) it -- not what the value means. (The ``sINT``/``dINT`` names are an
ATS-internal distinction; on the wire ``sINT`` is one 8-byte integer and
``dINT`` is two consecutive ones.) How a consumer *renders* a value -- mapping
-a cache-result integer to ``TCP_HIT``, or a ``dINT`` to ``1.1`` -- is layered
-on top by the consumer and is not part of the wire format.
+a cache-result integer to ``TCP_HIT``, for example -- is layered on top by the
+consumer and is not part of the wire format.
Value encodings
===============
@@ -129,10 +128,9 @@ sINT
portability is future work.
dINT
- Two consecutive ``sINT`` values: 16 bytes total, in host byte order. Used
- where one log field is stored as two integers, such as an HTTP version
- (major then minor). The reference decoder renders it as a JSON array, e.g.
- ``[1,1]``; turning that into ``1.1`` is a consumer concern.
+ Two consecutive ``sINT`` values: 16 bytes total, in host byte order. The
+ reference decoder renders them as a JSON array, e.g. ``[1,1]``;
interpreting
+ that pair is a consumer concern.
STRING
The string bytes followed by a single NUL, then zero padding up to the next
diff --git a/include/proxy/logging/LogAccess.h
b/include/proxy/logging/LogAccess.h
index 35f14ea55c..ea7a2aa3fa 100644
--- a/include/proxy/logging/LogAccess.h
+++ b/include/proxy/logging/LogAccess.h
@@ -224,7 +224,7 @@ public:
int marshal_server_resp_squid_len(char *); // INT
int marshal_server_resp_content_len(char *); // INT
int marshal_server_resp_header_len(char *); // INT
- int marshal_server_resp_http_version(char *); // INT
+ int marshal_server_resp_http_version(char *); // STR
int marshal_server_resp_time_ms(char *); // INT
int marshal_server_resp_time_s(char *); // INT
int marshal_server_transact_count(char *); // INT
@@ -240,7 +240,7 @@ public:
int marshal_cache_resp_squid_len(char *); // INT
int marshal_cache_resp_content_len(char *); // INT
int marshal_cache_resp_header_len(char *); // INT
- int marshal_cache_resp_http_version(char *); // INT
+ int marshal_cache_resp_http_version(char *); // STR
int marshal_cache_resp_all_header_fields(char *); // STR
void set_client_req_url(char *, int); // STR
diff --git a/include/proxy/logging/LogField.h b/include/proxy/logging/LogField.h
index 1fbb3dc216..819889477b 100644
--- a/include/proxy/logging/LogField.h
+++ b/include/proxy/logging/LogField.h
@@ -107,7 +107,7 @@ public:
enum class Type : uint8_t {
INVALID = 0, ///< Reserved: never written; a reader treats 0 (or any
unknown code) as unframmable and stops.
sINT = 1, ///< one int64_t, 8 bytes (host byte order).
- dINT = 2, ///< two int64_t (16 bytes), e.g. HTTP version major/minor.
+ dINT = 2, ///< two int64_t (16 bytes).
STRING = 3, ///< NUL-terminated, 8-byte padded.
IP = 4, ///< uint16_t family + family-sized address, 8-byte padded.
N_TYPES = 5, ///< Internal bound (asserts / name table); NOT a wire code.
diff --git a/src/proxy/logging/Log.cc b/src/proxy/logging/Log.cc
index 6e72ac408b..e87b345321 100644
--- a/src/proxy/logging/Log.cc
+++ b/src/proxy/logging/Log.cc
@@ -900,7 +900,7 @@ Log::init_fields()
global_field_list.add(field, false);
field_symbol_hash.emplace("ssql", field);
- field = new LogField("server_resp_http_version", "sshv",
LogField::Type::dINT, &LogAccess::marshal_server_resp_http_version,
+ field = new LogField("server_resp_http_version", "sshv",
LogField::Type::STRING, &LogAccess::marshal_server_resp_http_version,
&LogAccess::unmarshal_http_version);
global_field_list.add(field, false);
field_symbol_hash.emplace("sshv", field);
@@ -975,7 +975,7 @@ Log::init_fields()
global_field_list.add(field, false);
field_symbol_hash.emplace("cssql", field);
- field = new LogField("cached_resp_http_version", "csshv",
LogField::Type::dINT, &LogAccess::marshal_cache_resp_http_version,
+ field = new LogField("cached_resp_http_version", "csshv",
LogField::Type::STRING, &LogAccess::marshal_cache_resp_http_version,
&LogAccess::unmarshal_http_version);
global_field_list.add(field, false);
field_symbol_hash.emplace("csshv", field);
diff --git a/src/proxy/logging/LogAccess.cc b/src/proxy/logging/LogAccess.cc
index 6d0d2fe664..1114af1b05 100644
--- a/src/proxy/logging/LogAccess.cc
+++ b/src/proxy/logging/LogAccess.cc
@@ -53,6 +53,22 @@ DbgCtl dbg_ctl_log_resolve{"log-resolve"};
DbgCtl dbg_ctl_log_unmarshal_orun{"log-unmarshal-orun"}; // Overrun of
unmarshaling destination buffer.
DbgCtl dbg_ctl_log_unmarshal_data{"log-unmarshal-data"}; // Error in txn data
when unmarshalling.
+int
+marshal_http_version_string(char *buf, HTTPHdr *response)
+{
+ HTTPVersion version = response ? response->version_get() : HTTP_INVALID;
+ char str[8];
+ int str_len =
+ snprintf(str, sizeof(str), "%u.%u",
static_cast<unsigned>(version.get_major()),
static_cast<unsigned>(version.get_minor()));
+ int len = LogAccess::padded_length(str_len + 1);
+
+ ink_release_assert(str_len > 0 && str_len < static_cast<int>(sizeof(str)));
+ if (buf) {
+ LogAccess::marshal_str(buf, str, len);
+ }
+ return len;
+}
+
} // end anonymous namespace
#define DBG_UNMARSHAL_DEST_OVERRUN Dbg(dbg_ctl_log_unmarshal_orun, "Unmarshal
destination buffer overrun.");
@@ -1066,9 +1082,8 @@ LogAccess::marshal_cache_resp_all_header_fields(char *buf)
/*-------------------------------------------------------------------------
LogAccess::unmarshal_http_version
- The http version is marshalled as two consecutive integers, the first for
- the major number and the second for the minor number. Retrieve both
- numbers and return the result as "HTTP/major.minor".
+ The HTTP version is marshalled as a "major.minor" string. Prefix it with
+ "HTTP/" for the established text log representation.
-------------------------------------------------------------------------*/
int
@@ -1078,34 +1093,17 @@ LogAccess::unmarshal_http_version(char **buf, char
*dest, int len)
ink_assert(*buf != nullptr);
ink_assert(dest != nullptr);
- static const char http[] = "HTTP/";
- static int http_len = static_cast<int>(sizeof(http) - 1);
+ static constexpr std::string_view prefix{"HTTP/"};
- char val_buf[128];
- char *p = val_buf;
+ char *version = *buf;
+ int version_len = static_cast<int>(strlen(version));
+ int value_len = static_cast<int>(prefix.size()) + version_len;
- auto vb_left = [&]() -> int { return sizeof(val_buf) - (p - val_buf); };
-
- memcpy(p, http, http_len);
- p += http_len;
-
- int res1 = unmarshal_int_to_str(buf, p, vb_left());
- if (res1 < 0) {
- return -1;
- }
- p += res1;
- *p++ = '.';
- int res2 = unmarshal_int_to_str(buf, p, vb_left());
- if (res2 < 0) {
- DBG_UNMARSHAL_DEST_OVERRUN
- return -1;
- }
- p += res2;
-
- int val_len = p - val_buf;
- if (val_len < len) {
- memcpy(dest, val_buf, val_len);
- return val_len;
+ *buf += padded_strlen(version);
+ if (value_len <= len) {
+ memcpy(dest, prefix.data(), prefix.size());
+ memcpy(dest + prefix.size(), version, version_len);
+ return value_len;
}
DBG_UNMARSHAL_DEST_OVERRUN
return -1;
@@ -2922,17 +2920,7 @@ LogAccess::marshal_server_resp_squid_len(char *buf)
int
LogAccess::marshal_server_resp_http_version(char *buf)
{
- if (buf) {
- int64_t major = 0;
- int64_t minor = 0;
- if (m_server_response) {
- major = m_server_response->version_get().get_major();
- minor = m_server_response->version_get().get_minor();
- }
- marshal_int(buf, major);
- marshal_int((buf + INK_MIN_ALIGN), minor);
- }
- return (2 * INK_MIN_ALIGN);
+ return marshal_http_version_string(buf, m_server_response);
}
/*-------------------------------------------------------------------------
@@ -3073,17 +3061,7 @@ LogAccess::marshal_cache_resp_header_len(char *buf)
int
LogAccess::marshal_cache_resp_http_version(char *buf)
{
- if (buf) {
- int64_t major = 0;
- int64_t minor = 0;
- if (m_cache_response) {
- major = m_cache_response->version_get().get_major();
- minor = m_cache_response->version_get().get_minor();
- }
- marshal_int(buf, major);
- marshal_int((buf + INK_MIN_ALIGN), minor);
- }
- return (2 * INK_MIN_ALIGN);
+ return marshal_http_version_string(buf, m_cache_response);
}
int
diff --git a/src/proxy/logging/unit-tests/test_LogAccess.cc
b/src/proxy/logging/unit-tests/test_LogAccess.cc
index a3158aba4d..0a33a6d8df 100644
--- a/src/proxy/logging/unit-tests/test_LogAccess.cc
+++ b/src/proxy/logging/unit-tests/test_LogAccess.cc
@@ -215,23 +215,31 @@ TEST_CASE("LogAccess non-HttpSM client host port is
null-safe", "[LogAccess]")
CHECK(marshal_int_value([&](char *buf) { return
access.marshal_client_host_port(buf); }) == 4321);
}
-TEST_CASE("LogAccess unmarshal_http_version keeps the minor version",
"[LogAccess]")
+TEST_CASE("LogAccess marshals response HTTP versions as compact strings",
"[LogAccess]")
{
- auto render = [](int64_t major, int64_t minor) -> std::string {
- char marshalled[2 * INK_MIN_ALIGN];
- LogAccess::marshal_int(marshalled, major);
- LogAccess::marshal_int(marshalled + INK_MIN_ALIGN, minor);
+ NonHttpSmLogData data;
+ populate_non_http_sm_data(data, "GET", "https"sv, "example.com",
"/version"sv);
+ TransactionLogData log_data(data);
+ LogAccess access(log_data);
+
+ access.init();
+
+ auto check = [](auto marshal) {
+ CHECK(marshal(nullptr) == INK_MIN_ALIGN);
+ CHECK(marshal_string(marshal) == "0.0");
+
+ char marshalled[INK_MIN_ALIGN] = {};
+ marshal(marshalled);
char dest[64] = {};
char *src = marshalled;
int len = LogAccess::unmarshal_http_version(&src, dest,
sizeof(dest));
+
REQUIRE(len > 0);
- return std::string(dest, len);
+ CHECK(std::string(dest, len) == "HTTP/0.0");
+ CHECK(src == marshalled + INK_MIN_ALIGN);
};
- CHECK(render(1, 1) == "HTTP/1.1");
- CHECK(render(1, 0) == "HTTP/1.0");
- // known bug of `.0` suffix for HTTP/2 and HTTP/3
- CHECK(render(2, 0) == "HTTP/2.0");
- CHECK(render(3, 0) == "HTTP/3.0");
+ check([&](char *buf) { return access.marshal_server_resp_http_version(buf);
});
+ check([&](char *buf) { return access.marshal_cache_resp_http_version(buf);
});
}
diff --git a/src/proxy/logging/unit-tests/test_LogBuffer.cc
b/src/proxy/logging/unit-tests/test_LogBuffer.cc
index 4c019e7890..79778e9f5d 100644
--- a/src/proxy/logging/unit-tests/test_LogBuffer.cc
+++ b/src/proxy/logging/unit-tests/test_LogBuffer.cc
@@ -46,9 +46,9 @@ TEST_CASE("LogField::Type reflects each field's on-wire
framing", "[logging][v3]
Log::init_fields();
// Each field's declared type must match its on-wire framing, since the
schema
- // serializes Type directly: pssc/crc single ints, cqu string, chi IP, sshv a
- // pair (HTTP version), ppv a string (after the sINT/dINT type fixes).
- LogFormat fmt("v3wt", "%<pssc> %<crc> %<cqu> %<chi> %<sshv> %<ppv>");
+ // serializes Type directly: pssc/crc single ints, cqu/sshv/csshv/ppv
+ // strings, and chi an IP.
+ LogFormat fmt("v3wt", "%<pssc> %<crc> %<cqu> %<chi> %<sshv> %<csshv>
%<ppv>");
REQUIRE(fmt.valid());
std::vector<LogField::Type> types;
@@ -56,8 +56,9 @@ TEST_CASE("LogField::Type reflects each field's on-wire
framing", "[logging][v3]
types.push_back(f->type());
}
- REQUIRE(types == (std::vector<LogField::Type>{LogField::Type::sINT,
LogField::Type::sINT, LogField::Type::STRING,
- LogField::Type::IP,
LogField::Type::dINT, LogField::Type::STRING}));
+ REQUIRE(types ==
+ (std::vector<LogField::Type>{LogField::Type::sINT,
LogField::Type::sINT, LogField::Type::STRING, LogField::Type::IP,
+ LogField::Type::STRING,
LogField::Type::STRING, LogField::Type::STRING}));
}
TEST_CASE("marshal_int / unmarshal_int round-trip", "[logging][v3]")
diff --git a/src/traffic_logcat/unit-tests/test_LogEntryJson.cc
b/src/traffic_logcat/unit-tests/test_LogEntryJson.cc
index 6d44506a93..c3ef245ca9 100644
--- a/src/traffic_logcat/unit-tests/test_LogEntryJson.cc
+++ b/src/traffic_logcat/unit-tests/test_LogEntryJson.cc
@@ -295,7 +295,7 @@ TEST_CASE("v3 generic decode rejects field_count not
matching the symbol list",
TEST_CASE("v3 generic decode reads a dINT field (16 bytes)", "[logcat][v3]")
{
V3Segment seg;
- init_segment(seg, "sshv", {LogField::Type::dINT});
+ init_segment(seg, "pair", {LogField::Type::dINT});
auto *entry = reinterpret_cast<LogEntryHeader *>(seg.storage + DATA_OFF);
char *w = reinterpret_cast<char *>(entry) + sizeof(LogEntryHeader);
@@ -309,13 +309,13 @@ TEST_CASE("v3 generic decode reads a dINT field (16
bytes)", "[logcat][v3]")
char out[256];
int n = log_entry_to_json(entry, seg.header(), out, sizeof(out));
REQUIRE(n > 0);
- CHECK(std::string(out, n) == R"({"sshv":[1,1]})");
+ CHECK(std::string(out, n) == R"({"pair":[1,1]})");
}
TEST_CASE("v3 generic decode rejects a truncated dINT field", "[logcat][v3]")
{
V3Segment seg;
- init_segment(seg, "sshv", {LogField::Type::dINT});
+ init_segment(seg, "pair", {LogField::Type::dINT});
auto *entry = reinterpret_cast<LogEntryHeader *>(seg.storage + DATA_OFF);
char *w = reinterpret_cast<char *>(entry) + sizeof(LogEntryHeader);
diff --git a/tests/gold_tests/logging/binary_log_v3.test.py
b/tests/gold_tests/logging/binary_log_v3.test.py
index 9701a389f6..e3fad87462 100644
--- a/tests/gold_tests/logging/binary_log_v3.test.py
+++ b/tests/gold_tests/logging/binary_log_v3.test.py
@@ -43,7 +43,7 @@ class BinaryLogV3Test:
# chi = client IP -> IP -> "127.0.0.1"
# cqu = request URL -> STRING -> "http://127.0.0.1:<port>/get"
# pssc = response status -> sINT -> "200"
- # sshv = resp HTTP version -> dINT -> "HTTP/1.1" (ASCII) / [1,1] (JSON)
+ # sshv = resp HTTP version -> STRING -> "HTTP/1.1" (ASCII) / "1.1" (JSON)
# The dynamic listen port in cqu is masked with `` in the gold files.
log_format = '%<chi> %<cqu> %<pssc> %<sshv>'
@@ -151,7 +151,7 @@ class BinaryLogV3Test:
stdout += Testers.ContainsExpression(r'chi\s+IP', 'chi is framed as an
IP.')
stdout += Testers.ContainsExpression(r'cqu\s+STRING', 'cqu is framed
as a STRING.')
stdout += Testers.ContainsExpression(r'pssc\s+sINT', 'pssc is framed
as an sINT.')
- stdout += Testers.ContainsExpression(r'sshv\s+dINT', 'sshv is framed
as a dINT.')
+ stdout += Testers.ContainsExpression(r'sshv\s+STRING', 'sshv is framed
as a STRING.')
# traffic_logcat -H also prints the v2 header, but v2 segments carry no
# field-type schema.
diff --git a/tests/gold_tests/logging/gold/binary_log_v3_json.gold
b/tests/gold_tests/logging/gold/binary_log_v3_json.gold
index 3889f46817..452579eb8e 100644
--- a/tests/gold_tests/logging/gold/binary_log_v3_json.gold
+++ b/tests/gold_tests/logging/gold/binary_log_v3_json.gold
@@ -1,3 +1,3 @@
-{"chi":"127.0.0.1","cqu":"http://127.0.0.1:``/get","pssc":200,"sshv":[1,1]}
-{"chi":"127.0.0.1","cqu":"http://127.0.0.1:``/get","pssc":200,"sshv":[1,1]}
-{"chi":"127.0.0.1","cqu":"http://127.0.0.1:``/get","pssc":200,"sshv":[1,1]}
+{"chi":"127.0.0.1","cqu":"http://127.0.0.1:``/get","pssc":200,"sshv":"1.1"}
+{"chi":"127.0.0.1","cqu":"http://127.0.0.1:``/get","pssc":200,"sshv":"1.1"}
+{"chi":"127.0.0.1","cqu":"http://127.0.0.1:``/get","pssc":200,"sshv":"1.1"}
