Copilot commented on code in PR #13843:
URL: https://github.com/apache/trafficserver/pull/13843#discussion_r4286319918
##########
plugins/stats_over_http/stats_over_http.cc:
##########
@@ -298,116 +223,77 @@ stats_process_accept(TSCont contp, stats_state *my_state)
my_state->read_vio = TSVConnRead(my_state->net_vc, contp,
my_state->req_buffer, INT64_MAX);
}
-static int64_t
-stats_add_data_to_resp_buffer(const char *s, stats_state *my_state)
+static void
+stats_append(const char *s, stats_state *my_state)
{
- if (s == nullptr) {
- return 0;
+ if (s != nullptr) {
+ my_state->body.append(s);
}
- int64_t s_len = strlen(s);
-
- int64_t bytes_written = TSIOBufferWrite(my_state->resp_buffer, s, s_len);
- if (bytes_written == TS_ERROR) {
- return 0;
- }
-
- return bytes_written;
}
-static const char RESP_HEADER_JSON[] = "HTTP/1.0 200 OK\r\nContent-Type:
text/json\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_JSON_GZIP[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/json\r\nContent-Encoding:
gzip\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_JSON_DEFLATE[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/json\r\nContent-Encoding:
deflate\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_JSON_BR[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/json\r\nContent-Encoding:
br\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_CSV[] = "HTTP/1.0 200 OK\r\nContent-Type:
text/csv\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_CSV_GZIP[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/csv\r\nContent-Encoding:
gzip\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_CSV_DEFLATE[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/csv\r\nContent-Encoding:
deflate\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_CSV_BR[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/csv\r\nContent-Encoding:
br\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/plain; version=0.0.4;
charset=utf-8\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_GZIP[] = "HTTP/1.0 200
OK\r\nContent-Type: text/plain; version=0.0.4; "
-
"charset=utf-8\r\nContent-Encoding: gzip\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_DEFLATE[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/plain; version=0.0.4;
charset=utf-8\r\nContent-Encoding: deflate\r\nCache-Control: "
- "no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_BR[] = "HTTP/1.0 200
OK\r\nContent-Type: text/plain; version=0.0.4; "
-
"charset=utf-8\r\nContent-Encoding: br\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_V2[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/plain; version=2.0.0;
charset=utf-8\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_V2_GZIP[] = "HTTP/1.0 200
OK\r\nContent-Type: text/plain; version=2.0.0; "
-
"charset=utf-8\r\nContent-Encoding: gzip\r\nCache-Control: no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_V2_DEFLATE[] =
- "HTTP/1.0 200 OK\r\nContent-Type: text/plain; version=2.0.0;
charset=utf-8\r\nContent-Encoding: deflate\r\nCache-Control: "
- "no-cache\r\n\r\n";
-static const char RESP_HEADER_PROMETHEUS_V2_BR[] = "HTTP/1.0 200
OK\r\nContent-Type: text/plain; version=2.0.0; "
-
"charset=utf-8\r\nContent-Encoding: br\r\nCache-Control: no-cache\r\n\r\n";
-
-static int64_t
-stats_add_resp_header(stats_state *my_state)
+static std::string
+build_resp_header(output_format_t format, const cached_body &entry)
{
- switch (my_state->output_format) {
- case output_format_t::JSON_OUTPUT:
- if (my_state->encoding == encoding_format_t::GZIP) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_JSON_GZIP, my_state);
- } else if (my_state->encoding == encoding_format_t::DEFLATE) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_JSON_DEFLATE, my_state);
- } else if (my_state->encoding == encoding_format_t::BR) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_JSON_BR, my_state);
- } else {
- return stats_add_data_to_resp_buffer(RESP_HEADER_JSON, my_state);
- }
- break;
+ std::string header{"HTTP/1.0 200 OK\r\nContent-Type: "};
+
+ switch (format) {
case output_format_t::CSV_OUTPUT:
- if (my_state->encoding == encoding_format_t::GZIP) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_CSV_GZIP, my_state);
- } else if (my_state->encoding == encoding_format_t::DEFLATE) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_CSV_DEFLATE, my_state);
- } else if (my_state->encoding == encoding_format_t::BR) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_CSV_BR, my_state);
- } else {
- return stats_add_data_to_resp_buffer(RESP_HEADER_CSV, my_state);
- }
+ header += "text/csv";
break;
case output_format_t::PROMETHEUS_OUTPUT:
- if (my_state->encoding == encoding_format_t::GZIP) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_GZIP,
my_state);
- } else if (my_state->encoding == encoding_format_t::DEFLATE) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_DEFLATE,
my_state);
- } else if (my_state->encoding == encoding_format_t::BR) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_BR,
my_state);
- } else {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS, my_state);
- }
+ header += "text/plain; version=0.0.4; charset=utf-8";
break;
case output_format_t::PROMETHEUS_V2_OUTPUT:
- if (my_state->encoding == encoding_format_t::GZIP) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_V2_GZIP,
my_state);
- } else if (my_state->encoding == encoding_format_t::DEFLATE) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_V2_DEFLATE,
my_state);
- } else if (my_state->encoding == encoding_format_t::BR) {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_V2_BR,
my_state);
- } else {
- return stats_add_data_to_resp_buffer(RESP_HEADER_PROMETHEUS_V2,
my_state);
- }
+ header += "text/plain; version=2.0.0; charset=utf-8";
+ break;
+ default:
+ header += "text/json";
break;
}
- // Not reached.
- return stats_add_data_to_resp_buffer(RESP_HEADER_JSON, my_state);
+ header += "\r\n";
+
+ switch (entry.encoding) {
+ case encoding_format_t::GZIP:
+ header += "Content-Encoding: gzip\r\n";
+ break;
+ case encoding_format_t::DEFLATE:
+ header += "Content-Encoding: deflate\r\n";
+ break;
+ case encoding_format_t::BR:
+ header += "Content-Encoding: br\r\n";
+ break;
+ default:
+ break;
+ }
+
+ if (cache_ttl.count() > 0) {
+ auto age =
std::chrono::duration_cast<std::chrono::seconds>(std::chrono::steady_clock::now()
- entry.generated);
+
+ header += "Cache-Control: max-age=" + std::to_string(cache_ttl.count()) +
"\r\n";
+ header += "Age: " + std::to_string(age.count()) + "\r\n";
+ header += "Vary: Accept, Accept-Encoding\r\n";
+ } else {
+ header += "Cache-Control: no-cache\r\n";
+ }
+ header += "\r\n";
+
+ return header;
}
+static std::shared_ptr<const cached_body> get_body(stats_state *my_state);
+
static void
stats_process_read(TSCont contp, TSEvent event, stats_state *my_state)
{
Dbg(dbg_ctl, "stats_process_read(%d)", event);
if (event == TS_EVENT_VCONN_READ_READY) {
- my_state->output_bytes = stats_add_resp_header(my_state);
+ auto entry = get_body(my_state);
+ std::string header = build_resp_header(my_state->output_format, *entry);
+ int64_t nbytes = TSIOBufferWrite(my_state->resp_buffer, header.data(),
header.size());
+
+ nbytes += TSIOBufferWrite(my_state->resp_buffer, entry->body.data(),
entry->body.size());
Review Comment:
`TSIOBufferWrite` can return `TS_ERROR`, which would make `nbytes` negative
and get passed to `TSVConnWrite`. Please handle `TS_ERROR` explicitly (e.g.,
treat it as a failed response and cleanup, or compute `nbytes` from known sizes
/ `TSIOBufferReaderAvail` after writes and ensure it’s non-negative) before
calling `TSVConnWrite`.
##########
plugins/stats_over_http/stats_over_http.cc:
##########
@@ -901,94 +787,54 @@ json_out_stats(stats_state *my_state)
#if HAVE_BROTLI_ENCODE_H
static bool
-br_encode(stats_state *my_state, const char *data, int64_t len, bool finish,
TSIOBuffer out)
+br_compress(const std::string &in, std::string &out)
{
- BrotliEncoderOperation const op = finish ? BROTLI_OPERATION_FINISH :
BROTLI_OPERATION_PROCESS;
- const uint8_t *next_in = reinterpret_cast<const uint8_t
*>(data);
- size_t avail_in = static_cast<size_t>(len);
-
- do {
- int64_t avail = 0;
- uint8_t *next_out = reinterpret_cast<uint8_t
*>(TSIOBufferBlockWriteStart(TSIOBufferStart(out), &avail));
- size_t avail_out = static_cast<size_t>(avail);
-
- if (!BrotliEncoderCompressStream(my_state->bstrm.br, op, &avail_in,
&next_in, &avail_out, &next_out, nullptr)) {
- return false;
- }
- TSIOBufferProduce(out, avail - avail_out);
- } while (avail_in > 0 || BrotliEncoderHasMoreOutput(my_state->bstrm.br) ||
- (finish && !BrotliEncoderIsFinished(my_state->bstrm.br)));
+ size_t out_size = BrotliEncoderMaxCompressedSize(in.size());
+ if (out_size == 0) {
+ Dbg(dbg_ctl, "brotli input too large: %zu", in.size());
+ return false;
+ }
+ out.resize(out_size);
+ if (BrotliEncoderCompress(BROTLI_DEFAULT_QUALITY, BROTLI_DEFAULT_WINDOW,
BROTLI_DEFAULT_MODE, in.size(),
+ reinterpret_cast<const uint8_t *>(in.data()),
&out_size,
+ reinterpret_cast<uint8_t *>(out.data())) ==
BROTLI_FALSE) {
+ Dbg(dbg_ctl, "brotli compress error");
+ return false;
+ }
+ out.resize(out_size);
return true;
}
#endif
static bool
-gzip_encode(stats_state *my_state, const char *data, int64_t len, bool finish,
TSIOBuffer out)
+gzip_compress(const std::string &in, std::string &out, int mode)
{
- z_stream &zstrm = my_state->zstrm;
- int const flush = finish ? Z_FINISH : Z_NO_FLUSH;
- int err = Z_OK;
-
- zstrm.next_in = reinterpret_cast<Bytef *>(const_cast<char *>(data));
- zstrm.avail_in = static_cast<uInt>(len);
- do {
- int64_t avail = 0;
- zstrm.next_out = reinterpret_cast<Bytef
*>(TSIOBufferBlockWriteStart(TSIOBufferStart(out), &avail));
- zstrm.avail_out = static_cast<uInt>(avail);
- err = deflate(&zstrm, flush);
- TSIOBufferProduce(out, avail - zstrm.avail_out);
- } while (err == Z_OK && (finish || zstrm.avail_out == 0));
-
- return finish ? err == Z_STREAM_END : (err == Z_OK || err == Z_BUF_ERROR);
-}
+ z_stream zstrm{};
-static bool
-encode_chunk(stats_state *my_state, const char *data, int64_t len, bool
finish, TSIOBuffer out)
-{
-#if HAVE_BROTLI_ENCODE_H
- if (my_state->encoding == encoding_format_t::BR) {
- return br_encode(my_state, data, len, finish, out);
+ zstrm.data_type = Z_ASCII;
+ if (int err = deflateInit2(&zstrm, ZLIB_COMPRESSION_LEVEL, Z_DEFLATED, mode,
ZLIB_MEMLEVEL, Z_DEFAULT_STRATEGY); err != Z_OK) {
+ Dbg(dbg_ctl, "gzip initialization failed: %d", err);
+ return false;
}
-#endif
- return gzip_encode(my_state, data, len, finish, out);
-}
-// Replaces the uncompressed body in the response buffer with its compressed
form. The write VIO
-// has already sent the response header, so the body is all that remains in
resp_reader.
-static void
-compress_out_stats(stats_state *my_state)
-{
- TSIOBuffer out = TSIOBufferCreate();
- TSIOBufferReader out_reader = TSIOBufferReaderAlloc(out);
- int64_t const body_bytes = TSIOBufferReaderAvail(my_state->resp_reader);
- bool ok = true;
+ out.resize(deflateBound(&zstrm, in.size()));
+ zstrm.next_in = reinterpret_cast<Bytef *>(const_cast<char *>(in.data()));
+ zstrm.avail_in = in.size();
+ zstrm.next_out = reinterpret_cast<Bytef *>(out.data());
+ zstrm.avail_out = out.size();
Review Comment:
`z_stream::avail_in` is a `uInt`, but `in.size()` is `size_t`. For very
large stats bodies, this can overflow/truncate and cause incorrect compression
behavior. Please add an explicit size bound check (and fail compression
cleanly) when `in.size()` exceeds what zlib can accept, and similarly ensure
`deflateBound` is called with a value that won’t overflow the expected zlib
input type.
##########
tests/gold_tests/pluginTest/stats_over_http/stats_over_http.test.py:
##########
@@ -415,3 +415,84 @@ def run(self):
StatsOverHttpPluginTest().run()
+
+
+class StatsOverHttpCacheTtlTest:
+ """Verify that --cache-ttl serves a cached response until it goes stale."""
+
+ CACHE_TTL = 3
+
+ def __init__(self):
+ self.ts = Test.MakeATSProcess("ts_cache_ttl")
+ self.ts.Disk.plugin_config.AddLine(f'stats_over_http.so
--cache-ttl={self.CACHE_TTL} _stats')
+ self.ts.Disk.records_config.update(
+ {
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "stats_over_http",
+ })
+ self.url = f'http://127.0.0.1:{self.ts.Variables.port}/_stats'
Review Comment:
This test hard-codes requests to `self.ts.Variables.port`, but unlike
`StatsOverHttpPluginTest.__setupTS`, it doesn’t set
`proxy.config.http.server_ports` to ensure ATS actually listens on that port.
Aligning the setup with the existing test harness (by explicitly configuring
`proxy.config.http.server_ports`) would make the test more robust and
consistent with the rest of the file.
##########
plugins/stats_over_http/stats_over_http.cc:
##########
@@ -1039,32 +885,104 @@ prometheus_v2_out_stats(stats_state *my_state)
APPEND_STAT_PROMETHEUS_NUMERIC("current_time_epoch_ms", "%" PRIu64,
ms_since_epoch());
}
-static void
-stats_process_write(TSCont contp, TSEvent event, stats_state *my_state)
+static std::shared_ptr<const cached_body>
+generate_body(stats_state *my_state)
{
- if (event == TS_EVENT_VCONN_WRITE_READY) {
- if (my_state->body_written == 0) {
- my_state->body_written = 1;
- switch (my_state->output_format) {
- case output_format_t::JSON_OUTPUT:
- json_out_stats(my_state);
- break;
- case output_format_t::CSV_OUTPUT:
- csv_out_stats(my_state);
- break;
- case output_format_t::PROMETHEUS_OUTPUT:
- prometheus_out_stats(my_state);
- break;
- case output_format_t::PROMETHEUS_V2_OUTPUT:
- prometheus_v2_out_stats(my_state);
- break;
- }
+ auto entry = std::make_shared<cached_body>();
+ entry->generated = std::chrono::steady_clock::now();
+
+ switch (my_state->output_format) {
+ case output_format_t::CSV_OUTPUT:
+ csv_out_stats(my_state);
+ break;
+ case output_format_t::PROMETHEUS_OUTPUT:
+ prometheus_out_stats(my_state);
+ break;
+ case output_format_t::PROMETHEUS_V2_OUTPUT:
+ prometheus_v2_out_stats(my_state);
+ break;
+ default:
+ json_out_stats(my_state);
+ break;
+ }
+
+ bool compressed = false;
+
+ switch (my_state->encoding) {
+ case encoding_format_t::GZIP:
+ compressed = gzip_compress(my_state->body, entry->body, GZIP_MODE);
+ break;
+ case encoding_format_t::DEFLATE:
+ compressed = gzip_compress(my_state->body, entry->body, DEFLATE_MODE);
+ break;
+#if HAVE_BROTLI_ENCODE_H
+ case encoding_format_t::BR:
+ compressed = br_compress(my_state->body, entry->body);
+ break;
+#endif
+ default:
+ break;
+ }
+
+ if (compressed) {
+ entry->encoding = my_state->encoding;
+ } else {
+ entry->encoding = encoding_format_t::NONE;
+ entry->body = std::move(my_state->body);
+ }
+ my_state->body.clear();
+ my_state->prometheus_v2_families.clear();
+ my_state->prometheus_v2_family_order.clear();
+
+ return entry;
+}
+
+// Return a fresh cached body if there is one. When it is stale, one request
regenerates it while concurrent requests are
+// served the stale copy, provided it is less than twice the TTL old. Older
copies are not served; those requests regenerate.
+static std::shared_ptr<const cached_body>
+get_body(stats_state *my_state)
+{
+ if (cache_ttl.count() <= 0) {
+ return generate_body(my_state);
+ }
+
+ auto &slot =
body_cache[static_cast<size_t>(my_state->output_format)][static_cast<size_t>(my_state->encoding)];
+ bool owner = false;
+
+ {
+ std::lock_guard lock(body_cache_mutex);
- if (my_state->encoding != encoding_format_t::NONE) {
- compress_out_stats(my_state);
+ if (slot.entry) {
+ auto age = std::chrono::steady_clock::now() - slot.entry->generated;
+
+ if (age < cache_ttl || (slot.refreshing && age < 2 * cache_ttl)) {
+ return slot.entry;
}
- TSVIONBytesSet(my_state->write_vio, my_state->output_bytes);
}
+ if (!slot.refreshing) {
+ slot.refreshing = true;
+ owner = true;
+ }
+ }
Review Comment:
When `slot.entry` is null and one request has set `slot.refreshing=true`,
concurrent requests will still fall through and generate bodies in parallel
(because there’s no stale entry to return). This can cause a “thundering herd”
on cold start (or after eviction if an entry is cleared in the future),
negating the intended CPU savings. Consider adding a coordination mechanism so
non-owners wait for the refresh to complete when no cached entry exists (e.g.,
a `condition_variable` per slot, or a lightweight wait/reenable strategy),
while keeping the current “serve stale while refreshing” behavior when
`slot.entry` exists.
##########
tests/gold_tests/pluginTest/stats_over_http/stats_over_http.test.py:
##########
@@ -415,3 +415,84 @@ def run(self):
StatsOverHttpPluginTest().run()
+
+
+class StatsOverHttpCacheTtlTest:
+ """Verify that --cache-ttl serves a cached response until it goes stale."""
+
+ CACHE_TTL = 3
+
+ def __init__(self):
+ self.ts = Test.MakeATSProcess("ts_cache_ttl")
+ self.ts.Disk.plugin_config.AddLine(f'stats_over_http.so
--cache-ttl={self.CACHE_TTL} _stats')
+ self.ts.Disk.records_config.update(
+ {
+ "proxy.config.diags.debug.enabled": 1,
+ "proxy.config.diags.debug.tags": "stats_over_http",
+ })
+ self.url = f'http://127.0.0.1:{self.ts.Variables.port}/_stats'
+
+ def __epoch(self, curl_args: str = '') -> str:
+ return f"curl -s --http1.1 {curl_args} {self.url} | grep
current_time_epoch_ms"
+
+ def __testCacheHeaders(self):
+ tr = Test.AddTestRun('A cached stats response advertises its max-age
and age')
+ tr.Processes.Default.StartBefore(self.ts)
+ # Use CSV so this cache entry is not shared with the JSON entry the
fresh/stale runs depend on.
+ csv = "-H 'Accept: text/csv'"
+ tr.Processes.Default.Command = (
+ f'curl -s -o /dev/null --http1.1 {csv} {self.url} && sleep 2 && '
+ f'curl -vs -o /dev/null --http1.1 {csv} {self.url}')
+ tr.Processes.Default.ReturnCode = 0
+ tr.Processes.Default.Streams.stderr += Testers.ContainsExpression(
+ r'< Age: [1-9]', 'A cache hit should report the age of the cached
body.')
Review Comment:
This assertion is potentially flaky: it only matches a single digit age
(`1`–`9`). Under slow CI conditions (or if the first request/ATS is delayed),
the second request could legitimately observe `Age: 10`+ and fail the test.
Please widen the regex to accept multi-digit ages while still excluding 0
(e.g., `[1-9][0-9]*`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
