feat: Implementing trace of graphql executions#3215
Open
ommeirelles wants to merge 9 commits intocanary-v2from
Open
feat: Implementing trace of graphql executions#3215ommeirelles wants to merge 9 commits intocanary-v2from
ommeirelles wants to merge 9 commits intocanary-v2from
Conversation
ommeirelles
requested review from
hellofanny and
renatomaurovtex
and removed request for
a team
WalkthroughAdds a new diagnostics package and integrates OpenTelemetry tracing across the API and core: context now carries OTEL and an id, resolvers are wrapped with ResolverTrace, trace clients are initialized at runtime, and build/test code updated to await async context creation. (48 words) Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL as GraphQL Server
participant ContextFactory as Context Factory
participant ResolverTrace as ResolverTrace Wrapper
participant Tracer as Trace Client (OTEL)
participant Resolver as Original Resolver
Client->>GraphQL: Send GraphQL request
GraphQL->>ContextFactory: Build context (await)
ContextFactory->>GraphQL: Return context with id + OTEL
GraphQL->>ResolverTrace: Invoke wrapped resolver
ResolverTrace->>Tracer: Extract/activate context, start span
ResolverTrace->>Resolver: Execute original resolver
Resolver-->>ResolverTrace: Return (sync or Promise)
alt async result
Resolver-->>ResolverTrace: Promise resolves
end
ResolverTrace->>Tracer: End span
ResolverTrace-->>GraphQL: Return result
GraphQL-->>Client: Respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
ommeirelles
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/platforms/vtex/index.ts (1)
43-65:⚠️ Potential issue | 🔴 CriticalBug:
idis generated once per factory, not per request — all requests share the same ID.
crypto.randomBytes(32)on line 49 executes in the outer factory scope. The inner closure (line 50) is what runs per-request, but it reuses the sameid. This contradicts the "Request scope ID" comment on line 21. Moveidgeneration inside the returned function:Proposed fix
export const GraphqlVtexContextFactory = (options: Options) => { getTelemetryClient({ name, version, account: options.account, }) - const id = crypto.randomBytes(32).toString('hex') return (ctx: any): GraphqlContext => { - ctx.id = id + ctx.id = crypto.randomBytes(32).toString('hex') ctx.storage = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 43 - 65, The factory GraphqlVtexContextFactory currently generates id once in the outer scope (const id = crypto.randomBytes(32).toString('hex')) so every request shares the same id; move the id generation into the returned function so that inside the returned (ctx: any): GraphqlContext => { ... } you create a fresh id (e.g., const id = crypto.randomBytes(32).toString('hex')) and then assign ctx.id = id, removing the outer const id to ensure a unique request-scoped id for each ctx.
🧹 Nitpick comments (9)
packages/diagnostics/vite.config.mts (1)
21-23:dependencies ?? {}is a redundant null guard.
dependenciesis already initialized with= {}on line 6, so it can never beundefinedhere.♻️ Proposed fix
external: [ ...builtinModules.concat(builtinModules.map((e) => `node:${e}`)), - ...Object.keys({ - ...(dependencies ?? {}), - }), + ...Object.keys(dependencies), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/vite.config.mts` around lines 21 - 23, The spread uses a redundant nullish fallback because the variable dependencies is initialized to {} earlier; update the Object.keys spread to use dependencies directly (remove the "?? {}" guard) so the line becomes Object.keys({ ...(dependencies) }) or simply Object.keys(dependencies) as appropriate, ensuring you reference the existing dependencies variable used on line 6 and the Object.keys(...) expression to locate and modify the code.packages/diagnostics/tsconfig.json (1)
4-4:outDir: "dist/esm"doesn't match Vite build output paths.The Vite config outputs to
dist/es/anddist/cjs/, notdist/esm/. Sincevite-plugin-dtsdrives the declaration output (nottsc), this is non-functional, but it's misleading for anyone running a baretsccheck on this package.♻️ Suggested fix
- "outDir": "dist/esm", + "outDir": "dist",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/tsconfig.json` at line 4, The tsconfig.json currently sets "outDir": "dist/esm" which mismatches the Vite outputs and is misleading; update the tsconfig.json's outDir key to "dist/es" to match Vite's ES output (or remove the outDir entry entirely if you prefer to avoid any tsc emit assumptions since vite-plugin-dts drives declaration output) so that the "outDir" setting reflects the actual build paths used by the project.packages/api/src/typings/index.ts (1)
32-37:infoparameter should beGraphQLResolveInfo, notany.
infocarries the full GraphQL resolution context (field name, parent type, path, schema, variables). Typing it asanyopts out of type safety for every caller and forfeits valuable metadata for tracing.GraphQLResolveInfois available through the existinggraphqlpeer dependency.♻️ Proposed fix
+import type { GraphQLResolveInfo } from 'graphql' type Resolver< TContext extends Record<string, any>, TSource = any, TVars = any, TReturn = any, -> = (source: TSource, vars: TVars, context: TContext, info: any) => TReturn +> = (source: TSource, vars: TVars, context: TContext, info: GraphQLResolveInfo) => TReturnAs per coding guidelines: "Type safety with generated types" and "Ensure type safety and avoid type assertions when possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/typings/index.ts` around lines 32 - 37, The Resolver type currently types the fourth parameter as any; change it to GraphQLResolveInfo to preserve GraphQL metadata and enable type safety: import { GraphQLResolveInfo } from "graphql" at the top of the file and update the Resolver signature to (source: TSource, vars: TVars, context: TContext, info: GraphQLResolveInfo) => TReturn (keeping the generics intact); ensure the graphql peer dependency is used (no type assertions needed) so callers get proper info typing.packages/diagnostics/package.json (1)
12-12:license: "ISC"is inconsistent with the rest of the monorepo.Every other
@faststore/*package uses"MIT". Consider aligning unless ISC is deliberate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/package.json` at line 12, The package's license field in its package.json is set to "ISC" while the monorepo uses "MIT"; update the "license" property from "ISC" to "MIT" in this package's package.json so it matches other `@faststore` packages, then run your repo's license/validation script (or npm install/test) to confirm no further inconsistencies.packages/api/src/platforms/vtex/index.ts (2)
67-71: Remove commented-out code.Dead code adds noise. If
VtexResolveris needed later, it can be retrieved from git history.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 67 - 71, Remove the commented-out VtexResolver block: delete the entire commented code that defines VtexResolver (the lines referencing VtexResolver, GraphqlResolver, ResolverTrace, and GraphqlContext) so the file no longer contains this dead code; if needed later it can be restored from git history.
80-96:finalResolvers: anydrops type safety; variablenameshadows module-level import.Two concerns in
getResolvers:
- Line 83:
finalResolvers: anydiscards the resolver type structure. Use a more descriptive intermediate type or infer fromnativeGetResolvers().- Line 86: The destructured
namein.map(([name, resolverFunction])shadows thenameimport frompackage.json(line 5). Rename to e.g.resolverNamefor clarity.Proposed fix
export function getResolvers() { const resolvers = nativeGetResolvers() - const finalResolvers: any = {} + const finalResolvers: Record<string, Record<string, unknown>> = {} for (const [key, resolver] of Object.entries(resolvers)) { finalResolvers[key] = Object.fromEntries( - Object.entries(resolver).map(([name, resolverFunction]) => { - if (typeof resolverFunction !== 'function') - return [name, resolverFunction] + Object.entries(resolver).map(([resolverName, resolverFunction]) => { + if (typeof resolverFunction !== 'function') + return [resolverName, resolverFunction] - return [name, ResolverTrace(resolverFunction, `${key}(${name})`)] + return [resolverName, ResolverTrace(resolverFunction, `${key}(${resolverName})`)] }) ) }As per coding guidelines, "Ensure type safety and avoid type assertions when possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 80 - 96, getResolvers currently weakens types by declaring finalResolvers: any and also shadows the module-level import name via the map destructuring; update getResolvers to infer types from nativeGetResolvers() (e.g., let resolvers = nativeGetResolvers(); use const finalResolvers: Partial<typeof resolvers> = {} or an appropriate mapped type) and replace the destructured variable name in Object.entries(...).map(([name, resolverFunction]) => ...) with a non-conflicting identifier like resolverName, then assign ResolverTrace(resolverFunction, `${key}(${resolverName})`) so type safety is preserved and the imported name is not shadowed.packages/diagnostics/src/index.ts (2)
1-11: Namespace re-exports defeat tree-shaking — significant bundle size impact.
import *of@opentelemetry/apiand@opentelemetry/semantic-conventions, then re-exporting asOTELAPIandCONVENTIONS, forces bundlers to include the entire modules. Downstream code only uses a handful of symbols (context,SpanKind,ATTR_CODE_FUNCTION_NAME,ATTR_SERVICE_NAME,ATTR_SERVICE_VERSION).Consider re-exporting only the specific symbols needed, or let consumers import directly from
@opentelemetry/*packages.As per coding guidelines, "Consider bundle size impact of dependencies".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/index.ts` around lines 1 - 11, The current namespace re-exports (OTELAPI and CONVENTIONS) pull in entire `@opentelemetry` packages and defeat tree-shaking; replace the wildcard imports and re-exports with named exports for only the symbols actually used (e.g., export { context, SpanKind } from '@opentelemetry/api' and export { ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions'), or remove the OTELAPI/CONVENTIONS exports and let consumers import directly; update any internal references to OTELAPI or CONVENTIONS to use the named symbols or the original package imports to restore tree-shaking and reduce bundle size.
13-18: Redundant default export alongside named exports.The default export duplicates everything already available via named exports. This creates two ways to import the same thing with no benefit, and makes tree-shaking harder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/diagnostics/src/index.ts` around lines 13 - 18, Remove the redundant default export object that mirrors the named exports; delete the default export block that returns OTELAPI, CONVENTIONS, getTelemetryClient, and getTraceClient and rely solely on the existing named exports. Ensure the named exports OTELAPI, CONVENTIONS, getTelemetryClient, and getTraceClient are exported where they are defined (or add explicit export statements) so consumers import them by name and tree-shaking works correctly.packages/api/src/observability/telemetry.ts (1)
7-14: Return typeTReturnis inaccurate when wrapping async resolvers.When
fnreturnsPromise<X>(soTReturn = Promise<X>), the.then()on line 49 also producesPromise<X>, so theas TReturncast on line 52 happens to be correct at runtime. However, the outer function's declared return type isTReturn, yetOTELAPI.context.with(activeContext, callback)returns whatever the callback returns — the TypeScript compiler can't verify the span-wrapping logic preserves the type. This is acceptable for now but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/observability/telemetry.ts` around lines 7 - 14, The ResolverTrace wrapper's declared return type TReturn can diverge from the actual return of the wrapped function (especially when the wrapped function returns a Promise); update the ResolverTrace signature to take the resolver function as a generic function type (e.g., F extends (source: TSource, vars: TVars, context: TContext, info: any) => any) and use ReturnType<F> as the wrapper's return type so the compiler preserves the exact return (Promise or non-Promise) of the original fn; locate the ResolverTrace declaration and replace the TReturn generic with an F generic and use ReturnType<F> for the outer function's return type and for internal casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/package.json`:
- Around line 77-78: Remove the unnecessary explicit peerDependency entry for
"@vtex/diagnostics-nodejs" from the package.json for the `@faststore/api` package:
delete the "@vtex/diagnostics-nodejs": "^5.1.1" line from peerDependencies
(since `@faststore/diagnostics` already depends on it and the code in
`@faststore/api` does not import it), then update lockfiles/install
(npm/yarn/pnpm) as needed to ensure the dependency is resolved transitively and
run tests/build to confirm nothing breaks.
In `@packages/api/src/observability/telemetry.ts`:
- Around line 40-52: The span is not ended when fn throws synchronously or when
the returned promise rejects; wrap the invocation of fn (the call that assigns
returnedValue inside the context.with callback) in a try/catch so that
span.end() is called in the catch before rethrowing synchronous errors, and for
the async case replace the current then-only logic with a finally (or attach
both then and catch) on the Promise (e.g.,
Promise.resolve(returnedValue).finally(...)) to ensure span.end() runs on
resolve or rejection; reference the variables/functions returnedValue, fn, span
and the context.with callback when making the changes.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 44-48: getTelemetryClient is called without awaiting so the trace
client (TRACE_CLIENT) isn't initialized before withTraceClient runs; make
GraphqlVtexContextFactory async and await getTelemetryClient(...) inside it so
the telemetry client is ready when GraphqlVtexContextFactory returns, and then
update the caller that invokes this factory (the place where withTraceClient is
composed with the factory) to await the async GraphqlVtexContextFactory so OTEL
context is populated for early requests.
In `@packages/core/package.json`:
- Line 66: Remove the direct dependency entry for "@vtex/diagnostics-nodejs"
from the dependencies list in package.json (the explicit line that reads
"@vtex/diagnostics-nodejs": "^5.1.1") because it is already provided
transitively by "@faststore/diagnostics"; update the dependencies section to
delete that line so only "@faststore/diagnostics" remains responsible for
bringing in that package.
In `@packages/core/src/instrumentation.ts`:
- Around line 1-11: The register function can throw from its dynamic imports
(package.json, discovery.config, or `@faststore/diagnostics`) and must not crash
Next.js on startup; update the async function register to check
process.env.NEXT_RUNTIME as before but wrap the entire import/initialization
block in a try/catch, gracefully short-circuit on error (do not rethrow), and
emit a safe error message (e.g., via console.error or a fallback logger) so
telemetry failures do not prevent the app from starting; reference the register
function and the three dynamic imports (import('../package.json'),
import('../discovery.config'), import('@faststore/diagnostics')) when making the
change.
In `@packages/core/src/server/options.ts`:
- Around line 26-36: Change the generic and return typing and fix the
import/variable typo: make withTraceClient generic constrained to objects (e.g.
function withTraceClient<T extends object>(apiOptions: T): Promise<T & { OTEL:
unknown }>) so spreading ...apiOptions is type-safe and the return reflects the
added OTEL property; remove the useless optional chain on the dynamic import
(use await import('@faststore/diagnostics') instead of (await
import(...))?.getTraceClient()) so import errors propagate, rename traceCLient
to traceClient, call const traceClient = (await import(...)).getTraceClient();
and keep a defensive traceClient?.inject(OTEL) call for when getTraceClient()
returns undefined.
In `@packages/diagnostics/package.json`:
- Around line 1-33: The package.json for `@faststore/diagnostics` is missing a
publishConfig entry which causes npm to publish the scoped package as
restricted; update packages/diagnostics/package.json (package name
"@faststore/diagnostics") to include "publishConfig": { "access": "public" } at
the top level of the JSON so it mirrors other `@faststore/`* packages and can be
published successfully.
In `@packages/diagnostics/src/start.ts`:
- Around line 22-25: The traces exporter is forcing TLS off by hardcoding
insecure: true in the Exporters.CreateTracesExporterConfig call (tracesConfig) —
change this to derive from an environment variable instead (for example check
process.env.NODE_ENV !== 'production' or a dedicated OTLP_INSECURE flag) and
pass that boolean into the insecure option when calling
Exporters.CreateTracesExporterConfig( { endpoint: OTLP_ENDPOINT, insecure:
<derivedBool> } ) so production defaults to secure/TLS while non-production can
disable it via env.
- Line 60: The console.log call that prints "TELEMETRY CLIENT STARTED" with the
opt object should be removed or converted to structured debug logging: replace
the console.log('TELEMETRY CLIENT STARTED', opt) usage with a debug-level logger
(e.g., processLogger.debug or the project's logger) and ensure you do not emit
sensitive fields from the opt object—either omit or redact opt.account before
logging (or only log a non-identifying subset). Locate the console.log in
start.ts where the opt variable is used and change it to a debug-level,
structured log that excludes or masks the account identifier.
- Around line 32-63: getTelemetryClient currently recreates and overwrites the
global TELEMETRY_CLIENT and TRACE_CLIENT on every call; add an idempotency guard
at the start of getTelemetryClient that returns the existing TELEMETRY_CLIENT
immediately if it's already initialized (so you don't call NewTelemetryClient,
setupTracesExporter, newLogsClient/newMetricsClient/newTracesClient or
registerInstrumentations again). Locate getTelemetryClient and reference the
TELEMETRY_CLIENT and TRACE_CLIENT globals; implement an early return using the
same function signature and ensure you do not re-run setupTracesExporter,
NewTelemetryClient, or Instrumentation.CommonInstrumentations.minimal() when
TELEMETRY_CLIENT is present.
- Around line 51-52: The code incorrectly reuses tracesExporter (from
Exporters.CreateTracesExporterConfig) for logs and metrics; replace those calls
to create and pass signal-specific exporters by creating a logsExporter via
Exporters.CreateLogsExporterConfig() and a metricsExporter via
Exporters.CreateMetricsExporterConfig(), then call
TELEMETRY_CLIENT.newLogsClient({ exporter: logsExporter }) and
TELEMETRY_CLIENT.newMetricsClient({ exporter: metricsExporter }) while leaving
the traces client using tracesExporter.
In `@packages/diagnostics/vite.config.mts`:
- Around line 15-17: The Vite library config currently uses fileName:
'[format]/index' which makes Vite emit index.cjs for the 'cjs' format while
package.json's "require" expects ./dist/cjs/index.js; update the Vite config's
fileName to a function that returns a path forcing .js for the 'cjs' format and
.mjs for the ES format (i.e., return 'cjs/index.js' for format === 'cjs' and
'es/index.mjs' otherwise) so the emitted files match package.json, or
alternatively update package.json's "require" to point to ./dist/cjs/index.cjs
if you prefer Vite's default naming.
---
Outside diff comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 43-65: The factory GraphqlVtexContextFactory currently generates
id once in the outer scope (const id = crypto.randomBytes(32).toString('hex'))
so every request shares the same id; move the id generation into the returned
function so that inside the returned (ctx: any): GraphqlContext => { ... } you
create a fresh id (e.g., const id = crypto.randomBytes(32).toString('hex')) and
then assign ctx.id = id, removing the outer const id to ensure a unique
request-scoped id for each ctx.
---
Nitpick comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 7-14: The ResolverTrace wrapper's declared return type TReturn can
diverge from the actual return of the wrapped function (especially when the
wrapped function returns a Promise); update the ResolverTrace signature to take
the resolver function as a generic function type (e.g., F extends (source:
TSource, vars: TVars, context: TContext, info: any) => any) and use
ReturnType<F> as the wrapper's return type so the compiler preserves the exact
return (Promise or non-Promise) of the original fn; locate the ResolverTrace
declaration and replace the TReturn generic with an F generic and use
ReturnType<F> for the outer function's return type and for internal casts.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 67-71: Remove the commented-out VtexResolver block: delete the
entire commented code that defines VtexResolver (the lines referencing
VtexResolver, GraphqlResolver, ResolverTrace, and GraphqlContext) so the file no
longer contains this dead code; if needed later it can be restored from git
history.
- Around line 80-96: getResolvers currently weakens types by declaring
finalResolvers: any and also shadows the module-level import name via the map
destructuring; update getResolvers to infer types from nativeGetResolvers()
(e.g., let resolvers = nativeGetResolvers(); use const finalResolvers:
Partial<typeof resolvers> = {} or an appropriate mapped type) and replace the
destructured variable name in Object.entries(...).map(([name, resolverFunction])
=> ...) with a non-conflicting identifier like resolverName, then assign
ResolverTrace(resolverFunction, `${key}(${resolverName})`) so type safety is
preserved and the imported name is not shadowed.
In `@packages/api/src/typings/index.ts`:
- Around line 32-37: The Resolver type currently types the fourth parameter as
any; change it to GraphQLResolveInfo to preserve GraphQL metadata and enable
type safety: import { GraphQLResolveInfo } from "graphql" at the top of the file
and update the Resolver signature to (source: TSource, vars: TVars, context:
TContext, info: GraphQLResolveInfo) => TReturn (keeping the generics intact);
ensure the graphql peer dependency is used (no type assertions needed) so
callers get proper info typing.
In `@packages/diagnostics/package.json`:
- Line 12: The package's license field in its package.json is set to "ISC" while
the monorepo uses "MIT"; update the "license" property from "ISC" to "MIT" in
this package's package.json so it matches other `@faststore` packages, then run
your repo's license/validation script (or npm install/test) to confirm no
further inconsistencies.
In `@packages/diagnostics/src/index.ts`:
- Around line 1-11: The current namespace re-exports (OTELAPI and CONVENTIONS)
pull in entire `@opentelemetry` packages and defeat tree-shaking; replace the
wildcard imports and re-exports with named exports for only the symbols actually
used (e.g., export { context, SpanKind } from '@opentelemetry/api' and export {
ATTR_CODE_FUNCTION_NAME, ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from
'@opentelemetry/semantic-conventions'), or remove the OTELAPI/CONVENTIONS
exports and let consumers import directly; update any internal references to
OTELAPI or CONVENTIONS to use the named symbols or the original package imports
to restore tree-shaking and reduce bundle size.
- Around line 13-18: Remove the redundant default export object that mirrors the
named exports; delete the default export block that returns OTELAPI,
CONVENTIONS, getTelemetryClient, and getTraceClient and rely solely on the
existing named exports. Ensure the named exports OTELAPI, CONVENTIONS,
getTelemetryClient, and getTraceClient are exported where they are defined (or
add explicit export statements) so consumers import them by name and
tree-shaking works correctly.
In `@packages/diagnostics/tsconfig.json`:
- Line 4: The tsconfig.json currently sets "outDir": "dist/esm" which mismatches
the Vite outputs and is misleading; update the tsconfig.json's outDir key to
"dist/es" to match Vite's ES output (or remove the outDir entry entirely if you
prefer to avoid any tsc emit assumptions since vite-plugin-dts drives
declaration output) so that the "outDir" setting reflects the actual build paths
used by the project.
In `@packages/diagnostics/vite.config.mts`:
- Around line 21-23: The spread uses a redundant nullish fallback because the
variable dependencies is initialized to {} earlier; update the Object.keys
spread to use dependencies directly (remove the "?? {}" guard) so the line
becomes Object.keys({ ...(dependencies) }) or simply Object.keys(dependencies)
as appropriate, ensuring you reference the existing dependencies variable used
on line 6 and the Object.keys(...) expression to locate and modify the code.
Comment on lines
40
to
52
| const returnedValue = fn(source, vars, graphqlContext, info) | ||
|
|
||
| if (!span) return returnedValue | ||
|
|
||
| if (!(returnedValue instanceof Promise)) { | ||
| span.end() | ||
| return returnedValue | ||
| } | ||
|
|
||
| return returnedValue.then((value) => { | ||
| span.end() | ||
| return value | ||
| }) as TReturn |
There was a problem hiding this comment.
Span is never ended on errors — both sync throws and promise rejections leak spans.
If fn() throws synchronously (line 40), execution exits the context.with callback without calling span.end(). Similarly, if the returned promise rejects, there's no .catch() to end the span. Leaked spans corrupt trace data and can cause memory pressure in the exporter.
Proposed fix
- const returnedValue = fn(source, vars, graphqlContext, info)
+ let returnedValue: TReturn
+ try {
+ returnedValue = fn(source, vars, graphqlContext, info)
+ } catch (err) {
+ span?.setStatus({ code: OTELAPI.SpanStatusCode.ERROR })
+ span?.end()
+ throw err
+ }
if (!span) return returnedValue
if (!(returnedValue instanceof Promise)) {
span.end()
return returnedValue
}
- return returnedValue.then((value) => {
- span.end()
- return value
- }) as TReturn
+ return returnedValue
+ .then((value) => {
+ span.end()
+ return value
+ })
+ .catch((err) => {
+ span.setStatus({ code: OTELAPI.SpanStatusCode.ERROR })
+ span.end()
+ throw err
+ }) as TReturnAs per coding guidelines, "Proper error handling and validation".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const returnedValue = fn(source, vars, graphqlContext, info) | |
| if (!span) return returnedValue | |
| if (!(returnedValue instanceof Promise)) { | |
| span.end() | |
| return returnedValue | |
| } | |
| return returnedValue.then((value) => { | |
| span.end() | |
| return value | |
| }) as TReturn | |
| let returnedValue: TReturn | |
| try { | |
| returnedValue = fn(source, vars, graphqlContext, info) | |
| } catch (err) { | |
| span?.setStatus({ code: OTELAPI.SpanStatusCode.ERROR }) | |
| span?.end() | |
| throw err | |
| } | |
| if (!span) return returnedValue | |
| if (!(returnedValue instanceof Promise)) { | |
| span.end() | |
| return returnedValue | |
| } | |
| return returnedValue | |
| .then((value) => { | |
| span.end() | |
| return value | |
| }) | |
| .catch((err) => { | |
| span.setStatus({ code: OTELAPI.SpanStatusCode.ERROR }) | |
| span.end() | |
| throw err | |
| }) as TReturn |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/observability/telemetry.ts` around lines 40 - 52, The span
is not ended when fn throws synchronously or when the returned promise rejects;
wrap the invocation of fn (the call that assigns returnedValue inside the
context.with callback) in a try/catch so that span.end() is called in the catch
before rethrowing synchronous errors, and for the async case replace the current
then-only logic with a finally (or attach both then and catch) on the Promise
(e.g., Promise.resolve(returnedValue).finally(...)) to ensure span.end() runs on
resolve or rejection; reference the variables/functions returnedValue, fn, span
and the context.with callback when making the changes.
Comment on lines
1
to
11
| export async function register() { | ||
| if (process.env.NEXT_RUNTIME === 'nodejs') { | ||
| const { name, version } = await import('../package.json') | ||
| const { api } = await import('../discovery.config') | ||
| ;(await import('@faststore/diagnostics')).getTelemetryClient({ | ||
| name, | ||
| version, | ||
| account: api.storeId, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unhandled import failures can crash Next.js on startup.
Any of the three dynamic imports — package.json, discovery.config, or @faststore/diagnostics — can throw (e.g., missing discovery.config, misconfigured environment). Next.js doesn't catch errors thrown from register(), so a failed telemetry init will prevent the entire app from starting. Wrap the body in a try/catch and degrade gracefully.
🛡️ Proposed fix
export async function register() {
if (process.env.NEXT_RUNTIME === 'nodejs') {
- const { name, version } = await import('../package.json')
- const { api } = await import('../discovery.config')
- ;(await import('@faststore/diagnostics')).getTelemetryClient({
- name,
- version,
- account: api.storeId,
- })
+ try {
+ const { name, version } = await import('../package.json')
+ const { api } = await import('../discovery.config')
+ ;(await import('@faststore/diagnostics')).getTelemetryClient({
+ name,
+ version,
+ account: api.storeId,
+ })
+ } catch (err) {
+ console.warn('[faststore] Telemetry initialization failed:', err)
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/instrumentation.ts` around lines 1 - 11, The register
function can throw from its dynamic imports (package.json, discovery.config, or
`@faststore/diagnostics`) and must not crash Next.js on startup; update the async
function register to check process.env.NEXT_RUNTIME as before but wrap the
entire import/initialization block in a try/catch, gracefully short-circuit on
error (do not rethrow), and emit a safe error message (e.g., via console.error
or a fallback logger) so telemetry failures do not prevent the app from
starting; reference the register function and the three dynamic imports
(import('../package.json'), import('../discovery.config'),
import('@faststore/diagnostics')) when making the change.
packages/diagnostics/src/start.ts
Outdated
Comment on lines
51
to
52
| await TELEMETRY_CLIENT.newLogsClient({ exporter: tracesExporter }) | ||
| await TELEMETRY_CLIENT.newMetricsClient({ exporter: tracesExporter }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -path "*/packages/diagnostics/src/start.ts" -type fRepository: vtex/faststore
Length of output: 93
🏁 Script executed:
# Check the repository structure
ls -la packages/ 2>/dev/null | head -20
fd "start.ts" packages/diagnostics/Repository: vtex/faststore
Length of output: 707
🏁 Script executed:
# Read the start.ts file to understand the context
cat -n packages/diagnostics/src/start.ts | head -80Repository: vtex/faststore
Length of output: 2450
🏁 Script executed:
# Search for tracesExporter definition and Exporters.CreateTracesExporterConfig
rg "tracesExporter|CreateTracesExporterConfig" packages/diagnostics/src/ -A 2 -B 2Repository: vtex/faststore
Length of output: 1364
🏁 Script executed:
# Search for newLogsClient and newMetricsClient definitions
rg "newLogsClient|newMetricsClient" packages/diagnostics/ -B 2 -A 5Repository: vtex/faststore
Length of output: 700
🏁 Script executed:
# Search for other exporter creation methods
rg "CreateExporterConfig|CreateLogsExporter|CreateMetricsExporter" packages/ -t ts -t jsRepository: vtex/faststore
Length of output: 40
🏁 Script executed:
# Look for any package.json to understand dependencies
cat packages/diagnostics/package.json | grep -A 5 -B 5 "@vtex/diagnostics"Repository: vtex/faststore
Length of output: 356
🏁 Script executed:
# Search for other usages of newLogsClient or newMetricsClient
rg "newLogsClient|newMetricsClient" . -t ts -t js --max-count=20Repository: vtex/faststore
Length of output: 270
🏁 Script executed:
# Look for any patterns showing how exporters should be configured
rg "exporter.*config|exporter.*=" packages/ -t ts -B 2 -A 2Repository: vtex/faststore
Length of output: 40
🌐 Web query:
@vtex/diagnostics-nodejs newLogsClient newMetricsClient exporter OTLP configuration
💡 Result:
1) Configure OTLP via the library config (recommended)
Create a telemetry.json (or whatever you pass in NewTelemetryClient(..., { config: { configPath } })) and set exporters per signal:
{
"logs": {
"exporters": {
"mode": "otlp",
"otlp": { "endpoint": "otel-collector:4317" }
}
},
"metrics": {
"exporters": {
"mode": "otlp",
"otlp": { "endpoint": "otel-collector:4317" }
}
}
}The config schema supports mode: "otlp" | "console" | "none" and otlp.endpoint (plus optional headers, timeoutMs, batching/queue knobs, etc.). [1]
Then you can just do:
const telemetry = await NewTelemetryClient(appId, clientName, serviceName, { config: { configPath: './telemetry.json' } })
const logs = await telemetry.newLogsClient()
const metrics = await telemetry.newMetricsClient()newLogsClient() / newMetricsClient() accept per-client override objects. [2]
2) Configure OTLP exporter programmatically and pass it to the clients
Build an OTLP exporter using the helpers and inject it via overrides (both logs + metrics configs accept exporter?: Exporter). [3][4][5]
import { NewTelemetryClient, Exporters } from '@vtex/diagnostics-nodejs'
const telemetry = await NewTelemetryClient(appId, clientName, serviceName)
const logsExporter = Exporters.CreateExporter(
Exporters.CreateLogsExporterConfig({
endpoint: 'otel-collector:4317',
compression: 'gzip',
timeoutSeconds: 30,
insecure: true
}),
'otlp'
)
const metricsExporter = Exporters.CreateExporter(
Exporters.CreateMetricsExporterConfig({
endpoint: 'otel-collector:4317',
compression: 'gzip',
timeoutSeconds: 30,
insecure: true
}),
'otlp'
)
const logs = await telemetry.newLogsClient({ exporter: logsExporter })
const metrics = await telemetry.newMetricsClient({ exporter: metricsExporter })The exporter options include endpoint plus optional interval, timeoutSeconds, compression ('none'|'gzip'), and insecure. [3][6]
Sources
[1] https://cdn.jsdelivr.net/npm/@vtex/diagnostics-nodejs@5.1.1/dist/types/config.d.ts
[2] https://cdn.jsdelivr.net/npm/@vtex/diagnostics-nodejs@5.1.1/dist/telemetry/client.d.ts
[3] https://cdn.jsdelivr.net/npm/@vtex/diagnostics-nodejs@5.1.1/dist/types/exporters.d.ts
[4] https://cdn.jsdelivr.net/npm/@vtex/diagnostics-nodejs@5.1.1/dist/types/logs.d.ts
[5] https://cdn.jsdelivr.net/npm/@vtex/diagnostics-nodejs@5.1.1/dist/types/metrics.d.ts
[6] https://cdn.jsdelivr.net/npm/@vtex/diagnostics-nodejs@5.1.1/dist/exporters/index.d.ts
🏁 Script executed:
# Look for node_modules or any type definitions
find . -name "diagnostics-nodejs" -o -name "*diagnostics*" | grep -E "(node_modules|\.d\.ts)" | head -20Repository: vtex/faststore
Length of output: 40
🌐 Web query:
@vtex/diagnostics-nodejs npm package documentation
💡 Result:
@vtex/diagnostics-nodejs documentation (npm)
What it is: An OpenTelemetry-based Node.js library that provides a unified setup for traces, metrics, and logs, plus auto-instrumentation (Express/Fastify/Koa/NestJS), rule-based sampling, and hot-reloaded remote config (local file or S3). It’s marked as VTEX proprietary / internal-use in the README. [1]
Install
npm install `@vtex/diagnostics-nodejs`[1]
Minimal setup (important ordering)
Initialize the telemetry client before importing your framework, then create clients and register instrumentations. [1]
import { NewTelemetryClient, Instrumentation } from '@vtex/diagnostics-nodejs'
async function main() {
const instrumentations = Instrumentation.CommonInstrumentations.express()
const telemetryClient = await NewTelemetryClient(
'my-app-id',
'my-client-instance',
'my-service-name'
)
const tracesClient = await telemetryClient.newTracesClient({ setGlobalProvider: true })
const metricsClient = await telemetryClient.newMetricsClient()
const logsClient = await telemetryClient.newLogsClient()
telemetryClient.registerInstrumentations(instrumentations)
const express = require('express')
const app = express()
app.get('/api/users', (req, res) => {
logsClient.info('Users endpoint called', { userId: req.query.id })
res.json({ users: [] })
})
app.listen(3000)
}
main().catch(console.error)[1]
Remote config (hot reload)
File-based:
await NewTelemetryClient('app', 'client', 'service', {
config: { configPath: './config/telemetry.json', enableHotReload: true, pollIntervalMs: 15000 }
})S3-based:
await NewTelemetryClient('app', 'client', 'service', {
config: {
s3Config: { bucket: 'my-telemetry-configs', key: 'environments/prod/my-service.json', region: 'us-east-1' },
enableHotReload: true,
pollIntervalMs: 30000
}
})[1]
Primary doc/source: npm package README. [1]
Create separate exporters for logs, metrics, and traces signals.
The code reuses tracesExporter (created via Exporters.CreateTracesExporterConfig) for newLogsClient and newMetricsClient, but the library requires signal-specific exporter configs. Use Exporters.CreateLogsExporterConfig() for logs and Exporters.CreateMetricsExporterConfig() for metrics to avoid malformed telemetry data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/src/start.ts` around lines 51 - 52, The code incorrectly
reuses tracesExporter (from Exporters.CreateTracesExporterConfig) for logs and
metrics; replace those calls to create and pass signal-specific exporters by
creating a logsExporter via Exporters.CreateLogsExporterConfig() and a
metricsExporter via Exporters.CreateMetricsExporterConfig(), then call
TELEMETRY_CLIENT.newLogsClient({ exporter: logsExporter }) and
TELEMETRY_CLIENT.newMetricsClient({ exporter: metricsExporter }) while leaving
the traces client using tracesExporter.
packages/diagnostics/src/start.ts
Outdated
| TELEMETRY_CLIENT.registerInstrumentations( | ||
| Instrumentation.CommonInstrumentations.minimal() | ||
| ) | ||
| console.log('TELEMETRY CLIENT STARTED', opt) |
There was a problem hiding this comment.
Remove console.log or downgrade to structured debug logging.
This logs the opt object which includes the account identifier in production. Use a proper logger at debug level, or remove entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/src/start.ts` at line 60, The console.log call that
prints "TELEMETRY CLIENT STARTED" with the opt object should be removed or
converted to structured debug logging: replace the console.log('TELEMETRY CLIENT
STARTED', opt) usage with a debug-level logger (e.g., processLogger.debug or the
project's logger) and ensure you do not emit sensitive fields from the opt
object—either omit or redact opt.account before logging (or only log a
non-identifying subset). Locate the console.log in start.ts where the opt
variable is used and change it to a debug-level, structured log that excludes or
masks the account identifier.
packages/diagnostics/vite.config.mts
Outdated
Comment on lines
15
to
17
| formats: ['cjs', 'es'], | ||
| fileName: '[format]/index', | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check Vite version in the workspace
cat packages/diagnostics/package.json | grep -A2 '"vite"' || echo "vite not in package.json"Repository: vtex/faststore
Length of output: 119
🏁 Script executed:
# Check pnpm-workspace catalog for vite version
cat pnpm-workspace.yaml 2>/dev/null | grep -A50 "catalog:" | grep -E "vite|^\s+[a-z]" | head -20Repository: vtex/faststore
Length of output: 396
🏁 Script executed:
# Read the full vite.config.mts file
cat -n packages/diagnostics/vite.config.mtsRepository: vtex/faststore
Length of output: 909
🏁 Script executed:
# Check package.json exports/require fields
cat packages/diagnostics/package.json | grep -A10 '"exports"' || cat packages/diagnostics/package.json | grep -A5 '"require"'Repository: vtex/faststore
Length of output: 344
🏁 Script executed:
# Check if dist folder exists and what files are generated
ls -la packages/diagnostics/dist/cjs/ 2>/dev/null || echo "dist/cjs not present"Repository: vtex/faststore
Length of output: 78
🏁 Script executed:
# Search for any fileName function or custom extension handling in the codebase
rg -n "fileName" packages/diagnostics/ -A2 -B2Repository: vtex/faststore
Length of output: 380
CJS output extension mismatch with package.json "require" field.
Vite 7 library mode with fileName: '[format]/index' emits .cjs for CJS format by default, but package.json declares "require": "./dist/cjs/index.js". CommonJS consumers will get a MODULE_NOT_FOUND error. Use a fileName function to force the .js extension for CJS and .mjs for ES:
fileName: (format) => `${format === 'cjs' ? 'cjs' : 'es'}/index.${format === 'cjs' ? 'js' : 'mjs'}`Alternatively, update package.json to expect "require": "./dist/cjs/index.cjs" if you prefer Vite's default naming.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/vite.config.mts` around lines 15 - 17, The Vite library
config currently uses fileName: '[format]/index' which makes Vite emit index.cjs
for the 'cjs' format while package.json's "require" expects ./dist/cjs/index.js;
update the Vite config's fileName to a function that returns a path forcing .js
for the 'cjs' format and .mjs for the ES format (i.e., return 'cjs/index.js' for
format === 'cjs' and 'es/index.mjs' otherwise) so the emitted files match
package.json, or alternatively update package.json's "require" to point to
./dist/cjs/index.cjs if you prefer Vite's default naming.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/platforms/vtex/index.ts (1)
43-65:⚠️ Potential issue | 🔴 Critical
idis generated once at factory creation — all requests share the same ID.
crypto.randomBytes(32)on line 49 runs once whenGraphqlVtexContextFactoryis awaited during server init. The returned closure reuses this singleidfor every incoming request. Ifidis meant to be a per-request correlation identifier (as the JSDoc "Request scope ID" on line 21 suggests), it needs to move inside the returned function.Proposed fix
export const GraphqlVtexContextFactory = async (options: Options) => { await getTelemetryClient({ name, version, account: options.account, }) - const id = crypto.randomBytes(32).toString('hex') return (ctx: any): GraphqlContext => { - ctx.id = id + ctx.id = crypto.randomBytes(32).toString('hex') ctx.storage = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 43 - 65, The factory currently generates a single `id` once in GraphqlVtexContextFactory and reuses it for all requests; move the `crypto.randomBytes(32).toString('hex')` call into the returned closure so a fresh request-scoped ID is assigned to `ctx.id` on each invocation (update the returned function that sets `ctx.id`, leaving other initializations like `ctx.storage`, `ctx.clients`, `ctx.loaders`, `ctx.account`, and `ctx.OTEL` unchanged).packages/api/local/server.mjs (1)
11-23:⚠️ Potential issue | 🟡 MinorMissing
OTELproperty in local serverapiOptions.
GraphqlVtexContextFactorynow assignsctx.OTEL = options.OTEL. WithoutOTELhere,ctx.OTELwill beundefined, which violates theRecord<string, unknown>contract onGraphqlContext. The core options (inpackages/core/src/server/options.ts) already includeOTEL: {}. This local server should match.Proposed fix
const apiOptions = { platform: 'vtex', account: 'storeframework', locale: 'en-US', environment: 'vtexcommercestable', channel: '{"salesChannel":"1","regionId":"","hasOnlyDefaultSalesChannel":"true"}', showSponsored: false, + OTEL: {}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/local/server.mjs` around lines 11 - 23, The apiOptions object used to build context for GraphqlVtexContextFactory is missing the OTEL property, causing ctx.OTEL to be undefined and violating the GraphqlContext contract; update the apiOptions constant to include an OTEL field (e.g., OTEL: {}) so GraphqlVtexContextFactory receives OTEL and ctx.OTEL is defined when creating the Yoga server with GraphqlVtexSchema() and GraphqlVtexContextFactory(apiOptions).
🧹 Nitpick comments (5)
packages/api/vite.config.mts (1)
12-12: Consider external source maps (true) instead of'inline'for non-production.
'inline'embeds the full base64-encoded source map directly into every output file, which can balloon dist artifacts significantly in dev/watch mode. Usingtrue(or'hidden') generates a separate.mapfile instead, keeping the bundled output lean while preserving debuggability.♻️ Proposed adjustment
- sourcemap: mode === 'production' ? 'hidden' : 'inline', + sourcemap: mode === 'production' ? 'hidden' : true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/vite.config.mts` at line 12, The sourcemap config currently uses 'inline' for non-production which embeds base64 maps into files; update the sourcemap expression in vite.config.mts (the sourcemap option) to generate external maps for dev by returning true instead of 'inline' (e.g., keep production as 'hidden' but change the non-production branch to true) so source maps are emitted as separate .map files and dist artifacts stay lean.packages/core/src/server/options.ts (1)
27-36:as Thides the OTEL augmentation and the generic is unconstrained — type safety concern.The function always adds an
OTELproperty to the return value, but the return typePromise<T>doesn't reflect that. Meanwhile,Thas noextends objectconstraint, so spreading...apiOptionsis only safe by accident. A constrained return type would be more honest:-export async function withTraceClient<T = typeof apiOptions>( - apiOptions: T -): Promise<T> { +export async function withTraceClient<T extends Record<string, unknown> = typeof apiOptions>( + options: T +): Promise<T & { OTEL: Record<string, unknown> }> { const OTEL = {} getTraceClient(name)?.inject(OTEL) return { - ...apiOptions, + ...options, OTEL, - } as T + } }This also renames the parameter to avoid shadowing the module-level
apiOptions. As per coding guidelines, "Ensure type safety and avoid type assertions when possible".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server/options.ts` around lines 27 - 36, Change withTraceClient to accurately reflect that it augments the input options with an OTEL field and to constrain the generic: make the type parameter T extend object (e.g., <T extends object>) and change the return type to Promise<T & { OTEL: Record<string, unknown> }> (or a more specific OTEL type), rename the parameter (e.g., inputOptions) to avoid shadowing the module-level apiOptions, remove the unsafe "as T" cast, and return { ...inputOptions, OTEL } typed as T & { OTEL: ... } so the compiler knows the OTEL augmentation is present; keep the getTraceClient(name)?.inject(OTEL) call unchanged.packages/api/test/integration/queries.test.ts (1)
114-291: Consider extractingcreateRunnerinto a shared test utility.The
createRunnerfunction is essentially identical inqueries.test.tsandmutations.test.ts(sameapiOptionsshape, same schema/context wiring). A shared helper would reduce duplication and make future changes to the runner setup a single-point edit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/test/integration/queries.test.ts` around lines 114 - 291, The tests duplicate the createRunner setup across queries.test.ts and mutations.test.ts; extract it into a shared test utility to DRY the code. Create a new helper (e.g., export function createTestRunner or re-export createRunner) that encapsulates the apiOptions shape and schema/context wiring used in both files, move the existing createRunner implementation into that helper, update queries.test.ts and mutations.test.ts to import and call the shared createTestRunner (replacing their local createRunner), and remove the duplicated local definitions so future changes are made in one place.packages/api/src/platforms/vtex/index.ts (1)
67-71: Remove commented-out dead code.The
VtexResolverfunction is fully commented out. If it's no longer needed, remove it to keep the codebase clean. If it's planned for future use, track it in an issue instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/platforms/vtex/index.ts` around lines 67 - 71, Remove the dead commented-out VtexResolver block: delete the commented function and its lines referencing VtexResolver, GraphqlResolver, ResolverTrace, and GraphqlContext; if this behavior is required later, open an issue referencing VtexResolver and ResolverTrace instead of leaving the commented code in packages/api/src/platforms/vtex/index.ts.packages/api/src/observability/telemetry.ts (1)
7-14: Consider tightening theinfoparameter type.
info: anyon lines 13 and 20 could beGraphQLResolveInfofrom thegraphqlpackage. This would improve type safety and catch misuse at compile time without any runtime cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/observability/telemetry.ts` around lines 7 - 14, The ResolverTrace function currently types its fourth parameter as info: any; tighten this by importing GraphQLResolveInfo from the graphql package and changing the parameter type in ResolverTrace (and any internal uses) to info: GraphQLResolveInfo so callers and implementations get proper compile-time type checking; update both the function signature (fn: (source, vars, context, info: GraphQLResolveInfo) => TReturn) and the ResolverTrace declaration that references info to use GraphQLResolveInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 80-96: getResolvers() has two issues: the parameter name in the
map ([name, resolverFunction]) shadows the module-level name import and
finalResolvers is typed as any which defeats the final "satisfies typeof
resolvers" check. Fix by renaming the destructured variable to a non-conflicting
identifier (e.g., fieldName) in the Object.entries(...).map call so it doesn't
shadow the imported name, and give finalResolvers a proper type instead of any
(for example derive its shape from nativeGetResolvers() or use Partial<typeof
resolvers> / Record<string, typeof resolver> as appropriate) so the satisfies
typeof resolvers check is meaningful; keep wrapping resolver functions with
ResolverTrace(`${key}(${fieldName})`) and preserve use of nativeGetResolvers and
getResolvers.
In `@packages/api/vite.config.mts`:
- Line 6: The build is externalizing everything from devDependencies which
causes runtime packages to be omitted from the bundle; update the externals
logic in vite.config.mts to only use dependencies and peerDependencies (remove
devDependencies from the externals array/merge) and move runtime packages such
as `@envelop/`*, `@faststore/diagnostics`, `@opentelemetry/api`, graphql,
graphql-yoga, and tslib into package.json dependencies or peerDependencies as
appropriate, keeping only true build-time tools (vite, vitest,
`@graphql-codegen/`*) in devDependencies; reference the existing variables
dependencies, peerDependencies, devDependencies and the externals handling in
vite.config.mts when making the change.
In `@packages/diagnostics/src/config.json`:
- Around line 6-16: The "critical_endpoints" rule currently sets sampleRate: 1.0
which will trace every request to endpoints matching attribute "http.target"
value "^/api/graphql"; reduce the default sampling to a lower value (e.g., 0.1)
or make it environment-dependent by reading an env var and applying it via the
diagnostics config hot-reload mechanism so production uses a lower rate while
allowing full sampling in debug/staging; update the "critical_endpoints" entry
to use the new sampling value or env-backed value and ensure the config reload
logic respects the changed key.
In `@packages/diagnostics/src/start.ts`:
- Around line 54-55: The calls to newLogsClient() and newMetricsClient() are
being invoked without any logs/metrics exporter configuration in config.json, so
they will fall back to defaults; either add explicit logs and metrics exporter
sections to config.json (e.g., specify exporter type and options) or update the
code around newLogsClient() and newMetricsClient() to pass the intended exporter
config/options, and if the clients are intentionally left as no-ops add a
concise comment above the calls referencing newLogsClient() and
newMetricsClient() that this is deliberate and will be configured later so
reviewers understand the intent.
- Line 49: The use of require.resolve for configPath will break in ESM builds;
inside getTelemetryClient replace
require.resolve('@faststore/diagnostics/config') with an ESM-safe resolution
(either create a require via createRequire(import.meta.url).resolve(...) or use
await import.meta.resolve(...)) so the code runs in both CJS and ESM. Update the
top of the module to import createRequire from 'module' if you choose that
approach, or make the resolution async if using import.meta.resolve; ensure the
symbol configPath in getTelemetryClient uses the chosen resolver instead of
require.resolve.
In `@packages/diagnostics/vite.config.mts`:
- Line 10: Remove the unnecessary type assertion by updating the Vite config's
plugins array to use dts() directly; locate the plugins: [dts() as any] entry
and change it to plugins: [dts()] so it matches sibling packages (api,
components, sdk, ui) and removes the redundant cast.
---
Outside diff comments:
In `@packages/api/local/server.mjs`:
- Around line 11-23: The apiOptions object used to build context for
GraphqlVtexContextFactory is missing the OTEL property, causing ctx.OTEL to be
undefined and violating the GraphqlContext contract; update the apiOptions
constant to include an OTEL field (e.g., OTEL: {}) so GraphqlVtexContextFactory
receives OTEL and ctx.OTEL is defined when creating the Yoga server with
GraphqlVtexSchema() and GraphqlVtexContextFactory(apiOptions).
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 43-65: The factory currently generates a single `id` once in
GraphqlVtexContextFactory and reuses it for all requests; move the
`crypto.randomBytes(32).toString('hex')` call into the returned closure so a
fresh request-scoped ID is assigned to `ctx.id` on each invocation (update the
returned function that sets `ctx.id`, leaving other initializations like
`ctx.storage`, `ctx.clients`, `ctx.loaders`, `ctx.account`, and `ctx.OTEL`
unchanged).
---
Duplicate comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 50-53: The returnedValue Promise handling in the wrapper doesn't
end the span on rejection, so update the Promise chain for returnedValue (the
variable returned from the resolver) to attach a .catch(...) that calls
span.end() before rethrowing the error; keep the existing .then(...) that ends
the span on success and returns the value, and ensure both success and error
paths always call span.end() (refer to returnedValue, span.end, and the
surrounding resolver wrapper function).
In `@packages/diagnostics/vite.config.mts`:
- Around line 15-17: The bundle is emitting mismatched extensions due to the
static fileName '[format]/index'; update the Vite config (the
entry/formats/fileName block in packages/diagnostics/vite.config.mts) to supply
a fileName function that returns explicit extensions per format (e.g., return
'cjs/index.cjs' for 'cjs' and 'es/index.js' for 'es') so output extensions are
pinned regardless of package.json "type"; after changing fileName, verify
packages/diagnostics/package.json "type" and the "require" export path point to
the correct extension (e.g., ./dist/cjs/index.cjs) and adjust the "require"
field if you prefer Vite's default naming instead.
---
Nitpick comments:
In `@packages/api/src/observability/telemetry.ts`:
- Around line 7-14: The ResolverTrace function currently types its fourth
parameter as info: any; tighten this by importing GraphQLResolveInfo from the
graphql package and changing the parameter type in ResolverTrace (and any
internal uses) to info: GraphQLResolveInfo so callers and implementations get
proper compile-time type checking; update both the function signature (fn:
(source, vars, context, info: GraphQLResolveInfo) => TReturn) and the
ResolverTrace declaration that references info to use GraphQLResolveInfo.
In `@packages/api/src/platforms/vtex/index.ts`:
- Around line 67-71: Remove the dead commented-out VtexResolver block: delete
the commented function and its lines referencing VtexResolver, GraphqlResolver,
ResolverTrace, and GraphqlContext; if this behavior is required later, open an
issue referencing VtexResolver and ResolverTrace instead of leaving the
commented code in packages/api/src/platforms/vtex/index.ts.
In `@packages/api/test/integration/queries.test.ts`:
- Around line 114-291: The tests duplicate the createRunner setup across
queries.test.ts and mutations.test.ts; extract it into a shared test utility to
DRY the code. Create a new helper (e.g., export function createTestRunner or
re-export createRunner) that encapsulates the apiOptions shape and
schema/context wiring used in both files, move the existing createRunner
implementation into that helper, update queries.test.ts and mutations.test.ts to
import and call the shared createTestRunner (replacing their local
createRunner), and remove the duplicated local definitions so future changes are
made in one place.
In `@packages/api/vite.config.mts`:
- Line 12: The sourcemap config currently uses 'inline' for non-production which
embeds base64 maps into files; update the sourcemap expression in
vite.config.mts (the sourcemap option) to generate external maps for dev by
returning true instead of 'inline' (e.g., keep production as 'hidden' but change
the non-production branch to true) so source maps are emitted as separate .map
files and dist artifacts stay lean.
In `@packages/core/src/server/options.ts`:
- Around line 27-36: Change withTraceClient to accurately reflect that it
augments the input options with an OTEL field and to constrain the generic: make
the type parameter T extend object (e.g., <T extends object>) and change the
return type to Promise<T & { OTEL: Record<string, unknown> }> (or a more
specific OTEL type), rename the parameter (e.g., inputOptions) to avoid
shadowing the module-level apiOptions, remove the unsafe "as T" cast, and return
{ ...inputOptions, OTEL } typed as T & { OTEL: ... } so the compiler knows the
OTEL augmentation is present; keep the getTraceClient(name)?.inject(OTEL) call
unchanged.
Comment on lines
+80
to
+96
| export function getResolvers() { | ||
| const resolvers = nativeGetResolvers() | ||
|
|
||
| const finalResolvers: any = {} | ||
| for (const [key, resolver] of Object.entries(resolvers)) { | ||
| finalResolvers[key] = Object.fromEntries( | ||
| Object.entries(resolver).map(([name, resolverFunction]) => { | ||
| if (typeof resolverFunction !== 'function') | ||
| return [name, resolverFunction] | ||
|
|
||
| return [name, ResolverTrace(resolverFunction, `${key}(${name})`)] | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
| return finalResolvers satisfies typeof resolvers | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
name shadows the module-level name import; any nullifies the satisfies check.
Two concerns in getResolvers():
-
Line 86: The destructured
namein.map(([name, resolverFunction])shadows thenameimported frompackage.json(line 5). Not a runtime bug here, but easy to trip over in future edits. Rename to e.g.fieldName. -
Lines 83/95:
finalResolversis typedany, sosatisfies typeof resolversis a no-op — it won't catch shape mismatches at compile time.
Proposed fix
- const finalResolvers: any = {}
+ const finalResolvers: Record<string, Record<string, unknown>> = {}
for (const [key, resolver] of Object.entries(resolvers)) {
finalResolvers[key] = Object.fromEntries(
- Object.entries(resolver).map(([name, resolverFunction]) => {
+ Object.entries(resolver).map(([fieldName, resolverFunction]) => {
if (typeof resolverFunction !== 'function')
- return [name, resolverFunction]
+ return [fieldName, resolverFunction]
- return [name, ResolverTrace(resolverFunction, `${key}(${name})`)]
+ return [fieldName, ResolverTrace(resolverFunction, `${key}(${fieldName})`)]
})
)
}As per coding guidelines, "Ensure type safety and avoid type assertions when possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/platforms/vtex/index.ts` around lines 80 - 96,
getResolvers() has two issues: the parameter name in the map ([name,
resolverFunction]) shadows the module-level name import and finalResolvers is
typed as any which defeats the final "satisfies typeof resolvers" check. Fix by
renaming the destructured variable to a non-conflicting identifier (e.g.,
fieldName) in the Object.entries(...).map call so it doesn't shadow the imported
name, and give finalResolvers a proper type instead of any (for example derive
its shape from nativeGetResolvers() or use Partial<typeof resolvers> /
Record<string, typeof resolver> as appropriate) so the satisfies typeof
resolvers check is meaningful; keep wrapping resolver functions with
ResolverTrace(`${key}(${fieldName})`) and preserve use of nativeGetResolvers and
getResolvers.
Comment on lines
+6
to
+16
| { | ||
| "name": "critical_endpoints", | ||
| "sampleRate": 1.0, | ||
| "conditions": [ | ||
| { | ||
| "attribute": "http.target", | ||
| "operator": "Regex", | ||
| "value": "^/api/graphql" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
100% sampling on /api/graphql may be costly at scale.
sampleRate: 1.0 means every single GraphQL request produces a trace. This is great for debugging, but in production under load the exporter overhead and backend ingestion costs can be significant. Consider starting with a lower rate (e.g., 0.1) or making it environment-dependent via the diagnostics config hot-reload capability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/src/config.json` around lines 6 - 16, The
"critical_endpoints" rule currently sets sampleRate: 1.0 which will trace every
request to endpoints matching attribute "http.target" value "^/api/graphql";
reduce the default sampling to a lower value (e.g., 0.1) or make it
environment-dependent by reading an env var and applying it via the diagnostics
config hot-reload mechanism so production uses a lower rate while allowing full
sampling in debug/staging; update the "critical_endpoints" entry to use the new
sampling value or env-backed value and ensure the config reload logic respects
the changed key.
| }, | ||
| debug: globalThis.fsDiagnostics.IS_DEV, | ||
| config: { | ||
| configPath: require.resolve('@faststore/diagnostics/config'), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the diagnostics package structure and configuration
echo "=== Package.json content ==="
cat packages/diagnostics/package.json
echo ""
echo "=== start.ts file content (full) ==="
cat packages/diagnostics/src/start.tsRepository: vtex/faststore
Length of output: 3082
🏁 Script executed:
# Check for vite config in diagnostics package
echo "=== Vite config (if exists) ==="
fd "vite\.config" packages/diagnostics --exec cat {}
echo ""
echo "=== tsconfig.json in diagnostics ==="
fd "tsconfig" packages/diagnostics --exec cat {}
echo ""
echo "=== Check if require is imported/shimmed anywhere ==="
rg "createRequire|require\.resolve" packages/diagnostics/src/ -A 2 -B 2Repository: vtex/faststore
Length of output: 1543
🏁 Script executed:
# Check the main entry point
echo "=== index.ts (main entry) ==="
cat packages/diagnostics/src/index.ts
echo ""
echo "=== Check how start.ts is used ==="
rg "start\.ts|from.*start" packages/diagnostics/src/ -B 2 -A 2Repository: vtex/faststore
Length of output: 1068
require.resolve will fail in ESM builds—use createRequire or import.meta.resolve.
This file mixes ESM syntax (import ... with { type: 'json' } at line 11) with the CJS API require.resolve. The Vite config builds both CJS and ESM outputs (formats: ['es', 'cjs']), and getTelemetryClient (which contains the require.resolve call) is exported from the main entry point. When the ESM build is consumed, require will be undefined at runtime and throw an error.
Use createRequire(import.meta.url).resolve(...) or import.meta.resolve (Node 20+) to ensure the code works in both module formats.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/src/start.ts` at line 49, The use of require.resolve for
configPath will break in ESM builds; inside getTelemetryClient replace
require.resolve('@faststore/diagnostics/config') with an ESM-safe resolution
(either create a require via createRequire(import.meta.url).resolve(...) or use
await import.meta.resolve(...)) so the code runs in both CJS and ESM. Update the
top of the module to import createRequire from 'module' if you choose that
approach, or make the resolution async if using import.meta.resolve; ensure the
symbol configPath in getTelemetryClient uses the chosen resolver instead of
require.resolve.
Comment on lines
+54
to
+55
| await client.newLogsClient() | ||
| await client.newMetricsClient() |
There was a problem hiding this comment.
newLogsClient() and newMetricsClient() called without exporter config and no logs/metrics section in config.json.
The config at packages/diagnostics/src/config.json only defines traces.sampling — there's no logs or metrics exporter configuration. These clients will fall back to library defaults (likely no-op or console). If you intend to export logs and metrics, add the appropriate sections to config.json. If they're intentionally no-op placeholders for now, a brief comment would clarify intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/diagnostics/src/start.ts` around lines 54 - 55, The calls to
newLogsClient() and newMetricsClient() are being invoked without any
logs/metrics exporter configuration in config.json, so they will fall back to
defaults; either add explicit logs and metrics exporter sections to config.json
(e.g., specify exporter type and options) or update the code around
newLogsClient() and newMetricsClient() to pass the intended exporter
config/options, and if the clients are intentionally left as no-ops add a
concise comment above the calls referencing newLogsClient() and
newMetricsClient() that this is deliberate and will be configured later so
reviewers understand the intent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores