diff --git a/.changeset/olive-donkeys-relax.md b/.changeset/olive-donkeys-relax.md new file mode 100644 index 000000000000..aa13842c4859 --- /dev/null +++ b/.changeset/olive-donkeys-relax.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: keep one request event identity across `handle`, `load` and `handleFetch` diff --git a/packages/kit/src/exports/hooks/sequence.js b/packages/kit/src/exports/hooks/sequence.js index 892dc973fbd4..b569a07f4daf 100644 --- a/packages/kit/src/exports/hooks/sequence.js +++ b/packages/kit/src/exports/hooks/sequence.js @@ -1,9 +1,5 @@ /** @import { Handle, RequestEvent, ResolveOptions } from '@sveltejs/kit' */ -import { - merge_tracing, - get_request_store, - with_request_store -} from '@sveltejs/kit/internal/server'; +import { get_request_store, with_request_store } from '@sveltejs/kit/internal/server'; /** * A helper function for sequencing multiple `handle` calls in a middleware-like manner. @@ -98,11 +94,10 @@ export function sequence(...handlers) { return state.tracing.record_span({ name: `sveltekit.handle.sequenced.${handle.name ? handle.name : i}`, attributes: {}, - fn: async (current) => { - const traced_event = merge_tracing(event, current); - return await with_request_store({ event: traced_event, state }, () => + fn: async () => + with_request_store({ event, state }, () => handle({ - event: traced_event, + event, resolve: (event, options) => { /** @type {ResolveOptions['transformPageChunk']} */ const transformPageChunk = async ({ html, done }) => { @@ -138,8 +133,7 @@ export function sequence(...handlers) { }); } }) - ); - } + ) }); } }; diff --git a/packages/kit/src/exports/internal/server/index.js b/packages/kit/src/exports/internal/server/index.js index 2fe6434ae3fe..557eda303c18 100644 --- a/packages/kit/src/exports/internal/server/index.js +++ b/packages/kit/src/exports/internal/server/index.js @@ -1,4 +1,3 @@ -/** @import { Span } from '@opentelemetry/api' */ import { try_get_request_store } from './event.js'; export function get_origin() { @@ -7,22 +6,6 @@ export function get_origin() { return request && new URL(request.url).origin; } -/** - * @template {{ tracing: { enabled: boolean, root: Span, current: Span } }} T - * @param {T} event_like - * @param {Span} current - * @returns {T} - */ -export function merge_tracing(event_like, current) { - return { - ...event_like, - tracing: { - ...event_like.tracing, - current - } - }; -} - export { with_request_store, getRequestEvent, diff --git a/packages/kit/src/runtime/server/data/index.js b/packages/kit/src/runtime/server/data/index.js index e65fbe5c0230..215af27cb858 100644 --- a/packages/kit/src/runtime/server/data/index.js +++ b/packages/kit/src/runtime/server/data/index.js @@ -5,7 +5,6 @@ import { once } from '../../../utils/functions.js'; import { server_data_serializer_json } from '../page/data_serializer.js'; import { load_server_data } from '../page/load_data.js'; import { handle_error_and_jsonify } from '../errors.js'; -import { normalize_path } from '../../../utils/url.js'; import { text_encoder } from '../../utils.js'; import { with_version_header } from '../utils.js'; @@ -17,7 +16,6 @@ import { with_version_header } from '../utils.js'; * @param {import('@sveltejs/kit').SSRManifest} manifest * @param {import('types').SSRState} state * @param {boolean[] | undefined} invalidated_data_nodes - * @param {import('types').TrailingSlash} trailing_slash * @returns {Promise} */ export async function render_data( @@ -27,8 +25,7 @@ export async function render_data( options, manifest, state, - invalidated_data_nodes, - trailing_slash + invalidated_data_nodes ) { if (!route.page) { // requesting /__data.json should fail for a +server.js @@ -41,11 +38,6 @@ export async function render_data( let aborted = false; - const url = new URL(event.url); - url.pathname = normalize_path(url.pathname, trailing_slash); - - const new_event = { ...event, url }; - const functions = node_ids.map((n, i) => { return once(async () => { try { @@ -59,7 +51,7 @@ export async function render_data( const node = n == undefined ? n : await manifest._.nodes[n](); // load this. for the child, return as is. for the final result, stream things return load_server_data({ - event: new_event, + event, event_state, state, node, diff --git a/packages/kit/src/runtime/server/page/actions.js b/packages/kit/src/runtime/server/page/actions.js index 315fff550560..7aa203e5487a 100644 --- a/packages/kit/src/runtime/server/page/actions.js +++ b/packages/kit/src/runtime/server/page/actions.js @@ -4,7 +4,7 @@ import * as devalue from 'devalue'; import { DEV } from 'esm-env'; import { json } from '@sveltejs/kit'; import { HttpError, Redirect, ActionFailure, SvelteKitError } from '@sveltejs/kit/internal'; -import { with_request_store, merge_tracing } from '@sveltejs/kit/internal/server'; +import { with_request_store } from '@sveltejs/kit/internal/server'; import { normalize_error } from '../../../utils/error.js'; import { is_form_content_type, negotiate } from '../../../utils/http.js'; import { create_replacer, with_version_header } from '../utils.js'; @@ -281,11 +281,7 @@ async function call_action(event, event_state, actions) { 'http.route': event.route.id || 'unknown' }, fn: async (current) => { - const traced_event = merge_tracing(event, current); - - const result = await with_request_store({ event: traced_event, state: event_state }, () => - action(traced_event) - ); + const result = await with_request_store({ event, state: event_state }, () => action(event)); if (result instanceof ActionFailure) { current.setAttributes({ diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 67c9c3c37c07..3d9d8027d63a 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -2,7 +2,7 @@ import { DEV } from 'esm-env'; import { noop } from '../../../utils/functions.js'; import { disable_search, make_trackable } from '../../../utils/url.js'; import { validate_depends, validate_load_response } from '../../shared.js'; -import { with_request_store, merge_tracing } from '@sveltejs/kit/internal/server'; +import { with_request_store } from '@sveltejs/kit/internal/server'; import { record_span } from '../../telemetry/record_span.js'; import { base64_encode } from '../../utils.js'; import { NULL_BODY_STATUS } from '../constants.js'; @@ -81,11 +81,10 @@ export async function load_server_data({ event, event_state, state, node, parent 'sveltekit.load.environment': 'server', 'http.route': event.route.id || 'unknown' }, - fn: async (current) => { - const traced_event = merge_tracing(event, current); - const result = await with_request_store({ event: traced_event, state: event_state }, () => + fn: async () => + with_request_store({ event, state: event_state }, () => load.call(null, { - ...traced_event, + ...event, fetch: (info, init) => { const url = new URL(info instanceof Request ? info.url : info, event.url); @@ -170,10 +169,7 @@ export async function load_server_data({ event, event_state, state, node, parent } } }) - ); - - return result; - } + ) }); if (DEV) { @@ -232,11 +228,10 @@ export async function load_data({ 'sveltekit.load.environment': 'server', 'http.route': event.route.id || 'unknown' }, - fn: async (current) => { - const traced_event = merge_tracing(event, current); + fn: async () => { const child_state = { ...event_state, is_in_universal_load: true }; - return await with_request_store({ event: traced_event, state: child_state }, () => + return await with_request_store({ event, state: child_state }, () => load.call(null, { url: event.url, params: event.params, @@ -247,7 +242,7 @@ export async function load_data({ depends: noop, parent, untrack: (fn) => fn(), - tracing: traced_event.tracing + tracing: event.tracing }) ); } diff --git a/packages/kit/src/runtime/server/remote-functions.js b/packages/kit/src/runtime/server/remote-functions.js index 08e0f988aaa5..d99180e0bc92 100644 --- a/packages/kit/src/runtime/server/remote-functions.js +++ b/packages/kit/src/runtime/server/remote-functions.js @@ -3,7 +3,7 @@ import { json, error } from '@sveltejs/kit'; import { Redirect, SvelteKitError } from '@sveltejs/kit/internal'; -import { with_request_store, merge_tracing } from '@sveltejs/kit/internal/server'; +import { with_request_store } from '@sveltejs/kit/internal/server'; import { app_dir, base } from '$app/paths/internal/server'; import { is_form_content_type } from '../../utils/http.js'; import { create_remote_key, parse_remote_arg, split_remote_key, stringify } from '../shared.js'; @@ -29,10 +29,9 @@ export async function handle_remote_call(event, state, options, manifest, id) { attributes: { 'sveltekit.remote.call.id': id }, - fn: async (current) => { - const traced_event = merge_tracing(event, current); - const response = await with_request_store({ event: traced_event, state }, () => - handle_remote_call_internal(traced_event, state, options, manifest, id) + fn: async () => { + const response = await with_request_store({ event, state }, () => + handle_remote_call_internal(event, state, options, manifest, id) ); return with_version_header(response); } @@ -520,12 +519,10 @@ export async function handle_remote_form_post(event, state, manifest, id) { attributes: { 'sveltekit.remote.form.post.id': id }, - fn: (current) => { - const traced_event = merge_tracing(event, current); - return with_request_store({ event: traced_event, state }, () => - handle_remote_form_post_internal(traced_event, state, manifest, id) - ); - } + fn: () => + with_request_store({ event, state }, () => + handle_remote_form_post_internal(event, state, manifest, id) + ) }); } diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index a575adae62ac..b3783065ae50 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -2,7 +2,7 @@ import { DEV } from 'esm-env'; import { json, text } from '@sveltejs/kit'; import { Redirect, SvelteKitError } from '@sveltejs/kit/internal'; -import { merge_tracing, with_request_store } from '@sveltejs/kit/internal/server'; +import { with_request_store } from '@sveltejs/kit/internal/server'; import { base, app_dir } from '$app/paths/internal/server'; import { is_endpoint_request, render_endpoint } from './endpoint.js'; import { render_page } from './page/index.js'; @@ -39,7 +39,7 @@ import { import { server_data_serializer } from './page/data_serializer.js'; import { get_remote_id, handle_remote_call } from './remote-functions.js'; import { record_span } from '../telemetry/record_span.js'; -import { otel } from '../telemetry/otel.js'; +import { otel, trace } from '../telemetry/otel.js'; /** @type {import('types').RequiredResolveOptions['transformPageChunk']} */ const default_transform = ({ html }) => html; @@ -204,6 +204,8 @@ export async function internal_respond(request, options, manifest, state) { cookies, // @ts-expect-error `fetch` needs to be created after the `event` itself fetch: null, + // @ts-expect-error `tracing` needs the root span, which is created during `handle` + tracing: null, getClientAddress: state.getClientAddress || (() => { @@ -421,6 +423,9 @@ export async function internal_respond(request, options, manifest, state) { } }); } + } else { + // a data request can't follow a redirect, so normalize the path in place + url.pathname = normalize_path(url.pathname, trailing_slash); } if (state.before_handle || state.emulator?.platform) { @@ -484,18 +489,17 @@ export async function internal_respond(request, options, manifest, state) { 'sveltekit.is_sub_request': event.isSubRequest }, fn: async (root_span) => { - const traced_event = { - ...event, - tracing: { - enabled: __SVELTEKIT_SERVER_TRACING_ENABLED__, - root: root_span, - current: root_span + event.tracing = { + enabled: __SVELTEKIT_SERVER_TRACING_ENABLED__, + root: root_span, + get current() { + return trace?.getActiveSpan() ?? root_span; } }; - return await with_request_store({ event: traced_event, state: event_state }, () => + return await with_request_store({ event, state: event_state }, () => options.hooks.handle({ - event: traced_event, + event, resolve: (event, opts) => { return record_span({ name: 'sveltekit.resolve', @@ -506,30 +510,28 @@ export async function internal_respond(request, options, manifest, state) { // counter-intuitively, we need to clear the event, so that it's not // e.g. accessible when loading modules needed to handle the request return with_request_store(null, () => - resolve(merge_tracing(event, resolve_span), page_nodes, opts).then( - (response) => { - // add headers/cookies here, rather than inside `resolve`, so that we - // can do it once for all responses instead of once per `return` - for (const key in headers) { - const value = headers[key]; - response.headers.set(key, /** @type {string} */ (value)); - } - - add_cookies_to_headers(response.headers, new_cookies.values()); - - if (state.prerendering && event.route.id !== null) { - response.headers.set('x-sveltekit-routeid', encodeURI(event.route.id)); - } - - resolve_span.setAttributes({ - 'http.response.status_code': response.status, - 'http.response.body.size': - response.headers.get('content-length') || 'unknown' - }); - - return response; + resolve(event, page_nodes, opts).then((response) => { + // add headers/cookies here, rather than inside `resolve`, so that we + // can do it once for all responses instead of once per `return` + for (const key in headers) { + const value = headers[key]; + response.headers.set(key, /** @type {string} */ (value)); + } + + add_cookies_to_headers(response.headers, new_cookies.values()); + + if (state.prerendering && event.route.id !== null) { + response.headers.set('x-sveltekit-routeid', encodeURI(event.route.id)); } - ) + + resolve_span.setAttributes({ + 'http.response.status_code': response.status, + 'http.response.body.size': + response.headers.get('content-length') || 'unknown' + }); + + return response; + }) ); } }); @@ -655,8 +657,7 @@ export async function internal_respond(request, options, manifest, state) { options, manifest, state, - invalidated_data_nodes, - trailing_slash + invalidated_data_nodes ); } else { let endpoint; diff --git a/packages/kit/src/runtime/telemetry/otel.js b/packages/kit/src/runtime/telemetry/otel.js index a293423ebade..1b92b4a2682c 100644 --- a/packages/kit/src/runtime/telemetry/otel.js +++ b/packages/kit/src/runtime/telemetry/otel.js @@ -1,11 +1,19 @@ -/** @import { Tracer, SpanStatusCode, PropagationAPI, ContextAPI } from '@opentelemetry/api' */ +/** @import { Tracer, SpanStatusCode, PropagationAPI, ContextAPI, TraceAPI } from '@opentelemetry/api' */ /** @type {Promise<{ tracer: Tracer, SpanStatusCode: typeof SpanStatusCode, propagation: PropagationAPI, context: ContextAPI }> | null} */ export let otel = null; +/** + * Synchronously readable once `otel` has resolved, which is guaranteed + * before any span exists — `record_span` awaits `otel` first. + * @type {TraceAPI | null} + */ +export let trace = null; + if (__SVELTEKIT_SERVER_TRACING_ENABLED__) { otel = import('@opentelemetry/api') .then((module) => { + trace = module.trace; return { tracer: module.trace.getTracer('sveltekit'), propagation: module.propagation, diff --git a/packages/kit/test/apps/basics/src/hooks.server.js b/packages/kit/test/apps/basics/src/hooks.server.js index a0ac97c57f40..df4ad98a4b1b 100644 --- a/packages/kit/test/apps/basics/src/hooks.server.js +++ b/packages/kit/test/apps/basics/src/hooks.server.js @@ -193,7 +193,8 @@ export const handle = sequence( throw new Error('event !== e'); } - e.locals.message = 'hello from hooks.server.js'; + // reassignment, not mutation: only visible in handleFetch if the event is never copied + e.locals = { ...e.locals, message: 'hello from hooks.server.js' }; } return resolve(event, { @@ -204,7 +205,11 @@ export const handle = sequence( ); /** @type {import('@sveltejs/kit').HandleFetch} */ -export async function handleFetch({ request, fetch }) { +export async function handleFetch({ event, request, fetch }) { + if (event.url.pathname.startsWith('/get-request-event/via-')) { + request.headers.set('x-message', event.locals.message ?? 'missing'); + } + if (request.url.endsWith('/server-fetch-request.json')) { request = new Request( request.url.replace('/server-fetch-request.json', '/server-fetch-request-modified.json'), diff --git a/packages/kit/test/apps/basics/src/routes/get-request-event/via-data/+page.server.js b/packages/kit/test/apps/basics/src/routes/get-request-event/via-data/+page.server.js new file mode 100644 index 000000000000..6e412e919fb3 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/get-request-event/via-data/+page.server.js @@ -0,0 +1,10 @@ +import { getRequestEvent } from '$app/server'; + +/** @type {import('./$types').PageServerLoad} */ +export async function load({ fetch }) { + const event = getRequestEvent(); + // reassignment, not mutation: only visible in handleFetch if the store event is never a copy + event.locals = { ...event.locals, message: 'hello from the server load' }; + const res = await fetch('/headers/echo'); + return { message: (await res.json())['x-message'] }; +} diff --git a/packages/kit/test/apps/basics/src/routes/get-request-event/via-data/+page.svelte b/packages/kit/test/apps/basics/src/routes/get-request-event/via-data/+page.svelte new file mode 100644 index 000000000000..59b61bf1447a --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/get-request-event/via-data/+page.svelte @@ -0,0 +1,5 @@ + + +

{data.message}

diff --git a/packages/kit/test/apps/basics/src/routes/get-request-event/via-fetch/+page.server.js b/packages/kit/test/apps/basics/src/routes/get-request-event/via-fetch/+page.server.js new file mode 100644 index 000000000000..138899c5469c --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/get-request-event/via-fetch/+page.server.js @@ -0,0 +1,5 @@ +/** @type {import('./$types').PageServerLoad} */ +export async function load({ fetch }) { + const res = await fetch('/headers/echo'); + return { message: (await res.json())['x-message'] }; +} diff --git a/packages/kit/test/apps/basics/src/routes/get-request-event/via-fetch/+page.svelte b/packages/kit/test/apps/basics/src/routes/get-request-event/via-fetch/+page.svelte new file mode 100644 index 000000000000..59b61bf1447a --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/get-request-event/via-fetch/+page.svelte @@ -0,0 +1,5 @@ + + +

{data.message}

diff --git a/packages/kit/test/apps/basics/src/routes/tracing/current/+page.server.js b/packages/kit/test/apps/basics/src/routes/tracing/current/+page.server.js new file mode 100644 index 000000000000..5290f2b106be --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/tracing/current/+page.server.js @@ -0,0 +1,12 @@ +import { trace } from '@opentelemetry/api'; + +/** @type {import('./$types').PageServerLoad} */ +export async function load({ tracing }) { + tracing.current.setAttribute('current_matches_otel', trace.getActiveSpan() === tracing.current); + await Promise.resolve(); + tracing.current.setAttribute( + 'current_matches_otel_after_await', + trace.getActiveSpan() === tracing.current + ); + return { ok: true }; +} diff --git a/packages/kit/test/apps/basics/src/routes/tracing/current/+page.svelte b/packages/kit/test/apps/basics/src/routes/tracing/current/+page.svelte new file mode 100644 index 000000000000..abff1999b5cd --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/tracing/current/+page.svelte @@ -0,0 +1,5 @@ + + +

{data.ok}

diff --git a/packages/kit/test/apps/basics/test/server.test.js b/packages/kit/test/apps/basics/test/server.test.js index 1eef97fc5bf9..dd5a0927a6b4 100644 --- a/packages/kit/test/apps/basics/test/server.test.js +++ b/packages/kit/test/apps/basics/test/server.test.js @@ -1088,22 +1088,25 @@ test.describe('$app/env', () => { }); test.describe('tracing', () => { - // Helper function to find the resolve.root span deep in the handle.child chain /** * @param {import('../../../types.js').SpanTree} span + * @param {(span: import('../../../types.js').SpanTree) => boolean} predicate * @returns {import('../../../types.js').SpanTree | null} */ - function find_resolve_root_span(span) { - if (span.name === 'sveltekit.resolve') { + function find_span(span, predicate) { + if (predicate(span)) { return span; } for (const child of span.children || []) { - const found = find_resolve_root_span(child); + const found = find_span(child, predicate); if (found) return found; } return null; } + /** @param {import('../../../types.js').SpanTree} span */ + const find_resolve_root_span = (span) => find_span(span, (s) => s.name === 'sveltekit.resolve'); + function rand() { // node 18 doesn't have crypto.randomUUID() and we run tests in node 18 return Math.random().toString(36).substring(2, 15); @@ -1199,6 +1202,23 @@ test.describe('tracing', () => { }); }); + test('tracing.current in a server load is the load span', async ({ page, read_traces }) => { + const test_id = rand(); + await page.goto(`/tracing/current?test_id=${test_id}`); + const traces = read_traces(test_id); + expect(traces.length).toBeGreaterThan(0); + + // the attributes set via tracing.current land on the load span, not the root + const load_span = find_span( + traces[0], + (s) => s.attributes['sveltekit.load.node_id'] === 'src/routes/tracing/current/+page.server.js' + ); + expect(load_span).not.toBeNull(); + expect(load_span?.attributes.current_matches_otel).toBe(true); + expect(load_span?.attributes.current_matches_otel_after_await).toBe(true); + expect(traces[0].attributes.current_matches_otel).toBeUndefined(); + }); + test('correct spans are created for HttpError', async ({ page, read_traces }) => { const test_id = rand(); const response = await page.goto(`/tracing/http-error?test_id=${test_id}`); diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index de04d97a9331..cc1ede4dd4ee 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -1604,6 +1604,23 @@ test.describe('getRequestEvent', () => { await page.goto('/get-request-event/with-error'); expect(await page.textContent('h1')).toBe('Crashing now (500 hello from hooks.server.js)'); }); + + test('handleFetch sees what handle wrote to the event', async ({ page }) => { + await page.goto('/get-request-event/via-fetch'); + expect(await page.textContent('h1')).toBe('hello from hooks.server.js'); + }); + + test('handleFetch sees what a server load wrote during a data request', async ({ + app, + page, + javaScriptEnabled + }) => { + if (!javaScriptEnabled) return; + + await page.goto('/get-request-event/via-fetch'); + await app.goto('/get-request-event/via-data'); + expect(await page.textContent('h1')).toBe('hello from the server load'); + }); }); test.describe('params prop', () => {