Copilot commented on code in PR #13168:
URL: https://github.com/apache/trafficserver/pull/13168#discussion_r3262513106
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -186,6 +188,15 @@ cccs Proxy Cache Cache collapsed connection success;
-1: collapsing was attempted but failed, request went
upstream
0: collapsing was unnecessary
1: attempted to collapse and got a cache hit on
subsequent read attempts
+
+cfl Proxy Cache Freshness limit (in seconds) for the cached object.
+ Written when an object is served from or written to
+ cache. Value is ``-1`` if not applicable.
+
+cca Proxy Cache Current age (in seconds) of the cached object at serve
+ time. Computed from internal state without reading
+ response headers. Value is ``-1`` if not applicable.
+
Review Comment:
The documentation says `cca` is computed without reading response headers,
but the implementation calls `HttpTransactCache::calculate_document_age()`,
which reads `Age` via `base_response->get_age()` and uses the cached response
`Date`. Please either adjust the implementation or document that parsed cached
response headers contribute to the value.
##########
src/proxy/logging/TransactionLogData.cc:
##########
@@ -949,6 +949,26 @@ TransactionLogData::get_max_cache_open_write_retries()
const
return -1;
}
+int
+TransactionLogData::get_freshness_limit() const
+{
+ if (likely(m_http_sm != nullptr)) {
+ return m_http_sm->t_state.cache_info.freshness_limit;
+ }
+ return -1;
+}
+
+int64_t
+TransactionLogData::get_current_age() const
+{
+ if (likely(m_http_sm != nullptr)) {
+ return static_cast<int64_t>(m_http_sm->t_state.cache_info.current_age);
+ }
+ return -1;
+}
+
+// ===== Retry attempts =====
+
// ===== Retry attempts =====
Review Comment:
This adds a second identical `// ===== Retry attempts =====` section header
immediately before the existing one, which makes the file structure noisier
without adding information.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -7583,10 +7583,13 @@ HttpTransact::what_is_document_freshness(State *s,
HTTPHdr *client_request, HTTP
response_date = cached_obj_response->get_date();
fresh_limit = calculate_document_freshness_limit(s, cached_obj_response,
response_date, &heuristic);
+ s->cache_info.freshness_limit = fresh_limit; // save for logging
ink_assert(fresh_limit >= 0);
current_age =
HttpTransactCache::calculate_document_age(s->request_sent_time,
s->response_received_time, cached_obj_response,
response_date,
s->current.now);
+
+ s->cache_info.current_age = current_age;
Review Comment:
These fields are only populated when `what_is_document_freshness()` reaches
this block. Cache writes never call `calculate_document_freshness_limit()`
(this is its only call site), and cache-hit paths can return earlier from this
function (for example ttl-in-cache or must-revalidate) before either value is
set, so `cfl`/`cca` will log `-1` for documented cache-write and some cache-hit
cases.
##########
src/proxy/logging/Log.cc:
##########
@@ -722,6 +722,16 @@ Log::init_fields()
global_field_list.add(field, false);
field_symbol_hash.emplace("chm", field);
+ field = new LogField("cache_freshness_limit", "cfl", LogField::sINT,
&LogAccess::marshal_cache_freshness_limit,
+ &LogAccess::unmarshal_int_to_str);
+ global_field_list.add(field, false);
+ field_symbol_hash.emplace("cfl", field);
+
+ field = new LogField("cache_current_age", "cca", LogField::sINT,
&LogAccess::marshal_cache_current_age,
+ &LogAccess::unmarshal_int_to_str);
Review Comment:
There is no automated coverage for the new `cfl`/`cca` log tags or their
cache-hit/cache-write semantics. Existing logging gold/unit tests cover other
log fields, so this should add a logging test that enables these fields and
verifies the expected values for at least cache hit and non-applicable paths.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -7583,10 +7583,13 @@ HttpTransact::what_is_document_freshness(State *s,
HTTPHdr *client_request, HTTP
response_date = cached_obj_response->get_date();
fresh_limit = calculate_document_freshness_limit(s, cached_obj_response,
response_date, &heuristic);
+ s->cache_info.freshness_limit = fresh_limit; // save for logging
ink_assert(fresh_limit >= 0);
current_age =
HttpTransactCache::calculate_document_age(s->request_sent_time,
s->response_received_time, cached_obj_response,
response_date,
s->current.now);
+
+ s->cache_info.current_age = current_age;
Review Comment:
These values are captured during freshness lookup, before any stale
revalidation. If a stale cached object is revalidated with a 304 and then
served from cache, the log records the pre-revalidation freshness/age rather
than the updated object state at serve time as documented.
##########
include/proxy/http/HttpTransact.h:
##########
@@ -490,6 +490,8 @@ class HttpTransact
HTTPInfo transform_store;
CacheDirectives directives;
HTTPInfo *object_read = nullptr;
+ int freshness_limit = -1; // seconds; -1 = not yet set
+ ink_time_t current_age = -1; // seconds; -1 =
not yet set
Review Comment:
These defaults only apply when the transaction state is constructed. The
same `cache_info` is reused across additional cache lookups in a transaction
(for example `HttpSM::do_cache_lookup_and_read()` resets `request_sent_time`,
`response_received_time`, and `cache_lookup_result`, but not these new fields),
so a later miss/skipped lookup can log the previous hit's freshness/age instead
of `-1`. Reset these values wherever a new cache lookup/cache state is started.
--
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]
