You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/07/21 20:12:29 UTC
[PR] Logging: add cache freshness fields [trafficserver]
bneradt opened a new pull request, #13417:
URL: https://github.com/apache/trafficserver/pull/13417
Cache modeling cannot currently observe an object’s effective
freshness limit or its age when ATS serves it, leaving access logs
unable to describe cache lifetime and churn.
This adds cfl and cca log fields and records their values when ATS
prepares cache metadata or serves a cached response. It resets the
values for each lookup and adds replay coverage for writes, hits, and
non-cacheable responses.
Fixes: #13127
Co-authored-by: sheikh-saifi <p2...@pwr.nu.edu.pk>
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#issuecomment-5039749360
> Thanks for fixing this! the reset in DecideCacheLookup and splitting the write vs hit call sites was exactly what my version was missing. Went through the code and the three test cases (write/hit/uncacheable) and it all makes sense. Sorry for disappearing after the first review, got pulled into other stuff and should've followed up. Happy to help test or review anything else on this.
Sounds good. Thanks for the comments. I definitely plan to attribute your work with the commit, in addition to the PR here.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3770143634
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3059,6 +3062,7 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
obj = s->cache_info.object_read;
}
cached_response = obj->response_get();
+ set_cache_freshness_info(s, cached_response, obj->request_sent_time_get(), obj->response_received_time_get(), true);
Review Comment:
`what_is_document_freshness()` computes the age for this same object from `s->request_sent_time`/`s->response_received_time`, which are clamped with `std::min(s->client_request_time, ...)` at lines 2687-2688 and 3372-3373, and it then clamps `current_age` into `cache_guaranteed_max_lifetime`. Here the object's raw times are used, so `cca` can be a different number than the age that decided FRESH vs STALE. Deliberate? If so it would help to say in the doc which one `cca` is.
Note this also runs with `obj = &s->cache_info.object_store` on the `api_update_cached_object == CONTINUE` branch above, where the object came from a plugin.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3908472308
##########
tests/gold_tests/logging/verify_cache_freshness_fields.py:
##########
@@ -0,0 +1,81 @@
+'''
+Validate the cache freshness fields produced by cache-freshness-fields.test.py.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pathlib
+import sys
+
+
+def load_log_entries(log_path: pathlib.Path) -> dict[str, tuple[int, int, str, str]]:
+ entries: dict[str, tuple[int, int, str, str]] = {}
+
+ for line in log_path.read_text(encoding='utf-8').splitlines():
+ uuid, freshness_limit, current_age, cache_result, cache_write_result = line.split()
+ entries[uuid] = (int(freshness_limit), int(current_age), cache_result, cache_write_result)
+
+ return entries
Review Comment:
Addressed in e9988d6c89. The verifier now reports the file and line for rows with the wrong field count, rejects duplicate UUIDs explicitly, and converts those validation errors into concise stderr diagnostics. Targeted malformed-row and duplicate assertions pass, as does the full `cache-freshness-fields` AuTest.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3857707436
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -191,6 +193,17 @@ 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 an object served from or
+ written to cache. The limit reflects parsed cache response
+ metadata and the effective cache configuration. The value
Review Comment:
Updated in 2460007d79. The documentation now says the limit is calculated from response headers and the configured guaranteed/heuristic lifetime bounds, without implying that directives such as or are included.
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -191,6 +193,17 @@ 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 an object served from or
+ written to cache. The limit reflects parsed cache response
+ metadata and the effective cache configuration. The value
Review Comment:
Updated in 2460007d79. The documentation now says the limit is calculated from response headers and the configured guaranteed/heuristic lifetime bounds, without implying that `cache.config` directives such as `ttl-in-cache` or `revalidate-after` are included.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3059,6 +3062,7 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
obj = s->cache_info.object_read;
}
cached_response = obj->response_get();
+ set_cache_freshness_info(s, cached_response, obj->request_sent_time_get(), obj->response_received_time_get(), true);
Review Comment:
Fixed in 2460007d79. `build_response_from_cache()` now passes `s->request_sent_time` and `s->response_received_time`, so `cca` uses the same capped transaction timestamps as freshness evaluation and the Age header. This also avoids zero timestamps from `object_store` on the modifiable cached-response path.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -4835,6 +4839,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// unset warning revalidation failed header if it set
// (potentially added by negative revalidating)
delete_warning_value(base_response, HTTPWarningCode::REVALIDATION_FAILED);
+ set_cache_freshness_info(s, base_response, s->request_sent_time, s->response_received_time, true);
Review Comment:
Fixed in 2460007d79. The negative-revalidation early-return path now calls `set_cache_freshness_info()` after extending the cached response. I also added a replay regression that exercises a `TCP_REFRESH_FAIL_HIT` and verifies that `cfl` and `cca` are populated.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3770143634
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3059,6 +3062,7 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
obj = s->cache_info.object_read;
}
cached_response = obj->response_get();
+ set_cache_freshness_info(s, cached_response, obj->request_sent_time_get(), obj->response_received_time_get(), true);
Review Comment:
`what_is_document_freshness()` computes the age for this same object from `s->request_sent_time`/`s->response_received_time`, which are clamped with `std::min(s->client_request_time, ...)` at lines 2687-2688 and 3372-3373, and it then clamps `current_age` into `cache_guaranteed_max_lifetime`. Here the object's raw times are used, so `cca` can be a different number than the age that decided FRESH vs STALE. Deliberate? If so it would help to say in the doc which one `cca` is.
Note this also runs with `obj = &s->cache_info.object_store` on the `api_update_cached_object == CONTINUE` branch above, where the object came from a plugin.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3770551673
##########
doc/admin-guide/logging/formatting.en.rst:
##########
@@ -191,6 +193,17 @@ 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 an object served from or
+ written to cache. The limit reflects parsed cache response
+ metadata and the effective cache configuration. The value
Review Comment:
"the effective cache configuration" is broader than what `calculate_document_freshness_limit()` currently includes.
The calculation uses the response headers plus `guaranteed_min/max_lifetime` and `heuristic_min/max_lifetime`, but not `s->cache_control`. That excludes `cache.config` directives such as `ttl-in-cache` and `revalidate-after`.
For example, with `ttl-in-cache=300` and an origin response containing `max-age=1`, a hit at age 3 logs `cfl=1 cca=3`.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -3059,6 +3062,7 @@ HttpTransact::build_response_from_cache(State *s, HTTPWarningCode warning_code)
obj = s->cache_info.object_read;
}
cached_response = obj->response_get();
+ set_cache_freshness_info(s, cached_response, obj->request_sent_time_get(), obj->response_received_time_get(), true);
Review Comment:
These should use `s->request_sent_time` and `s->response_received_time` rather than the object's raw timestamps.
`what_is_document_freshness()` uses the transaction timestamps, which are capped against `s->client_request_time`. The `Age` header sent to the client uses those values as well. Using the object's raw timestamps here can therefore make `cca` differ from both the freshness decision and the response's `Age` header.
There is also a problem on the `api_update_cached_object == CONTINUE` path. There, `obj` is `&s->cache_info.object_store`, whose timestamps can still be zero after a plugin creates it with `TSHttpTxnCachedRespModifiableGet()`. Passing those values to `calculate_document_age()` makes `cca` roughly the current Unix timestamp.
Passing `s->request_sent_time` and `s->response_received_time` fixes both cases.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -4835,6 +4839,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
// unset warning revalidation failed header if it set
// (potentially added by negative revalidating)
delete_warning_value(base_response, HTTPWarningCode::REVALIDATION_FAILED);
+ set_cache_freshness_info(s, base_response, s->request_sent_time, s->response_received_time, true);
Review Comment:
This call is skipped by the negative revalidation path earlier in the function. That branch sets `action = SERVE_AND_UPDATE` and returns before reaching here.
Those transactions serve the stale cached object and extend its stored `Expires`, but leave both freshness fields at -1. Negative revalidation is enabled by default for 500, 502, 503, and 504 responses. There is a reproducer in the review summary.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#issuecomment-5500318690
Addressed the remaining review issue and force-pushed `18e63ce1c8`.
Write-side freshness limits are now kept pending separately for the original and transformed cache objects. `cfl` is published only after `tunnel_handler_cache_write()` reports a successful write; failures discard the pending value without overwriting freshness information for a cached object already selected for serving. If both object variants succeed, the transformed object retains precedence.
I also added deterministic failed-write coverage by exceeding `proxy.config.cache.max_doc_size`. The replay verifies that the write result is `ERR` and both `cfl` and `cca` remain `-1`; the existing successful-write, hit, negative-revalidation, and non-cacheable cases still pass.
Validation in `asfats5`:
- Full configure, format, build, and install: passed.
- Focused `cache-freshness-fields` AuTest: passed.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "sheikh-saifi (via GitHub)" <gi...@apache.org>.
sheikh-saifi commented on PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#issuecomment-5039227448
Thanks for fixing this! The reset in DecideCacheLookup and splitting the write vs hit call sites was exactly what my version was missing. Went through the code and the three test cases (write/hit/uncacheable) and it all makes sense.
Sorry for disappearing after the first review, got pulled into other stuff and should've followed up. Happy to help test/review anything else on this.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#issuecomment-5671646325
[approve ci autest 2]
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Logging: add cache freshness fields [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13417:
URL: https://github.com/apache/trafficserver/pull/13417#discussion_r3908413047
##########
tests/gold_tests/logging/verify_cache_freshness_fields.py:
##########
@@ -0,0 +1,81 @@
+'''
+Validate the cache freshness fields produced by cache-freshness-fields.test.py.
+'''
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import pathlib
+import sys
+
+
+def load_log_entries(log_path: pathlib.Path) -> dict[str, tuple[int, int, str, str]]:
+ entries: dict[str, tuple[int, int, str, str]] = {}
+
+ for line in log_path.read_text(encoding='utf-8').splitlines():
+ uuid, freshness_limit, current_age, cache_result, cache_write_result = line.split()
+ entries[uuid] = (int(freshness_limit), int(current_age), cache_result, cache_write_result)
+
+ return entries
Review Comment:
`load_log_entries()` silently overwrites entries if the log contains the same uuid more than once, which can make the test pass even when extra/unexpected log lines are present. It also raises a generic unpacking exception if a line doesn't have exactly 5 fields, which is harder to diagnose from CI logs. Consider validating the field count and rejecting duplicate uuids with a clear error.
This issue also appears on line 34 of the same file.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org