Add custom telemetry exporter support #1602
Conversation
🦋 Changeset detectedLatest commit: d3bd091 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/cli-v3/src/config.tsOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces support for OpenTelemetry (otel) exporters across the project. The changes enable configuration and integration of telemetry data collection by adding an Changes
Sequence DiagramsequenceDiagram
participant Config as Trigger Configuration
participant TracingSDK as Tracing SDK
participant Exporter as Span Exporter
Config->>TracingSDK: Provide exporters
TracingSDK->>Exporter: Create SpanProcessor
TracingSDK->>Exporter: Add to TraceProvider
Exporter-->>TracingSDK: Process and export spans
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
references/nextjs-realtime/trigger.config.ts (1)
48-51: Consider environment-based debug configuration.The AISDKExporter is configured with
debug: true, which might generate excessive logs in production.Consider using an environment variable to control debug mode:
const exporter = new AISDKExporter({ - debug: true, + debug: process.env.NODE_ENV !== 'production', client, });.changeset/rich-trainers-glow.md (1)
1-6: Enhance the changeset description with more details.Since this is a significant feature addition (OpenTelemetry exporter support), consider expanding the description to include:
- Purpose and benefits of adding otel exporter support
- Key components affected
- Any breaking changes or migration steps
packages/core/src/v3/otel/tracingSDK.ts (1)
157-159: Consider performance and error handling improvements.While the implementation is functional, consider these enhancements:
- Add validation for the exporters array
- Consider using BatchSpanProcessor for better performance
- Add error handling for individual exporter initialization
Example implementation:
- for (const exporter of config.exporters ?? []) { - traceProvider.addSpanProcessor(new SimpleSpanProcessor(exporter)); - } + const exporters = config.exporters ?? []; + if (exporters.length > 0) { + try { + for (const exporter of exporters) { + if (!exporter) { + console.warn("Skipping null or undefined exporter"); + continue; + } + + const processor = getEnvVar("OTEL_BATCH_PROCESSING_ENABLED") === "1" + ? new BatchSpanProcessor(exporter) + : new SimpleSpanProcessor(exporter); + + traceProvider.addSpanProcessor(processor); + } + } catch (error) { + console.error("Error initializing additional exporters:", error); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.changeset/rich-trainers-glow.md(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(1 hunks)packages/core/src/v3/config.ts(1 hunks)packages/core/src/v3/otel/tracingSDK.ts(2 hunks)references/nextjs-realtime/package.json(1 hunks)references/nextjs-realtime/src/trigger/ai.ts(2 hunks)references/nextjs-realtime/trigger.config.ts(1 hunks)
🔇 Additional comments (4)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
132-132: LGTM!The integration of exporters into the TracingSDK configuration is clean and follows best practices by providing a default empty array.
references/nextjs-realtime/package.json (1)
30-30: Verify langsmith package version and security.Please verify the following:
- Is ^0.2.15 the latest stable version of langsmith?
- Are there any known vulnerabilities in this version?
✅ Verification successful
✓ langsmith@^0.2.15 version is appropriate
The package is using the latest available version (0.2.15) and no security advisories were found in the npm ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest version and security advisories for langsmith # Get latest version from npm echo "Latest version from npm:" npm view langsmith version # Check for known vulnerabilities echo "Checking for vulnerabilities:" npm audit langsmith@0.2.15Length of output: 541
Script:
#!/bin/bash # Check for security advisories using GitHub CLI echo "Checking for security advisories:" gh api \ -H "Accept: application/vnd.github+json" \ -H "X-GitHub-Api-Version: 2022-11-28" \ /repos/langchain-ai/langsmith-sdk/security/advisories # Also check npm advisory database echo "Checking npm advisory database:" curl -s https://registry.npmjs.org/-/npm/v1/security/advisories/search?package=langsmithLength of output: 590
packages/core/src/v3/otel/tracingSDK.ts (2)
88-88: LGTM! Type definition looks good.The optional exporters array is correctly typed and placed in the TracingSDKConfig interface.
157-159: Verify impact on existing tracing functionality.The addition of multiple exporters could impact performance and resource usage. Please ensure:
- Existing tracing functionality remains unaffected
- Memory usage is acceptable with multiple exporters
- No duplicate spans are being exported
✅ Verification successful
Multiple exporters implementation is safe and well-designed
The implementation follows OpenTelemetry best practices:
- Each exporter gets its own SimpleSpanProcessor, preventing conflicts
- Proper shutdown handling is implemented
- Memory usage is optimized as spans are processed immediately
- No risk of duplicate spans as each processor handles its own export path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing span processor usage and potential conflicts # Look for span processor configurations echo "Checking span processor usage:" rg -l "SpanProcessor|TracerProvider" --type ts # Check for potential memory leak patterns echo "Checking for proper cleanup:" ast-grep --pattern 'shutdown() { $$$ this._traceProvider.shutdown() $$$ }'Length of output: 511
Script:
#!/bin/bash # Check for exporter configurations and processor types echo "Checking exporter configurations:" rg "new \w*SpanProcessor|addSpanProcessor" -A 2 --type ts echo -e "\nChecking shutdown patterns:" rg "shutdown.*Provider|Provider.*shutdown" --type ts echo -e "\nChecking for batch processors:" rg "BatchSpanProcessor" --type tsLength of output: 2783
| class LangsmithSpanExporterWrapper { | ||
| constructor( | ||
| private underlyingExporter: any, | ||
| private transformSpan: (span: any) => any | undefined = (span) => { | ||
| if (span.attributes["$span.partial"]) { | ||
| // Skip partial spans | ||
| return; | ||
| } | ||
|
|
||
| // Check if this is an attempt span | ||
| if (span.name.startsWith("Attempt ")) { | ||
| // Create a new span that wraps the original but modifies spanContext | ||
| const spanContext = span.spanContext(); | ||
|
|
||
| return { | ||
| ...span, | ||
| spanContext: () => spanContext, | ||
| parentSpanId: undefined, | ||
| }; | ||
| } | ||
| return span; | ||
| } | ||
| ) {} | ||
|
|
||
| export(spans: any[], resultCallback: (result: any) => void): void { | ||
| const modifiedSpans = spans.map(this.transformSpan); | ||
| this.underlyingExporter.export(modifiedSpans.filter(Boolean), resultCallback); | ||
| } | ||
|
|
||
| shutdown(): Promise<void> { | ||
| return this.underlyingExporter.shutdown(); | ||
| } | ||
|
|
||
| forceFlush?(): Promise<void> { | ||
| return this.underlyingExporter.forceFlush | ||
| ? this.underlyingExporter.forceFlush() | ||
| : Promise.resolve(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and error handling to the span exporter wrapper.
The implementation has several areas that could be improved:
- Use proper types instead of
any - Add error handling in the export method
- Consider memory efficiency for large span arrays
Consider this implementation:
+import type { SpanExporter, ReadableSpan } from '@opentelemetry/sdk-trace-base';
+
-class LangsmithSpanExporterWrapper {
+class LangsmithSpanExporterWrapper implements SpanExporter {
constructor(
- private underlyingExporter: any,
- private transformSpan: (span: any) => any | undefined = (span) => {
+ private underlyingExporter: SpanExporter,
+ private transformSpan: (span: ReadableSpan) => ReadableSpan | undefined = (span) => {
if (span.attributes["$span.partial"]) {
return;
}
if (span.name.startsWith("Attempt ")) {
const spanContext = span.spanContext();
return {
...span,
spanContext: () => spanContext,
parentSpanId: undefined,
};
}
return span;
}
) {}
- export(spans: any[], resultCallback: (result: any) => void): void {
+ export(spans: ReadableSpan[], resultCallback: (result: any) => void): void {
try {
- const modifiedSpans = spans.map(this.transformSpan);
+ const modifiedSpans = spans.map(this.transformSpan).filter(Boolean);
- this.underlyingExporter.export(modifiedSpans.filter(Boolean), resultCallback);
+ this.underlyingExporter.export(modifiedSpans, resultCallback);
+ } catch (error) {
+ resultCallback({ code: 1, error: error instanceof Error ? error : new Error(String(error)) });
}
}Committable suggestion skipped: line range outside the PR's diff.
| const telemetrySettings = AISDKExporter.getSettings(); | ||
|
|
||
| logger.info("Telemetry settings", { telemetrySettings }); | ||
|
|
||
| const result = streamText({ | ||
| model: openai(model), | ||
| prompt, | ||
| tools: { | ||
| getWeather: weatherTask.tool, | ||
| }, | ||
| maxSteps: 10, | ||
| experimental_telemetry: telemetrySettings, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle experimental telemetry feature gracefully.
The code uses experimental telemetry features without proper error handling or feature detection.
Consider adding error handling:
- const telemetrySettings = AISDKExporter.getSettings();
+ let telemetrySettings;
+ try {
+ telemetrySettings = AISDKExporter.getSettings();
+ logger.info("Telemetry settings", { telemetrySettings });
+ } catch (error) {
+ logger.warn("Failed to get telemetry settings", { error });
+ }
- logger.info("Telemetry settings", { telemetrySettings });
const result = streamText({
model: openai(model),
prompt,
- experimental_telemetry: telemetrySettings,
+ experimental_telemetry: telemetrySettings ?? undefined,
});📝 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.
| const telemetrySettings = AISDKExporter.getSettings(); | |
| logger.info("Telemetry settings", { telemetrySettings }); | |
| const result = streamText({ | |
| model: openai(model), | |
| prompt, | |
| tools: { | |
| getWeather: weatherTask.tool, | |
| }, | |
| maxSteps: 10, | |
| experimental_telemetry: telemetrySettings, | |
| let telemetrySettings; | |
| try { | |
| telemetrySettings = AISDKExporter.getSettings(); | |
| logger.info("Telemetry settings", { telemetrySettings }); | |
| } catch (error) { | |
| logger.warn("Failed to get telemetry settings", { error }); | |
| } | |
| const result = streamText({ | |
| model: openai(model), | |
| prompt, | |
| experimental_telemetry: telemetrySettings ?? undefined, | |
| }); |
packages/core/src/v3/config.ts
Outdated
| exporters?: Array<any>; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for exporters configuration.
Using Array<any> reduces type safety and could lead to runtime errors.
Consider using proper OpenTelemetry types:
+import type { SpanExporter } from '@opentelemetry/sdk-trace-base';
- exporters?: Array<any>;
+ exporters?: Array<SpanExporter>;📝 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.
| exporters?: Array<any>; | |
| import type { SpanExporter } from '@opentelemetry/sdk-trace-base'; | |
| exporters?: Array<SpanExporter>; | |
@trigger.dev/build
@trigger.dev/core
@trigger.dev/rsc
trigger.dev
@trigger.dev/sdk
@trigger.dev/react-hooks
commit: |
ericallam
force-pushed
the
ea-branch-4
branch
from
f967c63 to
e0c6a33
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/core/src/v3/semanticInternalAttributes.ts (1)
54-54: LGTM! Consider grouping related attributes.The new
SPAN_ATTEMPTattribute follows the established naming conventions and integrates well with the existing telemetry system.Consider grouping this attribute with other span-related attributes (
SPAN_PARTIAL,SPAN_ID) to improve code organization and maintainability. Here's a suggested reorganization:MACHINE_PRESET_MEMORY: "ctx.machine.memory", MACHINE_PRESET_CENTS_PER_MS: "ctx.machine.centsPerMs", + SPAN_ATTEMPT: "$span.attempt", SPAN_PARTIAL: "$span.partial", SPAN_ID: "$span.span_id", - SPAN_ATTEMPT: "$span.attempt",packages/cli-v3/src/config.ts (1)
320-323: Improve readability and add deprecation warningWhile the logic is correct, consider these improvements:
- The complex nullish coalescing chain could be simplified for better readability
- Add a deprecation warning for
config.instrumentationsto guide users toward the newtelemetry.instrumentations- Consider using proper type definitions instead of casting to
anyHere's a suggested refactoring:
- if (config.instrumentations ?? config.telemetry?.instrumentations) { - for (const instrumentation of config.telemetry?.instrumentations ?? - config.instrumentations ?? - []) { + const useDeprecatedPath = Boolean(config.instrumentations); + if (useDeprecatedPath) { + logger.warn( + "The 'instrumentations' option is deprecated. Please use 'telemetry.instrumentations' instead." + ); + } + + const instrumentations = config.telemetry?.instrumentations ?? config.instrumentations; + if (instrumentations) { + for (const instrumentation of instrumentations) {Also, consider defining a proper type for the instrumentation object instead of using
any:interface TelemetryInstrumentation { getModuleDefinitions?: () => Array<InstrumentationModuleDefinition>; }packages/core/src/v3/otel/tracingSDK.ts (1)
116-117: Consider environment-specific service namesThe service name fallback to "trigger.dev" might be too generic. Consider using a more specific default based on the application context.
- getEnvVar("OTEL_SERVICE_NAME") ?? "trigger.dev", + getEnvVar("OTEL_SERVICE_NAME") ?? `trigger.dev.${process.env.NODE_ENV ?? "development"}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/rich-trainers-glow.md(1 hunks)packages/cli-v3/src/config.ts(1 hunks)packages/cli-v3/src/entryPoints/deploy-run-worker.ts(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(1 hunks)packages/core/src/v3/config.ts(2 hunks)packages/core/src/v3/otel/tracingSDK.ts(5 hunks)packages/core/src/v3/semanticInternalAttributes.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(1 hunks)references/nextjs-realtime/package.json(2 hunks)references/nextjs-realtime/src/trigger/ai.ts(2 hunks)references/nextjs-realtime/trigger.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/v3/workers/taskExecutor.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- .changeset/rich-trainers-glow.md
- references/nextjs-realtime/src/trigger/ai.ts
- references/nextjs-realtime/trigger.config.ts
- packages/cli-v3/src/entryPoints/dev-run-worker.ts
- packages/core/src/v3/config.ts
- packages/cli-v3/src/entryPoints/deploy-run-worker.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
references/nextjs-realtime/package.json (1)
17-17: Verify OpenTelemetry and LangSmith package versionsThe specified versions might be outdated or incorrect:
@opentelemetry/exporter-trace-otlp-http@^0.57.0seems to be ahead of the latest stable versionlangsmith@^0.2.15seems to be ahead of the latest stable versionRun this script to check the latest stable versions:
Also applies to: 31-31
packages/core/src/v3/otel/tracingSDK.ts (1)
89-89: LGTM! Configuration change looks goodThe addition of optional exporters to TracingSDKConfig is well-typed and maintains backward compatibility.
| class ExternalSpanExporterWrapper { | ||
| constructor( | ||
| private underlyingExporter: SpanExporter, | ||
| private externalTraceId: string | ||
| ) {} | ||
|
|
||
| private transformSpan(span: ReadableSpan): ReadableSpan | undefined { | ||
| if (span.attributes[SemanticInternalAttributes.SPAN_PARTIAL]) { | ||
| // Skip partial spans | ||
| return; | ||
| } | ||
|
|
||
| const spanContext = span.spanContext(); | ||
|
|
||
| return { | ||
| ...span, | ||
| spanContext: () => ({ ...spanContext, traceId: this.externalTraceId }), | ||
| parentSpanId: span.attributes[SemanticInternalAttributes.SPAN_ATTEMPT] | ||
| ? undefined | ||
| : span.parentSpanId, | ||
| }; | ||
| } | ||
|
|
||
| export(spans: any[], resultCallback: (result: any) => void): void { | ||
| try { | ||
| const modifiedSpans = spans.map(this.transformSpan.bind(this)); | ||
| this.underlyingExporter.export( | ||
| modifiedSpans.filter(Boolean) as ReadableSpan[], | ||
| resultCallback | ||
| ); | ||
| } catch (e) { | ||
| console.error(e); | ||
| } | ||
| } | ||
|
|
||
| shutdown(): Promise<void> { | ||
| return this.underlyingExporter.shutdown(); | ||
| } | ||
|
|
||
| forceFlush?(): Promise<void> { | ||
| return this.underlyingExporter.forceFlush | ||
| ? this.underlyingExporter.forceFlush() | ||
| : Promise.resolve(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and error handling in ExternalSpanExporterWrapper
The wrapper implementation has several areas for improvement:
- Uses
any[]type instead of proper span types - Basic error handling that only logs errors
- Missing documentation explaining the wrapper's purpose
Consider these improvements:
+/**
+ * Wraps a SpanExporter to transform spans before export:
+ * - Replaces trace IDs with a consistent external ID
+ * - Filters out partial spans
+ * - Handles parent span relationships
+ */
class ExternalSpanExporterWrapper {
constructor(
private underlyingExporter: SpanExporter,
private externalTraceId: string
) {}
private transformSpan(span: ReadableSpan): ReadableSpan | undefined {
if (span.attributes[SemanticInternalAttributes.SPAN_PARTIAL]) {
// Skip partial spans
return;
}
const spanContext = span.spanContext();
return {
...span,
spanContext: () => ({ ...spanContext, traceId: this.externalTraceId }),
parentSpanId: span.attributes[SemanticInternalAttributes.SPAN_ATTEMPT]
? undefined
: span.parentSpanId,
};
}
- export(spans: any[], resultCallback: (result: any) => void): void {
+ export(
+ spans: ReadableSpan[],
+ resultCallback: (result: ExportResult) => void
+ ): void {
try {
const modifiedSpans = spans.map(this.transformSpan.bind(this));
this.underlyingExporter.export(
modifiedSpans.filter(Boolean) as ReadableSpan[],
resultCallback
);
} catch (e) {
- console.error(e);
+ console.error("Error in ExternalSpanExporterWrapper.export:", e);
+ resultCallback({ code: ExportResultCode.FAILED, error: e as Error });
}
}
shutdown(): Promise<void> {
- return this.underlyingExporter.shutdown();
+ return this.underlyingExporter.shutdown().catch((e) => {
+ console.error("Error in ExternalSpanExporterWrapper.shutdown:", e);
+ throw e;
+ });
}
forceFlush?(): Promise<void> {
- return this.underlyingExporter.forceFlush
+ return this.underlyingExporter.forceFlush?.()
+ .catch((e) => {
+ console.error("Error in ExternalSpanExporterWrapper.forceFlush:", e);
+ throw e;
+ })
? this.underlyingExporter.forceFlush()
: Promise.resolve();
}
}Committable suggestion skipped: line range outside the PR's diff.
| const externalTraceId = crypto.randomUUID(); | ||
|
|
||
| for (const exporter of config.exporters ?? []) { | ||
| traceProvider.addSpanProcessor( | ||
| getEnvVar("OTEL_BATCH_PROCESSING_ENABLED") === "1" | ||
| ? new BatchSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId), { | ||
| maxExportBatchSize: parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"), | ||
| scheduledDelayMillis: parseInt( | ||
| getEnvVar("OTEL_SPAN_SCHEDULED_DELAY_MILLIS") ?? "200" | ||
| ), | ||
| exportTimeoutMillis: parseInt( | ||
| getEnvVar("OTEL_SPAN_EXPORT_TIMEOUT_MILLIS") ?? "30000" | ||
| ), | ||
| maxQueueSize: parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"), | ||
| }) | ||
| : new SimpleSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId)) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling for exporters configuration
The exporter initialization has several potential issues:
- Missing validation for the exporters array
- No error handling for invalid environment variable values
- Potential memory pressure from unbounded queue sizes
Consider these improvements:
+ // Validate exporters
+ if (config.exporters?.length) {
+ if (!Array.isArray(config.exporters)) {
+ throw new Error("exporters must be an array");
+ }
+
+ for (const exporter of config.exporters) {
+ if (!exporter.export || typeof exporter.export !== "function") {
+ throw new Error("Invalid exporter: missing export function");
+ }
+ }
+ }
+ // Parse and validate env vars with defaults
+ const batchSize = Math.min(
+ parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"),
+ 100 // Maximum safe batch size
+ );
+ const queueSize = Math.min(
+ parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"),
+ 1000 // Maximum safe queue size
+ );
const externalTraceId = crypto.randomUUID();
for (const exporter of config.exporters ?? []) {
traceProvider.addSpanProcessor(
getEnvVar("OTEL_BATCH_PROCESSING_ENABLED") === "1"
? new BatchSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId), {
- maxExportBatchSize: parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"),
+ maxExportBatchSize: batchSize,
scheduledDelayMillis: parseInt(
getEnvVar("OTEL_SPAN_SCHEDULED_DELAY_MILLIS") ?? "200"
),
exportTimeoutMillis: parseInt(
getEnvVar("OTEL_SPAN_EXPORT_TIMEOUT_MILLIS") ?? "30000"
),
- maxQueueSize: parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"),
+ maxQueueSize: queueSize,
})
: new SimpleSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId))
);
}📝 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.
| const externalTraceId = crypto.randomUUID(); | |
| for (const exporter of config.exporters ?? []) { | |
| traceProvider.addSpanProcessor( | |
| getEnvVar("OTEL_BATCH_PROCESSING_ENABLED") === "1" | |
| ? new BatchSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId), { | |
| maxExportBatchSize: parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"), | |
| scheduledDelayMillis: parseInt( | |
| getEnvVar("OTEL_SPAN_SCHEDULED_DELAY_MILLIS") ?? "200" | |
| ), | |
| exportTimeoutMillis: parseInt( | |
| getEnvVar("OTEL_SPAN_EXPORT_TIMEOUT_MILLIS") ?? "30000" | |
| ), | |
| maxQueueSize: parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"), | |
| }) | |
| : new SimpleSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId)) | |
| ); | |
| } | |
| // Validate exporters | |
| if (config.exporters?.length) { | |
| if (!Array.isArray(config.exporters)) { | |
| throw new Error("exporters must be an array"); | |
| } | |
| for (const exporter of config.exporters) { | |
| if (!exporter.export || typeof exporter.export !== "function") { | |
| throw new Error("Invalid exporter: missing export function"); | |
| } | |
| } | |
| } | |
| // Parse and validate env vars with defaults | |
| const batchSize = Math.min( | |
| parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"), | |
| 100 // Maximum safe batch size | |
| ); | |
| const queueSize = Math.min( | |
| parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"), | |
| 1000 // Maximum safe queue size | |
| ); | |
| const externalTraceId = crypto.randomUUID(); | |
| for (const exporter of config.exporters ?? []) { | |
| traceProvider.addSpanProcessor( | |
| getEnvVar("OTEL_BATCH_PROCESSING_ENABLED") === "1" | |
| ? new BatchSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId), { | |
| maxExportBatchSize: batchSize, | |
| scheduledDelayMillis: parseInt( | |
| getEnvVar("OTEL_SPAN_SCHEDULED_DELAY_MILLIS") ?? "200" | |
| ), | |
| exportTimeoutMillis: parseInt( | |
| getEnvVar("OTEL_SPAN_EXPORT_TIMEOUT_MILLIS") ?? "30000" | |
| ), | |
| maxQueueSize: queueSize, | |
| }) | |
| : new SimpleSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId)) | |
| ); | |
| } |
ericallam
force-pushed
the
ea-branch-4
branch
from
e0c6a33 to
5d39e9d
Compare
…entations under the telemetry config property
ericallam
force-pushed
the
ea-branch-4
branch
from
5d39e9d to
d3bd091
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/v3/otel/tracingSDK.ts (1)
116-117: Consider a more specific default service name.The default service name "trigger.dev" is too generic. Consider using a more specific default that includes the package or component name for better observability.
- getEnvVar("OTEL_SERVICE_NAME") ?? "trigger.dev", + getEnvVar("OTEL_SERVICE_NAME") ?? "trigger.dev-core",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/rich-trainers-glow.md(1 hunks)packages/cli-v3/src/config.ts(1 hunks)packages/cli-v3/src/entryPoints/deploy-run-worker.ts(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(1 hunks)packages/core/src/v3/config.ts(2 hunks)packages/core/src/v3/otel/tracingSDK.ts(5 hunks)packages/core/src/v3/semanticInternalAttributes.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(1 hunks)references/nextjs-realtime/package.json(2 hunks)references/nextjs-realtime/src/trigger/ai.ts(2 hunks)references/nextjs-realtime/trigger.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/core/src/v3/semanticInternalAttributes.ts
- packages/cli-v3/src/entryPoints/dev-run-worker.ts
- packages/core/src/v3/workers/taskExecutor.ts
- .changeset/rich-trainers-glow.md
- references/nextjs-realtime/trigger.config.ts
- packages/cli-v3/src/entryPoints/deploy-run-worker.ts
- references/nextjs-realtime/package.json
- packages/cli-v3/src/config.ts
- packages/core/src/v3/config.ts
- references/nextjs-realtime/src/trigger/ai.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🔇 Additional comments (3)
packages/core/src/v3/otel/tracingSDK.ts (3)
23-23: LGTM! Type changes are well-defined.The addition of
ReadableSpanimport and optionalexportersproperty toTracingSDKConfigare appropriate for the new functionality.Also applies to: 89-89
160-177: Previous review comment about validation is still applicable.The suggestion to add validation for exporters and improve error handling for environment variables is still relevant.
Extract batch processing configuration to reduce duplication.
The batch processing configuration is duplicated from the main exporter setup. Consider extracting it into a shared configuration object.
+ const batchConfig = { + maxExportBatchSize: parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"), + scheduledDelayMillis: parseInt( + getEnvVar("OTEL_SPAN_SCHEDULED_DELAY_MILLIS") ?? "200" + ), + exportTimeoutMillis: parseInt( + getEnvVar("OTEL_SPAN_EXPORT_TIMEOUT_MILLIS") ?? "30000" + ), + maxQueueSize: parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"), + }; for (const exporter of config.exporters ?? []) { traceProvider.addSpanProcessor( getEnvVar("OTEL_BATCH_PROCESSING_ENABLED") === "1" - ? new BatchSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId), { - maxExportBatchSize: parseInt(getEnvVar("OTEL_SPAN_MAX_EXPORT_BATCH_SIZE") ?? "64"), - scheduledDelayMillis: parseInt( - getEnvVar("OTEL_SPAN_SCHEDULED_DELAY_MILLIS") ?? "200" - ), - exportTimeoutMillis: parseInt( - getEnvVar("OTEL_SPAN_EXPORT_TIMEOUT_MILLIS") ?? "30000" - ), - maxQueueSize: parseInt(getEnvVar("OTEL_SPAN_MAX_QUEUE_SIZE") ?? "512"), - }) + ? new BatchSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId), batchConfig) : new SimpleSpanProcessor(new ExternalSpanExporterWrapper(exporter, externalTraceId)) ); }
263-307: Previous review comment about type safety and error handling is still applicable.The suggestion to improve type safety, error handling, and add documentation to the ExternalSpanExporterWrapper class is still relevant.
ericallam
changed the title
No description provided.