From 138fc94c05256b35c2f9f74bed88c24a2bb4aef0 Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 04:41:51 -0700 Subject: [PATCH 1/9] Sketch out the shape of what I want --- src/workerd/api/cache.c++ | 37 +++++++++++++++++++++++++++++++++++-- src/workerd/api/cache.h | 3 +++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/workerd/api/cache.c++ b/src/workerd/api/cache.c++ index 8245e1f0d91..c009ce24929 100644 --- a/src/workerd/api/cache.c++ +++ b/src/workerd/api/cache.c++ @@ -76,6 +76,8 @@ jsg::Promise>> Cache::match(jsg::Lock& js, CompatibilityFlags::Reader flags) { // TODO(someday): Implement Cache API in preview. auto& context = IoContext::current(); + // TODO start span here + // TODO add options to span if (context.isFiddle()) { context.logWarningOnce(CACHE_API_PREVIEW_WARNING); return js.resolvedPromise(jsg::Optional>()); @@ -86,6 +88,8 @@ jsg::Promise>> Cache::match(jsg::Lock& js, return js.evalNow([&]() -> jsg::Promise>> { auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); + // add jsRequest->getUrl() to span + if (!options.orDefault({}).ignoreMethod.orDefault(false) && jsRequest->getMethodEnum() != kj::HttpMethod::GET) { return js.resolvedPromise(jsg::Optional>()); @@ -95,6 +99,9 @@ jsg::Promise>> Cache::match(jsg::Lock& js, jsRequest->getUrl(), kj::none, flags.getCacheApiCompatFlags()); auto requestHeaders = kj::HttpHeaders(context.getHeaderTable()); jsRequest->shallowCopyHeadersTo(requestHeaders); + + // parse each of the request headers to add info to span + requestHeaders.setPtr(context.getHeaderIds().cacheControl, "only-if-cached"); auto nativeRequest = httpClient->request( kj::HttpMethod::GET, validateUrl(jsRequest->getUrl()), requestHeaders, uint64_t(0)); @@ -117,6 +124,8 @@ jsg::Promise>> Cache::match(jsg::Lock& js, return kj::none; } + // TODO add cacheStatus to span + // The status code should be a 504 on cache miss, but we need to rely on CF-Cache-Status // because someone might cache a 504. // See https://httpwg.org/specs/rfc7234.html#cache-request-directive.only-if-cached @@ -244,6 +253,12 @@ jsg::Promise Cache::put(jsg::Lock& js, return js.evalNow([&] { auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); + auto& context = IoContext::current(); + // TODO start span here + // TODO add jsResponse->getStatus() to span + // TODO add response method + // TODO parse each of the headers + // TODO(conform): Require that jsRequest's url has an http or https scheme. This is only // important if api::Request is changed to parse its URL eagerly (as required by spec), rather // than at fetch()-time. @@ -262,8 +277,6 @@ jsg::Promise Cache::put(jsg::Lock& js, "Cannot cache response with 'Vary: *' header."); } - auto& context = IoContext::current(); - if (jsResponse->getStatus() == 304) { // Silently discard 304 status responses to conditional requests. Caching 304s could be a // source of bugs in a worker, since a worker which blindly stuffs responses from `fetch()` @@ -291,6 +304,8 @@ jsg::Promise Cache::put(jsg::Lock& js, auto serializePromise = jsResponse->send(js, serializer, {}, kj::none); auto payload = serializer.getPayload(); + // TODO get the payload size from payload.stream, add to span + // TODO(someday): Implement Cache API in preview. This bail-out lives all the way down here, // after all KJ_REQUIRE checks and the start of response serialization, so that Cache.put() // fulfills its contract, even in the preview. This prevents buggy code from working in the @@ -593,6 +608,24 @@ kj::Own Cache::getHttpClient(IoContext& context, return httpClient; } +kj::Own Cache::getHttpClientNew( + IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags) { + auto cacheClient = context.getCacheClient(); + auto metadata = CacheClient::SubrequestMetadata{ + .cfBlobJson = kj::mv(cfBlobJson), + .parentSpan = span, + .featureFlagsForFl = kj::none, + }; + if (enableCompatFlags) { + metadata.featureFlagsForFl = context.getWorker().getIsolate().getFeatureFlagsForFl(); + } + auto httpClient = + cacheName.map([&](kj::String& n) { + return cacheClient->getNamespace(n, kj::mv(metadata)); + }).orDefault([&]() { return cacheClient->getDefault(kj::mv(metadata)); }); + return httpClient; +} + // ======================================================================================= // CacheStorage diff --git a/src/workerd/api/cache.h b/src/workerd/api/cache.h index 64a14f5a1ed..9631d9e18be 100644 --- a/src/workerd/api/cache.h +++ b/src/workerd/api/cache.h @@ -95,6 +95,9 @@ class Cache: public jsg::Object { kj::StringPtr url, kj::Maybe cacheControl, bool enableCompatFlags); + + kj::Own getHttpClientNew( + IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags); }; // ======================================================================================= From 4890a33c9a1b33f9e2292df21ff3b42b9b5a2d9b Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 05:14:39 -0700 Subject: [PATCH 2/9] Claude sketch --- src/workerd/api/BUILD.bazel | 10 ++ src/workerd/api/cache-instrumentation-test.js | 109 ++++++++++++++++++ src/workerd/api/cache-test.js | 88 ++++++++++++++ src/workerd/api/cache-test.wd-test | 30 +++++ src/workerd/api/cache.c++ | 33 +++--- src/workerd/api/cache.h | 4 +- .../api/tests/instrumentation-tail-worker.js | 1 + 7 files changed, 256 insertions(+), 19 deletions(-) create mode 100644 src/workerd/api/cache-instrumentation-test.js create mode 100644 src/workerd/api/cache-test.js create mode 100644 src/workerd/api/cache-test.wd-test diff --git a/src/workerd/api/BUILD.bazel b/src/workerd/api/BUILD.bazel index f02b4a6c72d..423205fd658 100644 --- a/src/workerd/api/BUILD.bazel +++ b/src/workerd/api/BUILD.bazel @@ -428,6 +428,16 @@ wd_test( args = ["--experimental"], ) +wd_test( + src = "cache-test.wd-test", + args = ["--experimental"], + data = [ + "cache-instrumentation-test.js", + "cache-test.js", + "tests/instrumentation-tail-worker.js", + ], +) + wd_test( src = "kv-test.wd-test", args = ["--experimental"], diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js new file mode 100644 index 00000000000..c2eff8afc42 --- /dev/null +++ b/src/workerd/api/cache-instrumentation-test.js @@ -0,0 +1,109 @@ +// Copyright (c) 2017-2023 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 +import * as assert from 'node:assert'; +import { + invocationPromises, + spans, + testTailHandler, +} from 'test:instumentation-tail'; + +// Use shared instrumentation test tail worker +export default testTailHandler; + +export const test = { + async test() { + // Wait for all the tailStream executions to finish + await Promise.allSettled(invocationPromises); + + // Recorded streaming tail worker events, in insertion order, + // filtering spans not associated with Cache + let received = Array.from(spans.values()).filter( + (span) => span.name !== 'jsRpcSession' + ); + + // spans emitted by cache-test.js in execution order + let expected = [ + // cache.match() - HIT case + { + name: 'cache_match', + 'url.full': 'https://example.com/cached-resource', + closed: true, + }, + // cache.match() - MISS case + { + name: 'cache_match', + 'url.full': 'https://example.com/not-cached', + closed: true, + }, + // cache.match() with options + { + name: 'cache_match', + 'url.full': 'https://example.com/cached-with-options', + closed: true, + }, + // cache.put() with max-age + { + name: 'cache_put', + 'url.full': 'https://example.com/put-resource', + 'cache_control.cacheability': 'public', + 'cache_control.expiration': 'max-age=3600', + closed: true, + }, + // cache.put() with no-store + { + name: 'cache_put', + 'url.full': 'https://example.com/put-no-store', + 'cache_control.cacheability': 'no-store', + closed: true, + }, + // cache.put() with s-maxage + { + name: 'cache_put', + 'url.full': 'https://example.com/put-s-maxage', + 'cache_control.cacheability': 'public', + 'cache_control.expiration': 's-maxage=7200', + closed: true, + }, + // cache.delete() - exists + { + name: 'cache_delete', + 'url.full': 'https://example.com/delete-exists', + closed: true, + }, + // cache.delete() - doesn't exist + { + name: 'cache_delete', + 'url.full': 'https://example.com/delete-not-exists', + closed: true, + }, + // cache.delete() with options + { + name: 'cache_delete', + 'url.full': 'https://example.com/delete-with-options', + closed: true, + }, + ]; + + console.log(received); + assert.fail('test'); + // assert.equal( + // received.length, + // expected.length, + // `Expected ${expected.length} received ${received.length} spans` + // ); + // let errors = []; + // for (let i = 0; i < received.length; i++) { + // try { + // assert.deepStrictEqual(received[i], expected[i]); + // } catch (e) { + // console.error(`value: ${i} does not match`); + // console.log(e); + // errors.push(e); + // } + // } + // if (errors.length > 0) { + // throw 'cache spans are incorrect'; + // } + }, +}; diff --git a/src/workerd/api/cache-test.js b/src/workerd/api/cache-test.js new file mode 100644 index 00000000000..dd0a9d5cb96 --- /dev/null +++ b/src/workerd/api/cache-test.js @@ -0,0 +1,88 @@ +// Copyright (c) 2017-2023 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +import assert from 'node:assert'; + +export default { + async fetch(request) { + const cache = caches.default; + + // Test cache.match() - HIT case + const matchRequest1 = new Request('https://example.com/cached-resource', { + method: 'GET', + }); + let matchResult = await cache.match(matchRequest1); + + // Test cache.match() - MISS case + const matchRequest2 = new Request('https://example.com/not-cached', { + method: 'GET', + }); + matchResult = await cache.match(matchRequest2); + + // Test cache.match() with options + const matchRequest3 = new Request('https://example.com/cached-with-options', { + method: 'GET', + }); + matchResult = await cache.match(matchRequest3, { ignoreMethod: false }); + + // Test cache.put() + const putRequest1 = new Request('https://example.com/put-resource', { + method: 'GET', + }); + const putResponse1 = new Response('Test content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600', + }, + }); + await cache.put(putRequest1, putResponse1); + + // Test cache.put() with different cache control + const putRequest2 = new Request('https://example.com/put-no-store', { + method: 'GET', + }); + const putResponse2 = new Response('No store content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'no-store', + }, + }); + await cache.put(putRequest2, putResponse2); + + // Test cache.put() with s-maxage + const putRequest3 = new Request('https://example.com/put-s-maxage', { + method: 'GET', + }); + const putResponse3 = new Response('S-maxage content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 's-maxage=7200, public', + }, + }); + await cache.put(putRequest3, putResponse3); + + // Test cache.delete() - exists + const deleteRequest1 = new Request('https://example.com/delete-exists', { + method: 'GET', + }); + let deleteResult = await cache.delete(deleteRequest1); + + // Test cache.delete() - doesn't exist + const deleteRequest2 = new Request('https://example.com/delete-not-exists', { + method: 'GET', + }); + deleteResult = await cache.delete(deleteRequest2); + + // Test cache.delete() with options + const deleteRequest3 = new Request('https://example.com/delete-with-options', { + method: 'POST', + }); + deleteResult = await cache.delete(deleteRequest3, { ignoreMethod: true }); + + return new Response('Cache tests completed'); + }, +}; diff --git a/src/workerd/api/cache-test.wd-test b/src/workerd/api/cache-test.wd-test new file mode 100644 index 00000000000..17d6ea9a20f --- /dev/null +++ b/src/workerd/api/cache-test.wd-test @@ -0,0 +1,30 @@ +using Workerd = import "/workerd/workerd.capnp"; + +const unitTests :Workerd.Config = ( + services = [ + ( name = "cache-test", + worker = ( + modules = [ + ( name = "worker", esModule = embed "cache-test.js" ) + ], + compatibilityDate = "2023-07-24", + compatibilityFlags = ["experimental", "nodejs_compat", "service_binding_extra_handlers", "streaming_tail_worker"], + streamingTails = ["tail"], + ) + ), + (name = "tail", worker = .tailWorker, ), + ], + extensions = [ ( + modules = [ + ( name = "test:instumentation-tail", esModule = embed "tests/instrumentation-tail-worker.js" ), + ] + ) ] +); + +const tailWorker :Workerd.Worker = ( + modules = [ + (name = "worker", esModule = embed "cache-instrumentation-test.js") + ], + compatibilityDate = "2024-10-14", + compatibilityFlags = ["experimental", "nodejs_compat"], +); diff --git a/src/workerd/api/cache.c++ b/src/workerd/api/cache.c++ index c009ce24929..1780f185bf8 100644 --- a/src/workerd/api/cache.c++ +++ b/src/workerd/api/cache.c++ @@ -608,23 +608,22 @@ kj::Own Cache::getHttpClient(IoContext& context, return httpClient; } -kj::Own Cache::getHttpClientNew( - IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags) { - auto cacheClient = context.getCacheClient(); - auto metadata = CacheClient::SubrequestMetadata{ - .cfBlobJson = kj::mv(cfBlobJson), - .parentSpan = span, - .featureFlagsForFl = kj::none, - }; - if (enableCompatFlags) { - metadata.featureFlagsForFl = context.getWorker().getIsolate().getFeatureFlagsForFl(); - } - auto httpClient = - cacheName.map([&](kj::String& n) { - return cacheClient->getNamespace(n, kj::mv(metadata)); - }).orDefault([&]() { return cacheClient->getDefault(kj::mv(metadata)); }); - return httpClient; -} +// kj::Own Cache::getHttpClientNew( +// IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags) { +// auto cacheClient = context.getCacheClient(); +// auto metadata = CacheClient::SubrequestMetadata{ +// .cfBlobJson = kj::mv(cfBlobJson), +// .featureFlagsForFl = kj::none, +// }; +// if (enableCompatFlags) { +// metadata.featureFlagsForFl = context.getWorker().getIsolate().getFeatureFlagsForFl(); +// } +// auto httpClient = +// cacheName.map([&](kj::String& n) { +// return cacheClient->getNamespace(n, kj::mv(metadata)); +// }).orDefault([&]() { return cacheClient->getDefault(kj::mv(metadata)); }); +// return httpClient; +// } // ======================================================================================= // CacheStorage diff --git a/src/workerd/api/cache.h b/src/workerd/api/cache.h index 9631d9e18be..3cdf658af60 100644 --- a/src/workerd/api/cache.h +++ b/src/workerd/api/cache.h @@ -96,8 +96,8 @@ class Cache: public jsg::Object { kj::Maybe cacheControl, bool enableCompatFlags); - kj::Own getHttpClientNew( - IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags); + // kj::Own getHttpClientNew( + // IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags); }; // ======================================================================================= diff --git a/src/workerd/api/tests/instrumentation-tail-worker.js b/src/workerd/api/tests/instrumentation-tail-worker.js index 0b28f068c52..ab5e3ccc6a7 100644 --- a/src/workerd/api/tests/instrumentation-tail-worker.js +++ b/src/workerd/api/tests/instrumentation-tail-worker.js @@ -9,6 +9,7 @@ export const testTailHandler = { tailStream(event, env, ctx) { // Capture the top-level span ID from the onset event const topLevelSpanId = event.event.spanId; + console.log(event); // For each "onset" event, store a promise which we will resolve when // we receive the equivalent "outcome" event From 97a76046d3173b977bf2e33dff4d30f897b5d3a9 Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 06:22:21 -0700 Subject: [PATCH 3/9] Tests passing. Need to review llm generated code --- src/workerd/api/BUILD.bazel | 1 + src/workerd/api/cache-instrumentation-test.js | 78 ++++++++----------- src/workerd/api/cache-mock.js | 48 ++++++++++++ src/workerd/api/cache-test.js | 70 +++++++++++++---- src/workerd/api/cache-test.wd-test | 11 ++- 5 files changed, 146 insertions(+), 62 deletions(-) create mode 100644 src/workerd/api/cache-mock.js diff --git a/src/workerd/api/BUILD.bazel b/src/workerd/api/BUILD.bazel index 423205fd658..eccc6ad9f7c 100644 --- a/src/workerd/api/BUILD.bazel +++ b/src/workerd/api/BUILD.bazel @@ -433,6 +433,7 @@ wd_test( args = ["--experimental"], data = [ "cache-instrumentation-test.js", + "cache-mock.js", "cache-test.js", "tests/instrumentation-tail-worker.js", ], diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js index c2eff8afc42..2ca74c18925 100644 --- a/src/workerd/api/cache-instrumentation-test.js +++ b/src/workerd/api/cache-instrumentation-test.js @@ -24,25 +24,36 @@ export const test = { // spans emitted by cache-test.js in execution order let expected = [ - // cache.match() - HIT case + { + name: 'cache_delete', + 'url.full': 'https://example.com/delete-exists', + closed: true, + }, + { + name: 'cache_delete', + 'url.full': 'https://example.com/delete-not-exists', + closed: true, + }, + { + name: 'cache_delete', + 'url.full': 'https://example.com/delete-with-options', + closed: true, + }, { name: 'cache_match', 'url.full': 'https://example.com/cached-resource', closed: true, }, - // cache.match() - MISS case { name: 'cache_match', 'url.full': 'https://example.com/not-cached', closed: true, }, - // cache.match() with options { name: 'cache_match', 'url.full': 'https://example.com/cached-with-options', closed: true, }, - // cache.put() with max-age { name: 'cache_put', 'url.full': 'https://example.com/put-resource', @@ -50,60 +61,37 @@ export const test = { 'cache_control.expiration': 'max-age=3600', closed: true, }, - // cache.put() with no-store { name: 'cache_put', 'url.full': 'https://example.com/put-no-store', 'cache_control.cacheability': 'no-store', closed: true, }, - // cache.put() with s-maxage { name: 'cache_put', 'url.full': 'https://example.com/put-s-maxage', 'cache_control.cacheability': 'public', - 'cache_control.expiration': 's-maxage=7200', - closed: true, - }, - // cache.delete() - exists - { - name: 'cache_delete', - 'url.full': 'https://example.com/delete-exists', - closed: true, - }, - // cache.delete() - doesn't exist - { - name: 'cache_delete', - 'url.full': 'https://example.com/delete-not-exists', - closed: true, - }, - // cache.delete() with options - { - name: 'cache_delete', - 'url.full': 'https://example.com/delete-with-options', closed: true, }, ]; - console.log(received); - assert.fail('test'); - // assert.equal( - // received.length, - // expected.length, - // `Expected ${expected.length} received ${received.length} spans` - // ); - // let errors = []; - // for (let i = 0; i < received.length; i++) { - // try { - // assert.deepStrictEqual(received[i], expected[i]); - // } catch (e) { - // console.error(`value: ${i} does not match`); - // console.log(e); - // errors.push(e); - // } - // } - // if (errors.length > 0) { - // throw 'cache spans are incorrect'; - // } + assert.equal( + received.length, + expected.length, + `Expected ${expected.length} received ${received.length} spans` + ); + let errors = []; + for (let i = 0; i < received.length; i++) { + try { + assert.deepStrictEqual(received[i], expected[i]); + } catch (e) { + console.error(`value: ${i} does not match`); + console.log(e); + errors.push(e); + } + } + if (errors.length > 0) { + throw 'cache spans are incorrect'; + } }, }; diff --git a/src/workerd/api/cache-mock.js b/src/workerd/api/cache-mock.js new file mode 100644 index 00000000000..1f994072b40 --- /dev/null +++ b/src/workerd/api/cache-mock.js @@ -0,0 +1,48 @@ +// Copyright (c) 2017-2023 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// Mock cache backend - responds to GET (match), PUT (put), and PURGE (delete) +export default { + async fetch(request) { + const url = new URL(request.url); + const headers = new Headers(); + + // Handle cache.match() operations (GET requests) + if (request.method === 'GET') { + // Check if this is a cache.match() request (has only-if-cached) + const cacheControl = request.headers.get('cache-control'); + if (cacheControl?.includes('only-if-cached')) { + // Simulate cache HIT + if (url.pathname.includes('cached-resource')) { + headers.set('CF-Cache-Status', 'HIT'); + return new Response('Cached content', { status: 200, headers }); + } + // Simulate cache MISS + headers.set('CF-Cache-Status', 'MISS'); + return new Response(null, { status: 504, headers }); + } + } + + // Handle cache.put() operations (PUT requests) + if (request.method === 'PUT') { + // Read the body (which contains the serialized response to cache) + await request.text(); + + // Simulate successful cache write + return new Response(null, { status: 204 }); + } + + // Handle cache.delete() operations (PURGE requests) + if (request.method === 'PURGE') { + // Simulate successful deletion + if (url.pathname.includes('delete-exists')) { + return new Response(null, { status: 200 }); + } + // Simulate not found + return new Response(null, { status: 404 }); + } + + return new Response('Not Found', { status: 404 }); + }, +}; diff --git a/src/workerd/api/cache-test.js b/src/workerd/api/cache-test.js index dd0a9d5cb96..6f0061853ee 100644 --- a/src/workerd/api/cache-test.js +++ b/src/workerd/api/cache-test.js @@ -2,10 +2,10 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -import assert from 'node:assert'; +import * as assert from 'node:assert'; -export default { - async fetch(request) { +export const matchTest = { + async test(ctrl, env, ctx) { const cache = caches.default; // Test cache.match() - HIT case @@ -13,20 +13,37 @@ export default { method: 'GET', }); let matchResult = await cache.match(matchRequest1); + assert.ok(matchResult !== undefined, 'Should have a cache HIT'); + assert.strictEqual(await matchResult.text(), 'Cached content'); // Test cache.match() - MISS case const matchRequest2 = new Request('https://example.com/not-cached', { method: 'GET', }); matchResult = await cache.match(matchRequest2); + assert.strictEqual(matchResult, undefined, 'Should be cache MISS'); // Test cache.match() with options - const matchRequest3 = new Request('https://example.com/cached-with-options', { - method: 'GET', - }); + const matchRequest3 = new Request( + 'https://example.com/cached-with-options', + { + method: 'GET', + } + ); matchResult = await cache.match(matchRequest3, { ignoreMethod: false }); + assert.strictEqual( + matchResult, + undefined, + 'Should be cache MISS with options' + ); + }, +}; - // Test cache.put() +export const putTest = { + async test(ctrl, env, ctx) { + const cache = caches.default; + + // Test cache.put() with max-age const putRequest1 = new Request('https://example.com/put-resource', { method: 'GET', }); @@ -39,7 +56,7 @@ export default { }); await cache.put(putRequest1, putResponse1); - // Test cache.put() with different cache control + // Test cache.put() with no-store const putRequest2 = new Request('https://example.com/put-no-store', { method: 'GET', }); @@ -64,25 +81,46 @@ export default { }, }); await cache.put(putRequest3, putResponse3); + }, +}; + +export const deleteTest = { + async test(ctrl, env, ctx) { + const cache = caches.default; // Test cache.delete() - exists const deleteRequest1 = new Request('https://example.com/delete-exists', { method: 'GET', }); let deleteResult = await cache.delete(deleteRequest1); + assert.strictEqual( + deleteResult, + true, + 'Should return true for successful delete' + ); // Test cache.delete() - doesn't exist - const deleteRequest2 = new Request('https://example.com/delete-not-exists', { - method: 'GET', - }); + const deleteRequest2 = new Request( + 'https://example.com/delete-not-exists', + { + method: 'GET', + } + ); deleteResult = await cache.delete(deleteRequest2); + assert.strictEqual( + deleteResult, + false, + 'Should return false when not found' + ); // Test cache.delete() with options - const deleteRequest3 = new Request('https://example.com/delete-with-options', { - method: 'POST', - }); + const deleteRequest3 = new Request( + 'https://example.com/delete-with-options', + { + method: 'POST', + } + ); deleteResult = await cache.delete(deleteRequest3, { ignoreMethod: true }); - - return new Response('Cache tests completed'); + assert.strictEqual(deleteResult, false, 'Should handle options'); }, }; diff --git a/src/workerd/api/cache-test.wd-test b/src/workerd/api/cache-test.wd-test index 17d6ea9a20f..5d0d93a8926 100644 --- a/src/workerd/api/cache-test.wd-test +++ b/src/workerd/api/cache-test.wd-test @@ -7,11 +7,20 @@ const unitTests :Workerd.Config = ( modules = [ ( name = "worker", esModule = embed "cache-test.js" ) ], + cacheApiOutbound = "cache-backend", compatibilityDate = "2023-07-24", - compatibilityFlags = ["experimental", "nodejs_compat", "service_binding_extra_handlers", "streaming_tail_worker"], + compatibilityFlags = ["nodejs_compat", "streaming_tail_worker"], streamingTails = ["tail"], ) ), + ( name = "cache-backend", + worker = ( + modules = [ + ( name = "worker", esModule = embed "cache-mock.js" ) + ], + compatibilityDate = "2023-07-24", + ) + ), (name = "tail", worker = .tailWorker, ), ], extensions = [ ( From 510eed5333d4e1ffd06e172ed7e60628bdd08b2a Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 09:45:05 -0700 Subject: [PATCH 4/9] WIP --- src/workerd/api/BUILD.bazel | 2 +- src/workerd/api/cache-instrumentation-test.js | 81 ++++- src/workerd/api/cache-operations.js | 306 ++++++++++++++++++ src/workerd/api/cache-test.js | 126 -------- src/workerd/api/cache-test.wd-test | 2 +- 5 files changed, 386 insertions(+), 131 deletions(-) create mode 100644 src/workerd/api/cache-operations.js delete mode 100644 src/workerd/api/cache-test.js diff --git a/src/workerd/api/BUILD.bazel b/src/workerd/api/BUILD.bazel index eccc6ad9f7c..f3cc57fb6ed 100644 --- a/src/workerd/api/BUILD.bazel +++ b/src/workerd/api/BUILD.bazel @@ -434,7 +434,7 @@ wd_test( data = [ "cache-instrumentation-test.js", "cache-mock.js", - "cache-test.js", + "cache-operations.js", "tests/instrumentation-tail-worker.js", ], ) diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js index 2ca74c18925..18a5af98f59 100644 --- a/src/workerd/api/cache-instrumentation-test.js +++ b/src/workerd/api/cache-instrumentation-test.js @@ -18,12 +18,21 @@ export const test = { // Recorded streaming tail worker events, in insertion order, // filtering spans not associated with Cache - let received = Array.from(spans.values()).filter( - (span) => span.name !== 'jsRpcSession' - ); + let received = Array.from(spans.values()); + console.log(received); // spans emitted by cache-test.js in execution order let expected = [ + { + name: 'cache_match', + 'url.full': 'https://example.com/conditional-etag', + closed: true, + }, + { + name: 'cache_match', + 'url.full': 'https://example.com/conditional-last-modified', + closed: true, + }, { name: 'cache_delete', 'url.full': 'https://example.com/delete-exists', @@ -39,6 +48,72 @@ export const test = { 'url.full': 'https://example.com/delete-with-options', closed: true, }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-cache-tag', + 'cache_control.cacheability': 'public', + 'cache_control.expiration': 'max-age=3600', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-etag', + 'cache_control.cacheability': 'public', + 'cache_control.expiration': 'max-age=3600', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-expires', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-last-modified', + 'cache_control.cacheability': 'public', + 'cache_control.expiration': 'max-age=3600', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-set-cookie', + 'cache_control.cacheability': 'public', + 'cache_control.expiration': 'max-age=3600', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-set-cookie-private', + 'cache_control.cacheability': 'private', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-must-revalidate', + 'cache_control.cacheability': 'public', + 'cache_control.revalidation': 'must-revalidate', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-proxy-revalidate', + 'cache_control.cacheability': 'public', + 'cache_control.revalidation': 'proxy-revalidate', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-private', + 'cache_control.cacheability': 'private', + 'cache_control.expiration': 'max-age=3600', + closed: true, + }, + { + name: 'cache_put', + 'url.full': 'https://example.com/put-no-cache', + 'cache_control.expiration': 'no-cache', + closed: true, + }, { name: 'cache_match', 'url.full': 'https://example.com/cached-resource', diff --git a/src/workerd/api/cache-operations.js b/src/workerd/api/cache-operations.js new file mode 100644 index 00000000000..7e14b1bf23c --- /dev/null +++ b/src/workerd/api/cache-operations.js @@ -0,0 +1,306 @@ +// Copyright (c) 2017-2023 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 +// +// Cache API operations for generating instrumentation telemetry. +// The actual validation of telemetry spans happens in cache-instrumentation-test.js + +import * as assert from 'node:assert'; + +// Note that these aren't really tests of the cache API. We run through a bunch +// of the cache API operations to emit telemetry for the tests in cache-instrumentation-test.js + +export const matchOperations = { + async test(ctrl, env, ctx) { + const cache = caches.default; + + // Test cache.match() - HIT case + const matchRequest1 = new Request('https://example.com/cached-resource', { + method: 'GET', + }); + let matchResult = await cache.match(matchRequest1); + assert.ok(matchResult !== undefined, 'Should have a cache HIT'); + assert.strictEqual(await matchResult.text(), 'Cached content'); + + // Test cache.match() - MISS case + const matchRequest2 = new Request('https://example.com/not-cached', { + method: 'GET', + }); + matchResult = await cache.match(matchRequest2); + assert.strictEqual(matchResult, undefined, 'Should be cache MISS'); + + // Test cache.match() with options + const matchRequest3 = new Request( + 'https://example.com/cached-with-options', + { + method: 'GET', + } + ); + matchResult = await cache.match(matchRequest3, { ignoreMethod: false }); + assert.strictEqual( + matchResult, + undefined, + 'Should be cache MISS with options' + ); + }, +}; + +export const putOperations = { + async test(ctrl, env, ctx) { + const cache = caches.default; + + // Test cache.put() with max-age + const putRequest1 = new Request('https://example.com/put-resource', { + method: 'GET', + }); + const putResponse1 = new Response('Test content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600', + }, + }); + await cache.put(putRequest1, putResponse1); + + // Test cache.put() with no-store + const putRequest2 = new Request('https://example.com/put-no-store', { + method: 'GET', + }); + const putResponse2 = new Response('No store content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'no-store', + }, + }); + await cache.put(putRequest2, putResponse2); + + // Test cache.put() with s-maxage + const putRequest3 = new Request('https://example.com/put-s-maxage', { + method: 'GET', + }); + const putResponse3 = new Response('S-maxage content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 's-maxage=7200, public', + }, + }); + await cache.put(putRequest3, putResponse3); + }, +}; + +export const deleteOperations = { + async test(ctrl, env, ctx) { + const cache = caches.default; + + // Test cache.delete() - exists + const deleteRequest1 = new Request('https://example.com/delete-exists', { + method: 'GET', + }); + let deleteResult = await cache.delete(deleteRequest1); + assert.strictEqual( + deleteResult, + true, + 'Should return true for successful delete' + ); + + // Test cache.delete() - doesn't exist + const deleteRequest2 = new Request( + 'https://example.com/delete-not-exists', + { + method: 'GET', + } + ); + deleteResult = await cache.delete(deleteRequest2); + assert.strictEqual( + deleteResult, + false, + 'Should return false when not found' + ); + + // Test cache.delete() with options + const deleteRequest3 = new Request( + 'https://example.com/delete-with-options', + { + method: 'POST', + } + ); + deleteResult = await cache.delete(deleteRequest3, { ignoreMethod: true }); + assert.strictEqual(deleteResult, false, 'Should handle options'); + }, +}; + +export const headersOperations = { + async test(ctrl, env, ctx) { + const cache = caches.default; + + // Test cache.put() with Cache-Tag header + const putRequest1 = new Request('https://example.com/put-cache-tag', { + method: 'GET', + }); + const putResponse1 = new Response('Tagged content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600', + 'Cache-Tag': 'tag1,tag2,tag3', + }, + }); + await cache.put(putRequest1, putResponse1); + + // Test cache.put() with ETag header + const putRequest2 = new Request('https://example.com/put-etag', { + method: 'GET', + }); + const putResponse2 = new Response('ETag content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600', + ETag: '"abc123"', + }, + }); + await cache.put(putRequest2, putResponse2); + + // Test cache.put() with Expires header + const putRequest3 = new Request('https://example.com/put-expires', { + method: 'GET', + }); + const putResponse3 = new Response('Expires content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + Expires: 'Wed, 21 Oct 2025 07:28:00 GMT', + }, + }); + await cache.put(putRequest3, putResponse3); + + // Test cache.put() with Last-Modified header + const putRequest4 = new Request('https://example.com/put-last-modified', { + method: 'GET', + }); + const putResponse4 = new Response('Last-Modified content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600', + 'Last-Modified': 'Wed, 21 Oct 2020 07:28:00 GMT', + }, + }); + await cache.put(putRequest4, putResponse4); + + // Test cache.put() with Set-Cookie header (should not cache or require special handling) + const putRequest5 = new Request('https://example.com/put-set-cookie', { + method: 'GET', + }); + const putResponse5 = new Response('Cookie content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600', + 'Set-Cookie': 'sessionid=abc123; Path=/; HttpOnly', + }, + }); + await cache.put(putRequest5, putResponse5); + + // Test cache.put() with Set-Cookie and Cache-Control: private=Set-Cookie + const putRequest6 = new Request( + 'https://example.com/put-set-cookie-private', + { + method: 'GET', + } + ); + const putResponse6 = new Response('Cookie content with private', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600, private=Set-Cookie', + 'Set-Cookie': 'sessionid=xyz789; Path=/; HttpOnly', + }, + }); + await cache.put(putRequest6, putResponse6); + + // Test cache.put() with must-revalidate + const putRequest7 = new Request('https://example.com/put-must-revalidate', { + method: 'GET', + }); + const putResponse7 = new Response('Must revalidate content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600, must-revalidate', + }, + }); + await cache.put(putRequest7, putResponse7); + + // Test cache.put() with proxy-revalidate + const putRequest8 = new Request( + 'https://example.com/put-proxy-revalidate', + { + method: 'GET', + } + ); + const putResponse8 = new Response('Proxy revalidate content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'public, max-age=3600, proxy-revalidate', + }, + }); + await cache.put(putRequest8, putResponse8); + + // Test cache.put() with private cache control + const putRequest9 = new Request('https://example.com/put-private', { + method: 'GET', + }); + const putResponse9 = new Response('Private content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'private, max-age=3600', + }, + }); + await cache.put(putRequest9, putResponse9); + + // Test cache.put() with no-cache + const putRequest10 = new Request('https://example.com/put-no-cache', { + method: 'GET', + }); + const putResponse10 = new Response('No-cache content', { + status: 200, + headers: { + 'Content-Type': 'text/plain', + 'Cache-Control': 'no-cache, max-age=3600', + }, + }); + await cache.put(putRequest10, putResponse10); + }, +}; + +export const conditionalRequestOperations = { + async test(ctrl, env, ctx) { + const cache = caches.default; + + // Test cache.match() with If-None-Match (should work with ETag) + const matchRequest1 = new Request('https://example.com/conditional-etag', { + method: 'GET', + headers: { + 'If-None-Match': '"abc123"', + }, + }); + let matchResult = await cache.match(matchRequest1); + + // Test cache.match() with If-Modified-Since (should work with Last-Modified) + const matchRequest2 = new Request( + 'https://example.com/conditional-last-modified', + { + method: 'GET', + headers: { + 'If-Modified-Since': 'Wed, 21 Oct 2020 07:28:00 GMT', + }, + } + ); + matchResult = await cache.match(matchRequest2); + }, +}; diff --git a/src/workerd/api/cache-test.js b/src/workerd/api/cache-test.js deleted file mode 100644 index 6f0061853ee..00000000000 --- a/src/workerd/api/cache-test.js +++ /dev/null @@ -1,126 +0,0 @@ -// Copyright (c) 2017-2023 Cloudflare, Inc. -// Licensed under the Apache 2.0 license found in the LICENSE file or at: -// https://opensource.org/licenses/Apache-2.0 - -import * as assert from 'node:assert'; - -export const matchTest = { - async test(ctrl, env, ctx) { - const cache = caches.default; - - // Test cache.match() - HIT case - const matchRequest1 = new Request('https://example.com/cached-resource', { - method: 'GET', - }); - let matchResult = await cache.match(matchRequest1); - assert.ok(matchResult !== undefined, 'Should have a cache HIT'); - assert.strictEqual(await matchResult.text(), 'Cached content'); - - // Test cache.match() - MISS case - const matchRequest2 = new Request('https://example.com/not-cached', { - method: 'GET', - }); - matchResult = await cache.match(matchRequest2); - assert.strictEqual(matchResult, undefined, 'Should be cache MISS'); - - // Test cache.match() with options - const matchRequest3 = new Request( - 'https://example.com/cached-with-options', - { - method: 'GET', - } - ); - matchResult = await cache.match(matchRequest3, { ignoreMethod: false }); - assert.strictEqual( - matchResult, - undefined, - 'Should be cache MISS with options' - ); - }, -}; - -export const putTest = { - async test(ctrl, env, ctx) { - const cache = caches.default; - - // Test cache.put() with max-age - const putRequest1 = new Request('https://example.com/put-resource', { - method: 'GET', - }); - const putResponse1 = new Response('Test content', { - status: 200, - headers: { - 'Content-Type': 'text/plain', - 'Cache-Control': 'public, max-age=3600', - }, - }); - await cache.put(putRequest1, putResponse1); - - // Test cache.put() with no-store - const putRequest2 = new Request('https://example.com/put-no-store', { - method: 'GET', - }); - const putResponse2 = new Response('No store content', { - status: 200, - headers: { - 'Content-Type': 'text/plain', - 'Cache-Control': 'no-store', - }, - }); - await cache.put(putRequest2, putResponse2); - - // Test cache.put() with s-maxage - const putRequest3 = new Request('https://example.com/put-s-maxage', { - method: 'GET', - }); - const putResponse3 = new Response('S-maxage content', { - status: 200, - headers: { - 'Content-Type': 'text/plain', - 'Cache-Control': 's-maxage=7200, public', - }, - }); - await cache.put(putRequest3, putResponse3); - }, -}; - -export const deleteTest = { - async test(ctrl, env, ctx) { - const cache = caches.default; - - // Test cache.delete() - exists - const deleteRequest1 = new Request('https://example.com/delete-exists', { - method: 'GET', - }); - let deleteResult = await cache.delete(deleteRequest1); - assert.strictEqual( - deleteResult, - true, - 'Should return true for successful delete' - ); - - // Test cache.delete() - doesn't exist - const deleteRequest2 = new Request( - 'https://example.com/delete-not-exists', - { - method: 'GET', - } - ); - deleteResult = await cache.delete(deleteRequest2); - assert.strictEqual( - deleteResult, - false, - 'Should return false when not found' - ); - - // Test cache.delete() with options - const deleteRequest3 = new Request( - 'https://example.com/delete-with-options', - { - method: 'POST', - } - ); - deleteResult = await cache.delete(deleteRequest3, { ignoreMethod: true }); - assert.strictEqual(deleteResult, false, 'Should handle options'); - }, -}; diff --git a/src/workerd/api/cache-test.wd-test b/src/workerd/api/cache-test.wd-test index 5d0d93a8926..27c4e3740a4 100644 --- a/src/workerd/api/cache-test.wd-test +++ b/src/workerd/api/cache-test.wd-test @@ -5,7 +5,7 @@ const unitTests :Workerd.Config = ( ( name = "cache-test", worker = ( modules = [ - ( name = "worker", esModule = embed "cache-test.js" ) + ( name = "worker", esModule = embed "cache-operations.js" ) ], cacheApiOutbound = "cache-backend", compatibilityDate = "2023-07-24", From 69f7c29e4fbf521a1ff5f7834be821e7ecd2c2c8 Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 14:00:50 -0700 Subject: [PATCH 5/9] Migrate match to the new instrumentation style --- src/workerd/api/cache-instrumentation-test.js | 30 ++++++-- src/workerd/api/cache-operations.js | 2 +- src/workerd/api/cache.c++ | 75 +++++++++++++------ src/workerd/api/cache.h | 6 +- src/workerd/io/io-thread-context.c++ | 3 + src/workerd/io/io-thread-context.h | 5 +- 6 files changed, 87 insertions(+), 34 deletions(-) diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js index 18a5af98f59..3e9a2e5d72c 100644 --- a/src/workerd/api/cache-instrumentation-test.js +++ b/src/workerd/api/cache-instrumentation-test.js @@ -21,16 +21,24 @@ export const test = { let received = Array.from(spans.values()); console.log(received); - // spans emitted by cache-test.js in execution order + // spans emitted by cache-operations.js in execution order let expected = [ { name: 'cache_match', - 'url.full': 'https://example.com/conditional-etag', + 'cache.url': 'https://example.com/conditional-etag', + 'cache.header.if_none_match': 'abc123', + 'cache.response.status_code': 504n, + 'cache.response.body.size': 0n, + 'cache.response.cache_status': 'MISS', closed: true, }, { name: 'cache_match', - 'url.full': 'https://example.com/conditional-last-modified', + 'cache.url': 'https://example.com/conditional-last-modified', + 'cache.header.if_modified_since': 'Wed, 21 Oct 2020 07:28:00 GMT', + 'cache.response.status_code': 504n, + 'cache.response.body.size': 0n, + 'cache.response.cache_status': 'MISS', closed: true, }, { @@ -116,17 +124,27 @@ export const test = { }, { name: 'cache_match', - 'url.full': 'https://example.com/cached-resource', + 'cache.url': 'https://example.com/cached-resource', + 'cache.response.status_code': 200n, + 'cache.response.body.size': 14n, + 'cache.response.cache_status': 'HIT', closed: true, }, { name: 'cache_match', - 'url.full': 'https://example.com/not-cached', + 'cache.url': 'https://example.com/not-cached', + 'cache.response.status_code': 504n, + 'cache.response.body.size': 0n, + 'cache.response.cache_status': 'MISS', closed: true, }, { name: 'cache_match', - 'url.full': 'https://example.com/cached-with-options', + 'cache.ignore_method': false, + 'cache.url': 'https://example.com/cached-with-options', + 'cache.response.status_code': 504n, + 'cache.response.body.size': 0n, + 'cache.response.cache_status': 'MISS', closed: true, }, { diff --git a/src/workerd/api/cache-operations.js b/src/workerd/api/cache-operations.js index 7e14b1bf23c..52759acf5dd 100644 --- a/src/workerd/api/cache-operations.js +++ b/src/workerd/api/cache-operations.js @@ -286,7 +286,7 @@ export const conditionalRequestOperations = { const matchRequest1 = new Request('https://example.com/conditional-etag', { method: 'GET', headers: { - 'If-None-Match': '"abc123"', + 'If-None-Match': 'abc123', }, }); let matchResult = await cache.match(matchRequest1); diff --git a/src/workerd/api/cache.c++ b/src/workerd/api/cache.c++ index 1780f185bf8..7621e77a852 100644 --- a/src/workerd/api/cache.c++ +++ b/src/workerd/api/cache.c++ @@ -76,8 +76,16 @@ jsg::Promise>> Cache::match(jsg::Lock& js, CompatibilityFlags::Reader flags) { // TODO(someday): Implement Cache API in preview. auto& context = IoContext::current(); - // TODO start span here - // TODO add options to span + auto traceSpan = context.makeTraceSpan("cache_match"_kjc); + auto userSpan = context.makeUserTraceSpan("cache_match"_kjc); + TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); + + KJ_IF_SOME(o, options) { + KJ_IF_SOME(ignoreMethod, o.ignoreMethod) { + traceContext.userSpan.setTag("cache.ignore_method"_kjc, ignoreMethod); + } + } + if (context.isFiddle()) { context.logWarningOnce(CACHE_API_PREVIEW_WARNING); return js.resolvedPromise(jsg::Optional>()); @@ -88,31 +96,46 @@ jsg::Promise>> Cache::match(jsg::Lock& js, return js.evalNow([&]() -> jsg::Promise>> { auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); - // add jsRequest->getUrl() to span - + traceContext.userSpan.setTag("cache.url"_kjc, kj::str(jsRequest->getUrl())); if (!options.orDefault({}).ignoreMethod.orDefault(false) && jsRequest->getMethodEnum() != kj::HttpMethod::GET) { return js.resolvedPromise(jsg::Optional>()); } - auto httpClient = getHttpClient(context, jsRequest->serializeCfBlobJson(js), "cache_match"_kjc, - jsRequest->getUrl(), kj::none, flags.getCacheApiCompatFlags()); + auto httpClient = getHttpClientNew( + context, jsRequest->serializeCfBlobJson(js), traceContext, flags.getCacheApiCompatFlags()); auto requestHeaders = kj::HttpHeaders(context.getHeaderTable()); jsRequest->shallowCopyHeadersTo(requestHeaders); + auto headerIds = context.getHeaderIds(); // parse each of the request headers to add info to span + KJ_IF_SOME(range, requestHeaders.get(headerIds.range)) { + traceContext.userSpan.setTag("cache.header.range"_kjc, kj::str(range)); + } + KJ_IF_SOME(ifModifiedSince, requestHeaders.get(headerIds.ifModifiedSince)) { + traceContext.userSpan.setTag("cache.header.if_modified_since"_kjc, kj::str(ifModifiedSince)); + } + KJ_IF_SOME(ifNoneMatch, requestHeaders.get(headerIds.ifNoneMatch)) { + traceContext.userSpan.setTag("cache.header.if_none_match"_kjc, kj::str(ifNoneMatch)); + } requestHeaders.setPtr(context.getHeaderIds().cacheControl, "only-if-cached"); auto nativeRequest = httpClient->request( kj::HttpMethod::GET, validateUrl(jsRequest->getUrl()), requestHeaders, uint64_t(0)); return context.awaitIo(js, kj::mv(nativeRequest.response), - [httpClient = kj::mv(httpClient), &context](jsg::Lock& js, + [httpClient = kj::mv(httpClient), &context, traceContext = kj::mv(traceContext), + headerIds = kj::mv(headerIds)](jsg::Lock& js, kj::HttpClient::Response&& response) mutable -> jsg::Optional> { response.body = response.body.attach(kj::mv(httpClient)); + traceContext.userSpan.setTag("cache.response.status_code"_kjc, int64_t(response.statusCode)); + KJ_IF_SOME(length, response.body->tryGetLength()) { + traceContext.userSpan.setTag("cache.response.body.size"_kjc, int64_t(length)); + } + kj::StringPtr cacheStatus; - KJ_IF_SOME(cs, response.headers->get(context.getHeaderIds().cfCacheStatus)) { + KJ_IF_SOME(cs, response.headers->get(headerIds.cfCacheStatus)) { cacheStatus = cs; } else { // This is an internal error representing a violation of the contract between us and @@ -125,6 +148,7 @@ jsg::Promise>> Cache::match(jsg::Lock& js, } // TODO add cacheStatus to span + traceContext.userSpan.setTag("cache.response.cache_status"_kjc, kj::str(cacheStatus)); // The status code should be a 504 on cache miss, but we need to rely on CF-Cache-Status // because someone might cache a 504. @@ -608,22 +632,25 @@ kj::Own Cache::getHttpClient(IoContext& context, return httpClient; } -// kj::Own Cache::getHttpClientNew( -// IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags) { -// auto cacheClient = context.getCacheClient(); -// auto metadata = CacheClient::SubrequestMetadata{ -// .cfBlobJson = kj::mv(cfBlobJson), -// .featureFlagsForFl = kj::none, -// }; -// if (enableCompatFlags) { -// metadata.featureFlagsForFl = context.getWorker().getIsolate().getFeatureFlagsForFl(); -// } -// auto httpClient = -// cacheName.map([&](kj::String& n) { -// return cacheClient->getNamespace(n, kj::mv(metadata)); -// }).orDefault([&]() { return cacheClient->getDefault(kj::mv(metadata)); }); -// return httpClient; -// } +kj::Own Cache::getHttpClientNew(IoContext& context, + kj::Maybe cfBlobJson, + TraceContext& traceContext, + bool enableCompatFlags) { + auto cacheClient = context.getCacheClient(); + auto metadata = CacheClient::SubrequestMetadata{ + .cfBlobJson = kj::mv(cfBlobJson), + .parentSpan = traceContext.span, + .featureFlagsForFl = kj::none, + }; + if (enableCompatFlags) { + metadata.featureFlagsForFl = context.getWorker().getIsolate().getFeatureFlagsForFl(); + } + auto httpClient = + cacheName.map([&](kj::String& n) { + return cacheClient->getNamespace(n, kj::mv(metadata)); + }).orDefault([&]() { return cacheClient->getDefault(kj::mv(metadata)); }); + return httpClient; +} // ======================================================================================= // CacheStorage diff --git a/src/workerd/api/cache.h b/src/workerd/api/cache.h index 3cdf658af60..3a7ac3f3ee9 100644 --- a/src/workerd/api/cache.h +++ b/src/workerd/api/cache.h @@ -96,8 +96,10 @@ class Cache: public jsg::Object { kj::Maybe cacheControl, bool enableCompatFlags); - // kj::Own getHttpClientNew( - // IoContext& context, kj::Maybe cfBlobJson, bool enableCompatFlags); + kj::Own getHttpClientNew(IoContext& context, + kj::Maybe cfBlobJson, + TraceContext& traceContext, + bool enableCompatFlags); }; // ======================================================================================= diff --git a/src/workerd/io/io-thread-context.c++ b/src/workerd/io/io-thread-context.c++ index 00a8e286858..51c336eb8e2 100644 --- a/src/workerd/io/io-thread-context.c++ +++ b/src/workerd/io/io-thread-context.c++ @@ -9,6 +9,9 @@ ThreadContext::HeaderIdBundle::HeaderIdBundle(kj::HttpHeaderTable::Builder& buil cacheControl(builder.add("Cache-Control")), pragma(builder.add("Pragma")), cfCacheNamespace(builder.add("CF-Cache-Namespace")), + range(builder.add("Range")), + ifModifiedSince(builder.add("If-Modified-Since")), + ifNoneMatch(builder.add("If-None-Match")), cfKvMetadata(builder.add("CF-KV-Metadata")), cfR2ErrorHeader(builder.add("CF-R2-Error")), cfBlobMetadataSize(builder.add("CF-R2-Metadata-Size")), diff --git a/src/workerd/io/io-thread-context.h b/src/workerd/io/io-thread-context.h index 3ab58e1ac6a..3217573efe9 100644 --- a/src/workerd/io/io-thread-context.h +++ b/src/workerd/io/io-thread-context.h @@ -18,7 +18,10 @@ class ThreadContext { const kj::HttpHeaderId cfCacheStatus; // used by cache API implementation const kj::HttpHeaderId cacheControl; const kj::HttpHeaderId pragma; - const kj::HttpHeaderId cfCacheNamespace; // used by Cache binding implementation + const kj::HttpHeaderId cfCacheNamespace; // used by Cache binding implementation + const kj::HttpHeaderId range; + const kj::HttpHeaderId ifModifiedSince; + const kj::HttpHeaderId ifNoneMatch; const kj::HttpHeaderId cfKvMetadata; // used by KV binding implementation const kj::HttpHeaderId cfR2ErrorHeader; // used by R2 binding implementation const kj::HttpHeaderId cfBlobMetadataSize; // used by R2 binding implementation From 7053dbf91016d025fd5a6c6b378d04207cb4feca Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 14:26:07 -0700 Subject: [PATCH 6/9] Migrate delete --- src/workerd/api/cache-instrumentation-test.js | 13 ++++++-- src/workerd/api/cache.c++ | 32 +++++++++++++------ 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js index 3e9a2e5d72c..17f8f4a3f14 100644 --- a/src/workerd/api/cache-instrumentation-test.js +++ b/src/workerd/api/cache-instrumentation-test.js @@ -43,17 +43,24 @@ export const test = { }, { name: 'cache_delete', - 'url.full': 'https://example.com/delete-exists', + 'cache.url': 'https://example.com/delete-exists', + 'cache.response.status_code': 200n, + 'cache.response.success': true, closed: true, }, { name: 'cache_delete', - 'url.full': 'https://example.com/delete-not-exists', + 'cache.url': 'https://example.com/delete-not-exists', + 'cache.response.status_code': 404n, + 'cache.response.success': false, closed: true, }, { name: 'cache_delete', - 'url.full': 'https://example.com/delete-with-options', + 'cache.ignore_method': true, + 'cache.url': 'https://example.com/delete-with-options', + 'cache.response.status_code': 404n, + 'cache.response.success': false, closed: true, }, { diff --git a/src/workerd/api/cache.c++ b/src/workerd/api/cache.c++ index 7621e77a852..9646ff182aa 100644 --- a/src/workerd/api/cache.c++ +++ b/src/workerd/api/cache.c++ @@ -124,8 +124,8 @@ jsg::Promise>> Cache::match(jsg::Lock& js, kj::HttpMethod::GET, validateUrl(jsRequest->getUrl()), requestHeaders, uint64_t(0)); return context.awaitIo(js, kj::mv(nativeRequest.response), - [httpClient = kj::mv(httpClient), &context, traceContext = kj::mv(traceContext), - headerIds = kj::mv(headerIds)](jsg::Lock& js, + [httpClient = kj::mv(httpClient), &context, traceContext = kj::mv(traceContext)]( + jsg::Lock& js, kj::HttpClient::Response&& response) mutable -> jsg::Optional> { response.body = response.body.attach(kj::mv(httpClient)); @@ -135,8 +135,9 @@ jsg::Promise>> Cache::match(jsg::Lock& js, } kj::StringPtr cacheStatus; - KJ_IF_SOME(cs, response.headers->get(headerIds.cfCacheStatus)) { + KJ_IF_SOME(cs, response.headers->get(context.getHeaderIds().cfCacheStatus)) { cacheStatus = cs; + traceContext.userSpan.setTag("cache.response.cache_status"_kjc, kj::str(cacheStatus)); } else { // This is an internal error representing a violation of the contract between us and // the cache. Since it is always conformant to return undefined from Cache::match() @@ -147,9 +148,6 @@ jsg::Promise>> Cache::match(jsg::Lock& js, return kj::none; } - // TODO add cacheStatus to span - traceContext.userSpan.setTag("cache.response.cache_status"_kjc, kj::str(cacheStatus)); - // The status code should be a 504 on cache miss, but we need to rely on CF-Cache-Status // because someone might cache a 504. // See https://httpwg.org/specs/rfc7234.html#cache-request-directive.only-if-cached @@ -519,6 +517,16 @@ jsg::Promise Cache::delete_(jsg::Lock& js, CompatibilityFlags::Reader flags) { // TODO(someday): Implement Cache API in preview. auto& context = IoContext::current(); + auto traceSpan = context.makeTraceSpan("cache_delete"_kjc); + auto userSpan = context.makeUserTraceSpan("cache_delete"_kjc); + TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); + + KJ_IF_SOME(o, options) { + KJ_IF_SOME(ignoreMethod, o.ignoreMethod) { + traceContext.userSpan.setTag("cache.ignore_method"_kjc, ignoreMethod); + } + } + if (context.isFiddle()) { context.logWarningOnce(CACHE_API_PREVIEW_WARNING); return js.resolvedPromise(false); @@ -529,6 +537,7 @@ jsg::Promise Cache::delete_(jsg::Lock& js, return js.evalNow([&]() -> jsg::Promise { auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); + traceContext.userSpan.setTag("cache.url"_kjc, kj::str(jsRequest->getUrl())); if (!options.orDefault({}).ignoreMethod.orDefault(false) && jsRequest->getMethodEnum() != kj::HttpMethod::GET) { return js.resolvedPromise(false); @@ -536,8 +545,8 @@ jsg::Promise Cache::delete_(jsg::Lock& js, // Make the PURGE request to cache. - auto httpClient = getHttpClient(context, jsRequest->serializeCfBlobJson(js), "cache_delete"_kjc, - jsRequest->getUrl(), kj::none, flags.getCacheApiCompatFlags()); + auto httpClient = getHttpClientNew( + context, jsRequest->serializeCfBlobJson(js), traceContext, flags.getCacheApiCompatFlags()); auto requestHeaders = kj::HttpHeaders(context.getHeaderTable()); jsRequest->shallowCopyHeadersTo(requestHeaders); // HACK: The cache doesn't permit PURGE requests from the outside world. It does this by @@ -551,12 +560,17 @@ jsg::Promise Cache::delete_(jsg::Lock& js, kj::HttpMethod::PURGE, validateUrl(jsRequest->getUrl()), requestHeaders, uint64_t(0)); return context.awaitIo(js, kj::mv(nativeRequest.response), - [httpClient = kj::mv(httpClient)](jsg::Lock&, kj::HttpClient::Response&& response) -> bool { + [httpClient = kj::mv(httpClient), traceContext = kj::mv(traceContext)]( + jsg::Lock&, kj::HttpClient::Response&& response) mutable -> bool { + traceContext.userSpan.setTag("cache.response.status_code"_kjc, int64_t(response.statusCode)); if (response.statusCode == 200) { + traceContext.userSpan.setTag("cache.response.success"_kjc, true); return true; } else if (response.statusCode == 404) { + traceContext.userSpan.setTag("cache.response.success"_kjc, false); return false; } else if (response.statusCode == 429) { + traceContext.userSpan.setTag("cache.response.success"_kjc, false); // Throw, but do not log the response to Sentry, as rate-limited subrequests are normal JSG_FAIL_REQUIRE( Error, "Unable to delete cached response. Subrequests are being rate-limited."); From 09ad98a423d14abd01285083bcc4738b48915682 Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 15:13:38 -0700 Subject: [PATCH 7/9] Migrate put --- src/workerd/api/cache-instrumentation-test.js | 150 +++++++++++++----- src/workerd/api/cache.c++ | 137 +++++++--------- src/workerd/api/cache.h | 7 - src/workerd/io/io-thread-context.c++ | 4 + src/workerd/io/io-thread-context.h | 4 + 5 files changed, 169 insertions(+), 133 deletions(-) diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js index 17f8f4a3f14..2cbf3fedc21 100644 --- a/src/workerd/api/cache-instrumentation-test.js +++ b/src/workerd/api/cache-instrumentation-test.js @@ -19,38 +19,42 @@ export const test = { // Recorded streaming tail worker events, in insertion order, // filtering spans not associated with Cache let received = Array.from(spans.values()); - console.log(received); // spans emitted by cache-operations.js in execution order let expected = [ { name: 'cache_match', - 'cache.url': 'https://example.com/conditional-etag', - 'cache.header.if_none_match': 'abc123', + 'cache.request.url': 'https://example.com/conditional-etag', + 'cache.request.method': 'GET', + 'cache.request.header.if_none_match': 'abc123', 'cache.response.status_code': 504n, 'cache.response.body.size': 0n, 'cache.response.cache_status': 'MISS', + 'cache.response.success': false, closed: true, }, { name: 'cache_match', - 'cache.url': 'https://example.com/conditional-last-modified', - 'cache.header.if_modified_since': 'Wed, 21 Oct 2020 07:28:00 GMT', + 'cache.request.url': 'https://example.com/conditional-last-modified', + 'cache.request.method': 'GET', + 'cache.request.header.if_modified_since': + 'Wed, 21 Oct 2020 07:28:00 GMT', 'cache.response.status_code': 504n, 'cache.response.body.size': 0n, 'cache.response.cache_status': 'MISS', + 'cache.response.success': false, closed: true, }, { name: 'cache_delete', - 'cache.url': 'https://example.com/delete-exists', + 'cache.request.url': 'https://example.com/delete-exists', 'cache.response.status_code': 200n, 'cache.response.success': true, closed: true, }, { name: 'cache_delete', - 'cache.url': 'https://example.com/delete-not-exists', + 'cache.request.url': 'https://example.com/delete-not-exists', 'cache.response.status_code': 404n, 'cache.response.success': false, closed: true, @@ -58,119 +62,177 @@ export const test = { { name: 'cache_delete', 'cache.ignore_method': true, - 'cache.url': 'https://example.com/delete-with-options', + 'cache.request.url': 'https://example.com/delete-with-options', 'cache.response.status_code': 404n, 'cache.response.success': false, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-cache-tag', - 'cache_control.cacheability': 'public', - 'cache_control.expiration': 'max-age=3600', + 'cache.request.url': 'https://example.com/put-cache-tag', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'public, max-age=3600', + 'cache.request.payload.header.cache_tag': 'tag1,tag2,tag3', + 'cache.request.payload.size': 143n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-etag', - 'cache_control.cacheability': 'public', - 'cache_control.expiration': 'max-age=3600', + 'cache.request.url': 'https://example.com/put-etag', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'public, max-age=3600', + 'cache.request.payload.header.etag': '"abc123"', + 'cache.request.payload.size': 130n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-expires', + 'cache.request.url': 'https://example.com/put-expires', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.expires': 'Wed, 21 Oct 2025 07:28:00 GMT', + 'cache.request.payload.size': 120n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-last-modified', - 'cache_control.cacheability': 'public', - 'cache_control.expiration': 'max-age=3600', + 'cache.request.url': 'https://example.com/put-last-modified', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'public, max-age=3600', + 'cache.request.payload.header.last_modified': + 'Wed, 21 Oct 2020 07:28:00 GMT', + 'cache.request.payload.size': 169n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-set-cookie', - 'cache_control.cacheability': 'public', - 'cache_control.expiration': 'max-age=3600', + 'cache.request.url': 'https://example.com/put-set-cookie', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'public, max-age=3600', + 'cache.request.payload.size': 164n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-set-cookie-private', - 'cache_control.cacheability': 'private', + 'cache.request.url': 'https://example.com/put-set-cookie-private', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': + 'public, max-age=3600, private=Set-Cookie', + 'cache.request.payload.size': 197n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-must-revalidate', - 'cache_control.cacheability': 'public', - 'cache_control.revalidation': 'must-revalidate', + 'cache.request.url': 'https://example.com/put-must-revalidate', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': + 'public, max-age=3600, must-revalidate', + 'cache.request.payload.size': 142n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-proxy-revalidate', - 'cache_control.cacheability': 'public', - 'cache_control.revalidation': 'proxy-revalidate', + 'cache.request.url': 'https://example.com/put-proxy-revalidate', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': + 'public, max-age=3600, proxy-revalidate', + 'cache.request.payload.size': 144n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-private', - 'cache_control.cacheability': 'private', - 'cache_control.expiration': 'max-age=3600', + 'cache.request.url': 'https://example.com/put-private', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'private, max-age=3600', + 'cache.request.payload.size': 118n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-no-cache', - 'cache_control.expiration': 'no-cache', + 'cache.request.url': 'https://example.com/put-no-cache', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'no-cache, max-age=3600', + 'cache.request.payload.size': 120n, + 'cache.response.success': true, closed: true, }, { name: 'cache_match', - 'cache.url': 'https://example.com/cached-resource', + 'cache.request.url': 'https://example.com/cached-resource', + 'cache.request.method': 'GET', 'cache.response.status_code': 200n, 'cache.response.body.size': 14n, 'cache.response.cache_status': 'HIT', + 'cache.response.success': true, closed: true, }, { name: 'cache_match', - 'cache.url': 'https://example.com/not-cached', + 'cache.request.url': 'https://example.com/not-cached', + 'cache.request.method': 'GET', 'cache.response.status_code': 504n, 'cache.response.body.size': 0n, 'cache.response.cache_status': 'MISS', + 'cache.response.success': false, closed: true, }, { name: 'cache_match', 'cache.ignore_method': false, - 'cache.url': 'https://example.com/cached-with-options', + 'cache.request.url': 'https://example.com/cached-with-options', + 'cache.request.method': 'GET', 'cache.response.status_code': 504n, 'cache.response.body.size': 0n, 'cache.response.cache_status': 'MISS', + 'cache.response.success': false, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-resource', - 'cache_control.cacheability': 'public', - 'cache_control.expiration': 'max-age=3600', + 'cache.request.url': 'https://example.com/put-resource', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'public, max-age=3600', + 'cache.request.payload.size': 114n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-no-store', - 'cache_control.cacheability': 'no-store', + 'cache.request.url': 'https://example.com/put-no-store', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 'no-store', + 'cache.request.payload.size': 106n, + 'cache.response.success': true, closed: true, }, { name: 'cache_put', - 'url.full': 'https://example.com/put-s-maxage', - 'cache_control.cacheability': 'public', + 'cache.request.url': 'https://example.com/put-s-maxage', + 'cache.request.method': 'GET', + 'cache.request.payload.status_code': 200n, + 'cache.request.payload.header.cache_control': 's-maxage=7200, public', + 'cache.request.payload.size': 119n, + 'cache.response.success': true, closed: true, }, ]; diff --git a/src/workerd/api/cache.c++ b/src/workerd/api/cache.c++ index 9646ff182aa..eb01c7e535c 100644 --- a/src/workerd/api/cache.c++ +++ b/src/workerd/api/cache.c++ @@ -96,13 +96,15 @@ jsg::Promise>> Cache::match(jsg::Lock& js, return js.evalNow([&]() -> jsg::Promise>> { auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); - traceContext.userSpan.setTag("cache.url"_kjc, kj::str(jsRequest->getUrl())); + traceContext.userSpan.setTag("cache.request.url"_kjc, kj::str(jsRequest->getUrl())); + traceContext.userSpan.setTag("cache.request.method"_kjc, kj::str(jsRequest->getMethodEnum())); + if (!options.orDefault({}).ignoreMethod.orDefault(false) && jsRequest->getMethodEnum() != kj::HttpMethod::GET) { return js.resolvedPromise(jsg::Optional>()); } - auto httpClient = getHttpClientNew( + auto httpClient = getHttpClient( context, jsRequest->serializeCfBlobJson(js), traceContext, flags.getCacheApiCompatFlags()); auto requestHeaders = kj::HttpHeaders(context.getHeaderTable()); jsRequest->shallowCopyHeadersTo(requestHeaders); @@ -110,13 +112,14 @@ jsg::Promise>> Cache::match(jsg::Lock& js, auto headerIds = context.getHeaderIds(); // parse each of the request headers to add info to span KJ_IF_SOME(range, requestHeaders.get(headerIds.range)) { - traceContext.userSpan.setTag("cache.header.range"_kjc, kj::str(range)); + traceContext.userSpan.setTag("cache.request.header.range"_kjc, kj::str(range)); } KJ_IF_SOME(ifModifiedSince, requestHeaders.get(headerIds.ifModifiedSince)) { - traceContext.userSpan.setTag("cache.header.if_modified_since"_kjc, kj::str(ifModifiedSince)); + traceContext.userSpan.setTag( + "cache.request.header.if_modified_since"_kjc, kj::str(ifModifiedSince)); } KJ_IF_SOME(ifNoneMatch, requestHeaders.get(headerIds.ifNoneMatch)) { - traceContext.userSpan.setTag("cache.header.if_none_match"_kjc, kj::str(ifNoneMatch)); + traceContext.userSpan.setTag("cache.request.header.if_none_match"_kjc, kj::str(ifNoneMatch)); } requestHeaders.setPtr(context.getHeaderIds().cacheControl, "only-if-cached"); @@ -145,6 +148,7 @@ jsg::Promise>> Cache::match(jsg::Lock& js, // script fail. However, it might be indicative of a larger problem, and should be // investigated. LOG_CACHE_ERROR_ONCE("Response to Cache API GET has no CF-Cache-Status: ", response); + traceContext.userSpan.setTag("cache.response.success"_kjc, false); return kj::none; } @@ -158,13 +162,16 @@ jsg::Promise>> Cache::match(jsg::Lock& js, // this URL result in a 200, causing us to return true from Cache::delete_()? If so, that's // a small inconsistency: we shouldn't have a match failure but a delete success. if (cacheStatus == "MISS" || cacheStatus == "EXPIRED" || cacheStatus == "UPDATING") { + traceContext.userSpan.setTag("cache.response.success"_kjc, false); return kj::none; } else if (cacheStatus != "HIT") { // Another internal error. See above comment where we retrieve the CF-Cache-Status header. LOG_CACHE_ERROR_ONCE("Response to Cache API GET has invalid CF-Cache-Status: ", response); + traceContext.userSpan.setTag("cache.response.success"_kjc, false); return kj::none; } + traceContext.userSpan.setTag("cache.response.success"_kjc, true); return makeHttpResponse(js, kj::HttpMethod::GET, {}, response.statusCode, response.statusText, *response.headers, kj::mv(response.body), kj::none); }); @@ -276,10 +283,14 @@ jsg::Promise Cache::put(jsg::Lock& js, auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); auto& context = IoContext::current(); - // TODO start span here - // TODO add jsResponse->getStatus() to span - // TODO add response method - // TODO parse each of the headers + auto traceSpan = context.makeTraceSpan("cache_put"_kjc); + auto userSpan = context.makeUserTraceSpan("cache_put"_kjc); + TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); + + traceContext.userSpan.setTag("cache.request.url"_kjc, kj::str(jsRequest->getUrl())); + traceContext.userSpan.setTag("cache.request.method"_kjc, kj::str(jsRequest->getMethodEnum())); + traceContext.userSpan.setTag( + "cache.request.payload.status_code"_kjc, int64_t(jsResponse->getStatus())); // TODO(conform): Require that jsRequest's url has an http or https scheme. This is only // important if api::Request is changed to parse its URL eagerly (as required by spec), rather @@ -299,6 +310,24 @@ jsg::Promise Cache::put(jsg::Lock& js, "Cannot cache response with 'Vary: *' header."); } + KJ_IF_SOME(cacheControl, responseHeadersRef->getNoChecks(js, "cache-control"_kj)) { + traceContext.userSpan.setTag( + "cache.request.payload.header.cache_control"_kjc, kj::str(cacheControl)); + } + KJ_IF_SOME(cacheTag, responseHeadersRef->getNoChecks(js, "cache-tag"_kj)) { + traceContext.userSpan.setTag("cache.request.payload.header.cache_tag"_kjc, kj::str(cacheTag)); + } + KJ_IF_SOME(etag, responseHeadersRef->getNoChecks(js, "etag"_kj)) { + traceContext.userSpan.setTag("cache.request.payload.header.etag"_kjc, kj::str(etag)); + } + KJ_IF_SOME(expires, responseHeadersRef->getNoChecks(js, "expires"_kj)) { + traceContext.userSpan.setTag("cache.request.payload.header.expires"_kjc, kj::str(expires)); + } + KJ_IF_SOME(lastModified, responseHeadersRef->getNoChecks(js, "last-modified"_kj)) { + traceContext.userSpan.setTag( + "cache.request.payload.header.last_modified"_kjc, kj::str(lastModified)); + } + if (jsResponse->getStatus() == 304) { // Silently discard 304 status responses to conditional requests. Caching 304s could be a // source of bugs in a worker, since a worker which blindly stuffs responses from `fetch()` @@ -326,7 +355,9 @@ jsg::Promise Cache::put(jsg::Lock& js, auto serializePromise = jsResponse->send(js, serializer, {}, kj::none); auto payload = serializer.getPayload(); - // TODO get the payload size from payload.stream, add to span + KJ_IF_SOME(length, payload.stream->tryGetLength()) { + traceContext.userSpan.setTag("cache.request.payload.size"_kjc, int64_t(length)); + } // TODO(someday): Implement Cache API in preview. This bail-out lives all the way down here, // after all KJ_REQUIRE checks and the start of response serialization, so that Cache.put() @@ -356,11 +387,12 @@ jsg::Promise Cache::put(jsg::Lock& js, [this, &context, jsRequest = kj::mv(jsRequest), cacheControl = kj::mv(cacheControl), serializePromise = kj::mv(serializePromise), writePayloadHeadersPromise = kj::mv(payload.writeHeadersPromise), - enableCompatFlags = flags.getCacheApiCompatFlags()](jsg::Lock& js, + enableCompatFlags = flags.getCacheApiCompatFlags(), + traceContext = kj::mv(traceContext)](jsg::Lock& js, IoOwn payloadStream) mutable -> jsg::Promise { // Make the PUT request to cache. - auto httpClient = getHttpClient(context, jsRequest->serializeCfBlobJson(js), "cache_put"_kjc, - jsRequest->getUrl(), kj::mv(cacheControl), enableCompatFlags); + auto httpClient = getHttpClient( + context, jsRequest->serializeCfBlobJson(js), traceContext, enableCompatFlags); auto requestHeaders = kj::HttpHeaders(context.getHeaderTable()); jsRequest->shallowCopyHeadersTo(requestHeaders); auto nativeRequest = httpClient->request(kj::HttpMethod::PUT, @@ -435,7 +467,8 @@ jsg::Promise Cache::put(jsg::Lock& js, kj::Promise responsePromise, kj::Own bodyStream, kj::Promise pumpRequestBodyPromise, kj::Promise writePayloadHeadersPromise, - kj::Own payloadStream) -> kj::Promise> { + kj::Own payloadStream, + TraceContext traceContext) -> kj::Promise> { // This is extremely odd and a bit annoying but we have to make sure // these are destroyed in a particular order due to cross-dependencies // for each. If the kj::Promise returned by handleSerialize is dropped @@ -484,8 +517,13 @@ jsg::Promise Cache::put(jsg::Lock& js, // ephemeral K/V store, and we never guaranteed the script we'd actually cache anything. if (response.statusCode != 204 && response.statusCode != 413) { LOG_CACHE_ERROR_ONCE("Response to Cache API PUT was neither 204 nor 413: ", response); + } else if (response.statusCode == 204) { + traceContext.userSpan.setTag("cache.response.success"_kjc, true); + } else if (response.statusCode == 413) { + traceContext.userSpan.setTag("cache.response.success"_kjc, false); } } catch (...) { + traceContext.userSpan.setTag("cache.response.success"_kjc, false); auto exception = kj::getCaughtExceptionAsKj(); if (exception.getType() != kj::Exception::Type::DISCONNECTED) { kj::throwFatalException(kj::mv(exception)); @@ -506,7 +544,7 @@ jsg::Promise Cache::put(jsg::Lock& js, handleSerialize(kj::mv(serializePromise), kj::mv(httpClient), kj::mv(nativeRequest.response), kj::mv(nativeRequest.body), kj::mv(pumpRequestBodyPromise), kj::mv(writePayloadHeadersPromise), - kj::mv(payloadStream))); + kj::mv(payloadStream), kj::mv(traceContext))); })); }); } @@ -537,7 +575,7 @@ jsg::Promise Cache::delete_(jsg::Lock& js, return js.evalNow([&]() -> jsg::Promise { auto jsRequest = Request::coerce(js, kj::mv(requestOrUrl), kj::none); - traceContext.userSpan.setTag("cache.url"_kjc, kj::str(jsRequest->getUrl())); + traceContext.userSpan.setTag("cache.request.url"_kjc, kj::str(jsRequest->getUrl())); if (!options.orDefault({}).ignoreMethod.orDefault(false) && jsRequest->getMethodEnum() != kj::HttpMethod::GET) { return js.resolvedPromise(false); @@ -545,7 +583,7 @@ jsg::Promise Cache::delete_(jsg::Lock& js, // Make the PURGE request to cache. - auto httpClient = getHttpClientNew( + auto httpClient = getHttpClient( context, jsRequest->serializeCfBlobJson(js), traceContext, flags.getCacheApiCompatFlags()); auto requestHeaders = kj::HttpHeaders(context.getHeaderTable()); jsRequest->shallowCopyHeadersTo(requestHeaders); @@ -582,71 +620,6 @@ jsg::Promise Cache::delete_(jsg::Lock& js, } kj::Own Cache::getHttpClient(IoContext& context, - kj::Maybe cfBlobJson, - kj::LiteralStringConst operationName, - kj::StringPtr url, - kj::Maybe cacheControl, - bool enableCompatFlags) { - auto span = context.makeTraceSpan(operationName); - auto userSpan = context.makeUserTraceSpan(operationName); - - userSpan.setTag("url.full"_kjc, kj::str(url)); - // TODO(o11y): Can we parse cacheControl more cleanly? For example, if tags are duplicated in the - // same category we should choose the last value. We should also support these tags for - // cache_match (where we should pull them from the returned response and need to keep the span - // alive until then). - KJ_IF_SOME(c, cacheControl) { - // cacheability - if (c.contains("no-store")) { - userSpan.setTag("cache_control.cacheability"_kjc, kj::str("no-store")); - } else if (c.contains("private")) { - userSpan.setTag("cache_control.cacheability"_kjc, kj::str("private")); - } else if (c.contains("public")) { - userSpan.setTag("cache_control.cacheability"_kjc, kj::str("public")); - } - - // expiration - if (c.contains("no-cache")) { - userSpan.setTag("cache_control.expiration"_kjc, kj::str("no-cache")); - } else KJ_IF_SOME(idx, c.find("max-age="_kj)) { - auto maybeNum = c.slice(idx + "max-age="_kj.size()).tryParseAs(); - KJ_IF_SOME(num, maybeNum) { - userSpan.setTag("cache_control.expiration"_kjc, kj::str(kj::str("max-age="), kj::str(num))); - } - } else KJ_IF_SOME(idx, c.find("s-maxage="_kj)) { - auto maybeNum = c.slice(idx + "s-maxage="_kj.size()).tryParseAs(); - KJ_IF_SOME(num, maybeNum) { - userSpan.setTag( - "cache_control.expiration"_kjc, kj::str(kj::str("s-maxage="), kj::str(num))); - } - } - - // revalidation. Note: There are also stale-while-revalidate and stale-if-error directives, but - // they are ignored by the Workers Cache API and we do not set them as tags accordingly. - if (c.contains("must-revalidate")) { - userSpan.setTag("cache_control.revalidation"_kjc, kj::str("must-revalidate")); - } else if (c.contains("proxy-revalidate")) { - userSpan.setTag("cache_control.revalidation"_kjc, kj::str("proxy-revalidate")); - } - } - auto cacheClient = context.getCacheClient(); - auto metadata = CacheClient::SubrequestMetadata{ - .cfBlobJson = kj::mv(cfBlobJson), - .parentSpan = span, - .featureFlagsForFl = kj::none, - }; - if (enableCompatFlags) { - metadata.featureFlagsForFl = context.getWorker().getIsolate().getFeatureFlagsForFl(); - } - auto httpClient = - cacheName.map([&](kj::String& n) { - return cacheClient->getNamespace(n, kj::mv(metadata)); - }).orDefault([&]() { return cacheClient->getDefault(kj::mv(metadata)); }); - httpClient = httpClient.attach(kj::mv(span), kj::mv(userSpan), kj::mv(cacheClient)); - return httpClient; -} - -kj::Own Cache::getHttpClientNew(IoContext& context, kj::Maybe cfBlobJson, TraceContext& traceContext, bool enableCompatFlags) { diff --git a/src/workerd/api/cache.h b/src/workerd/api/cache.h index 3a7ac3f3ee9..bafd161d7da 100644 --- a/src/workerd/api/cache.h +++ b/src/workerd/api/cache.h @@ -90,13 +90,6 @@ class Cache: public jsg::Object { kj::Maybe cacheName; kj::Own getHttpClient(IoContext& context, - kj::Maybe cfBlobJson, - kj::LiteralStringConst operationName, - kj::StringPtr url, - kj::Maybe cacheControl, - bool enableCompatFlags); - - kj::Own getHttpClientNew(IoContext& context, kj::Maybe cfBlobJson, TraceContext& traceContext, bool enableCompatFlags); diff --git a/src/workerd/io/io-thread-context.c++ b/src/workerd/io/io-thread-context.c++ index 51c336eb8e2..faeccd611f1 100644 --- a/src/workerd/io/io-thread-context.c++ +++ b/src/workerd/io/io-thread-context.c++ @@ -7,6 +7,10 @@ ThreadContext::HeaderIdBundle::HeaderIdBundle(kj::HttpHeaderTable::Builder& buil contentEncoding(builder.add("Content-Encoding")), cfCacheStatus(builder.add("CF-Cache-Status")), cacheControl(builder.add("Cache-Control")), + cacheTag(builder.add("Cache-Tag")), + etag(builder.add("ETag")), + expires(builder.add("Expires")), + lastModified(builder.add("Last-Modified")), pragma(builder.add("Pragma")), cfCacheNamespace(builder.add("CF-Cache-Namespace")), range(builder.add("Range")), diff --git a/src/workerd/io/io-thread-context.h b/src/workerd/io/io-thread-context.h index 3217573efe9..bec5565123e 100644 --- a/src/workerd/io/io-thread-context.h +++ b/src/workerd/io/io-thread-context.h @@ -17,6 +17,10 @@ class ThreadContext { const kj::HttpHeaderId contentEncoding; const kj::HttpHeaderId cfCacheStatus; // used by cache API implementation const kj::HttpHeaderId cacheControl; + const kj::HttpHeaderId cacheTag; + const kj::HttpHeaderId etag; + const kj::HttpHeaderId expires; + const kj::HttpHeaderId lastModified; const kj::HttpHeaderId pragma; const kj::HttpHeaderId cfCacheNamespace; // used by Cache binding implementation const kj::HttpHeaderId range; From ea68141e0d0a633385f689842f649f7454347844 Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 15:20:50 -0700 Subject: [PATCH 8/9] Clean up --- src/workerd/api/tests/instrumentation-tail-worker.js | 1 - src/workerd/io/io-thread-context.c++ | 4 ---- src/workerd/io/io-thread-context.h | 4 ---- 3 files changed, 9 deletions(-) diff --git a/src/workerd/api/tests/instrumentation-tail-worker.js b/src/workerd/api/tests/instrumentation-tail-worker.js index ab5e3ccc6a7..0b28f068c52 100644 --- a/src/workerd/api/tests/instrumentation-tail-worker.js +++ b/src/workerd/api/tests/instrumentation-tail-worker.js @@ -9,7 +9,6 @@ export const testTailHandler = { tailStream(event, env, ctx) { // Capture the top-level span ID from the onset event const topLevelSpanId = event.event.spanId; - console.log(event); // For each "onset" event, store a promise which we will resolve when // we receive the equivalent "outcome" event diff --git a/src/workerd/io/io-thread-context.c++ b/src/workerd/io/io-thread-context.c++ index faeccd611f1..51c336eb8e2 100644 --- a/src/workerd/io/io-thread-context.c++ +++ b/src/workerd/io/io-thread-context.c++ @@ -7,10 +7,6 @@ ThreadContext::HeaderIdBundle::HeaderIdBundle(kj::HttpHeaderTable::Builder& buil contentEncoding(builder.add("Content-Encoding")), cfCacheStatus(builder.add("CF-Cache-Status")), cacheControl(builder.add("Cache-Control")), - cacheTag(builder.add("Cache-Tag")), - etag(builder.add("ETag")), - expires(builder.add("Expires")), - lastModified(builder.add("Last-Modified")), pragma(builder.add("Pragma")), cfCacheNamespace(builder.add("CF-Cache-Namespace")), range(builder.add("Range")), diff --git a/src/workerd/io/io-thread-context.h b/src/workerd/io/io-thread-context.h index bec5565123e..3217573efe9 100644 --- a/src/workerd/io/io-thread-context.h +++ b/src/workerd/io/io-thread-context.h @@ -17,10 +17,6 @@ class ThreadContext { const kj::HttpHeaderId contentEncoding; const kj::HttpHeaderId cfCacheStatus; // used by cache API implementation const kj::HttpHeaderId cacheControl; - const kj::HttpHeaderId cacheTag; - const kj::HttpHeaderId etag; - const kj::HttpHeaderId expires; - const kj::HttpHeaderId lastModified; const kj::HttpHeaderId pragma; const kj::HttpHeaderId cfCacheNamespace; // used by Cache binding implementation const kj::HttpHeaderId range; From 042f8d808ec020944841c51753dcbe56b2d2222f Mon Sep 17 00:00:00 2001 From: Jeremy Morrell Date: Thu, 23 Oct 2025 15:27:58 -0700 Subject: [PATCH 9/9] Rename attribute --- src/workerd/api/cache-instrumentation-test.js | 4 ++-- src/workerd/api/cache.c++ | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/workerd/api/cache-instrumentation-test.js b/src/workerd/api/cache-instrumentation-test.js index 2cbf3fedc21..62d18d2e5a8 100644 --- a/src/workerd/api/cache-instrumentation-test.js +++ b/src/workerd/api/cache-instrumentation-test.js @@ -61,7 +61,7 @@ export const test = { }, { name: 'cache_delete', - 'cache.ignore_method': true, + 'cache.request.ignore_method': true, 'cache.request.url': 'https://example.com/delete-with-options', 'cache.response.status_code': 404n, 'cache.response.success': false, @@ -196,7 +196,7 @@ export const test = { }, { name: 'cache_match', - 'cache.ignore_method': false, + 'cache.request.ignore_method': false, 'cache.request.url': 'https://example.com/cached-with-options', 'cache.request.method': 'GET', 'cache.response.status_code': 504n, diff --git a/src/workerd/api/cache.c++ b/src/workerd/api/cache.c++ index eb01c7e535c..9e981ce0c9a 100644 --- a/src/workerd/api/cache.c++ +++ b/src/workerd/api/cache.c++ @@ -82,7 +82,7 @@ jsg::Promise>> Cache::match(jsg::Lock& js, KJ_IF_SOME(o, options) { KJ_IF_SOME(ignoreMethod, o.ignoreMethod) { - traceContext.userSpan.setTag("cache.ignore_method"_kjc, ignoreMethod); + traceContext.userSpan.setTag("cache.request.ignore_method"_kjc, ignoreMethod); } } @@ -561,7 +561,7 @@ jsg::Promise Cache::delete_(jsg::Lock& js, KJ_IF_SOME(o, options) { KJ_IF_SOME(ignoreMethod, o.ignoreMethod) { - traceContext.userSpan.setTag("cache.ignore_method"_kjc, ignoreMethod); + traceContext.userSpan.setTag("cache.request.ignore_method"_kjc, ignoreMethod); } }