Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: memoize blobs requests in the request scope #2777

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor: create reusable helper to record warnings
pieh committed Mar 20, 2025
commit 2f7dee1bdc2ab01fef32807cea9a481357540a32
18 changes: 5 additions & 13 deletions src/run/handlers/cache.cts
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ import {
} from '../regional-blob-store.cjs'

import { getLogger, getRequestContext } from './request-context.cjs'
import { getTracer } from './tracer.cjs'
import { getTracer, recordWarning } from './tracer.cjs'

type TagManifestBlobCache = Record<string, Promise<TagManifest | null>>

@@ -85,13 +85,7 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {
if (!requestContext) {
// we will not be able to use request context for date header calculation
// we will fallback to using blobs
getCacheKeySpan.recordException(
new Error('CacheHandler was called without a request context'),
)
getCacheKeySpan.setAttributes({
severity: 'alert',
warning: true,
})
recordWarning(new Error('CacheHandler was called without a request context'), getCacheKeySpan)
return
}

@@ -100,15 +94,13 @@ export class NetlifyCacheHandler implements CacheHandlerForMultipleVersions {
// so as a safety measure we will not use any of them and let blobs be used
// to calculate the date header
requestContext.responseCacheGetLastModified = undefined
getCacheKeySpan.recordException(
recordWarning(
new Error(
`Multiple response cache keys used in single request: ["${requestContext.responseCacheKey}, "${key}"]`,
),
getCacheKeySpan,
)
getCacheKeySpan.setAttributes({
severity: 'alert',
warning: true,
})

return
}

13 changes: 13 additions & 0 deletions src/run/handlers/tracer.cts
Original file line number Diff line number Diff line change
@@ -89,3 +89,16 @@ export function getTracer(): RuntimeTracer {

return tracer
}

export function recordWarning(warning: Error, span?: Span) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getCacheKeySpan looked to be non-nullable, can we make span non-nullable here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recordWarning(
new Error(
`Blob size calculation did fallback to JSON.stringify. Kind: KnownKindFailed: ${knownKindFailed}, ${valueToStore.value?.kind ?? 'undefined'}`,
),
)
this call doesn't have access to span to pass it, so that's why I made it optional.

Generally preferably we do pass span explicitly instead of getting "active span", because it's more stable on which span things would be recorded, but it's better to record at all than skip recording because we don't have reference to a span.

Alternative could be to make above linked code get active span and then make span here non-nullable, but I did want to simplify usage of it, but would be fine making that change (less "magic")

Copy link
Contributor Author

@pieh pieh Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did attempt the alternative but to me it moved too much tracing related logic and checks to module that doesn't already have reference to containing span I mentioned above and concern of finding active span seems much more in tracer.cts module than modules that might want to record warnings, so I'd prefer to keep the signature of this function as it is

const spanToRecordWarningOn = span ?? trace.getActiveSpan()
if (!spanToRecordWarningOn) {
return
}

spanToRecordWarningOn.recordException(warning)
spanToRecordWarningOn.setAttributes({
severity: 'alert',
warning: true,
})
}
16 changes: 6 additions & 10 deletions src/run/headers.ts
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ import type { NextConfigComplete } from 'next/dist/server/config-shared.js'
import type { NetlifyCachedRouteValue, NetlifyCacheHandlerValue } from '../shared/cache-types.cjs'

import { getLogger, RequestContext } from './handlers/request-context.cjs'
import { recordWarning } from './handlers/tracer.cjs'
import { getMemoizedKeyValueStoreBackedByRegionalBlobStore } from './regional-blob-store.cjs'

const ALL_VARIATIONS = Symbol.for('ALL_VARIATIONS')
@@ -145,13 +146,10 @@ export const adjustDateHeader = async ({
// request context would contain lastModified value
// this is not fatal as we have fallback,
// but we want to know about it happening
span.recordException(
recordWarning(
new Error('lastModified not found in requestContext, falling back to trying blobs'),
span,
)
span.setAttributes({
severity: 'alert',
warning: true,
})

const cacheStore = getMemoizedKeyValueStoreBackedByRegionalBlobStore({ consistency: 'strong' })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming note - perhaps getMemoizedRegionalBlobStore or getRegionalBlobService?

const cacheEntry = await cacheStore.get<NetlifyCacheHandlerValue>(
@@ -164,15 +162,13 @@ export const adjustDateHeader = async ({
if (!lastModified) {
// this should never happen as we only execute this code path for cached responses
// and those should always have lastModified value
span.recordException(
recordWarning(
new Error(
'lastModified not found in either requestContext or blobs, date header for cached response is not set',
),
span,
)
span.setAttributes({
severity: 'alert',
warning: true,
})

return
}