Skip to content
Draft
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions apps/search/src/instrumentations/otel-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,30 @@ import {
ATTR_SERVICE_INSTANCE_ID,
} from "@opentelemetry/semantic-conventions/incubating";
import { createBatchSpanProcessor } from "@saleor/apps-otel/src/batch-span-processor-factory";
import { DeferredSampler } from "@saleor/apps-otel/src/deferred-sampler";
import { createHttpInstrumentation } from "@saleor/apps-otel/src/http-instrumentation-factory";
import { ObservabilityAttributes } from "@saleor/apps-otel/src/observability-attributes";
import { createServiceInstanceId } from "@saleor/apps-otel/src/service-instance-id-factory";
import { TailSamplingProcessor } from "@saleor/apps-otel/src/tail-sampling-processor";
import { registerOTel } from "@vercel/otel";

import pkg from "../../package.json";

const batchProcessor = createBatchSpanProcessor({
accessToken: process.env.OTEL_ACCESS_TOKEN,
});

const tailSamplingProcessor = new TailSamplingProcessor({
processor: batchProcessor,
slowThresholdMs: 5000,
exportErrors: true,
exportSlowSpans: true,
Comment on lines +16 to +25
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The tail sampling configuration values (slowThresholdMs: 5000, exportErrors: true, exportSlowSpans: true) are hardcoded. Consider extracting these to environment variables or a configuration file to allow tuning in different environments (development, staging, production) without code changes.

Suggested change
const batchProcessor = createBatchSpanProcessor({
accessToken: process.env.OTEL_ACCESS_TOKEN,
});
const tailSamplingProcessor = new TailSamplingProcessor({
processor: batchProcessor,
slowThresholdMs: 5000,
exportErrors: true,
exportSlowSpans: true,
// Tail sampling configuration from environment variables with defaults
const TAIL_SAMPLING_SLOW_THRESHOLD_MS = process.env.TAIL_SAMPLING_SLOW_THRESHOLD_MS
? parseInt(process.env.TAIL_SAMPLING_SLOW_THRESHOLD_MS, 10)
: 5000;
const TAIL_SAMPLING_EXPORT_ERRORS = process.env.TAIL_SAMPLING_EXPORT_ERRORS
? process.env.TAIL_SAMPLING_EXPORT_ERRORS.toLowerCase() === "true"
: true;
const TAIL_SAMPLING_EXPORT_SLOW_SPANS = process.env.TAIL_SAMPLING_EXPORT_SLOW_SPANS
? process.env.TAIL_SAMPLING_EXPORT_SLOW_SPANS.toLowerCase() === "true"
: true;
const batchProcessor = createBatchSpanProcessor({
accessToken: process.env.OTEL_ACCESS_TOKEN,
});
const tailSamplingProcessor = new TailSamplingProcessor({
processor: batchProcessor,
slowThresholdMs: TAIL_SAMPLING_SLOW_THRESHOLD_MS,
exportErrors: TAIL_SAMPLING_EXPORT_ERRORS,
exportSlowSpans: TAIL_SAMPLING_EXPORT_SLOW_SPANS,

Copilot uses AI. Check for mistakes.
});

registerOTel({
serviceName: process.env.OTEL_SERVICE_NAME,
// Note: DeferredSampler + TailSamplingProcessor must be used together
traceSampler: new DeferredSampler(),
attributes: {
[ATTR_SERVICE_VERSION]: pkg.version,
[ATTR_DEPLOYMENT_ENVIRONMENT_NAME]: process.env.ENV,
Expand All @@ -23,10 +38,6 @@ registerOTel({
env: undefined,
[ObservabilityAttributes.VERCEL_ENV]: process.env.VERCEL_ENV,
},
spanProcessors: [
createBatchSpanProcessor({
accessToken: process.env.OTEL_ACCESS_TOKEN,
}),
],
instrumentations: [createHttpInstrumentation()],
spanProcessors: [tailSamplingProcessor],
instrumentations: [createHttpInstrumentation({ usingDeferredSpanProcessor: true })],
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The parameter name "usingDeferredSpanProcessor" is misleading. It refers to using a DeferredSampler, not a DeferredSpanProcessor. Consider using "usingDeferredSampler" or "allowRootSpans" for clarity.

Suggested change
instrumentations: [createHttpInstrumentation({ usingDeferredSpanProcessor: true })],
instrumentations: [createHttpInstrumentation({ usingDeferredSampler: true })],

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The TailSamplingFetchInstrumentation is not being added to the instrumentations array. According to the documentation in fetch-instrumentation.ts, this instrumentation was created specifically to support tail sampling by recording span data for non-sampled spans. Without it, fetch calls won't be properly instrumented for tail sampling. The instrumentations array should include an instance of TailSamplingFetchInstrumentation.

Copilot uses AI. Check for mistakes.
});
1 change: 1 addition & 0 deletions cspell.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export default {
"claude",
"SEPA",
"mailpit",
"traceparent"
],
language: "en-US",
useGitignore: true,
Expand Down
13 changes: 11 additions & 2 deletions packages/otel/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
"scripts": {
"check-types": "tsc",
"lint": "eslint .",
"lint:fix": "eslint --fix ."
"lint:fix": "eslint --fix .",
"test": "vitest",
"test:ci": "vitest run --coverage"
Comment on lines +9 to +10
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The package.json defines test scripts but there's no vitest.config.ts file in the packages/otel directory. Based on other apps in the repository (like stripe and np-atobarai), a vitest configuration file is needed to properly set up the test environment, specify test file patterns, and configure test setup files.

Copilot uses AI. Check for mistakes.
Comment on lines +9 to +10
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

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

The otel package now includes test scripts but is missing a vitest.config.ts file. Other apps in the repository (like apps/np-atobarai, apps/stripe) have vitest configuration files that define test workspaces, setup files, and environment settings. Without this configuration, vitest may not find the test files or run with the correct settings.

Create a vitest.config.ts file similar to other packages in the monorepo, defining the test workspace configuration for unit tests.

Copilot uses AI. Check for mistakes.
Comment on lines 5 to +10
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

The test commands reference setup files (pattern seen in other packages), but there is no setup-tests.ts file in the otel package src directory. This file is needed to configure the test environment according to repository conventions.

Copilot uses AI. Check for mistakes.
},
"dependencies": {
"@opentelemetry/exporter-metrics-otlp-http": "catalog:",
Expand All @@ -18,24 +20,31 @@
},
"devDependencies": {
"@opentelemetry/api": "catalog:",
"@opentelemetry/instrumentation": "catalog:",
"@opentelemetry/sdk-metrics": "catalog:",
"@opentelemetry/sdk-trace-node": "catalog:",
"@opentelemetry/semantic-conventions": "catalog:",
"@saleor/app-sdk": "link:../../node_modules/@saleor/app-sdk",
"@saleor/eslint-config-apps": "workspace:*",
"@saleor/typescript-config-apps": "workspace:*",
"@types/node": "catalog:",
"@vercel/otel": "catalog:",
"@vitest/coverage-v8": "catalog:",
"eslint": "catalog:",
"next": "catalog:",
"typescript": "catalog:",
"urql": "catalog:"
"urql": "catalog:",
"vite": "catalog:",
"vitest": "catalog:"
},
"peerDependencies": {
"@opentelemetry/api": "catalog:",
"@opentelemetry/instrumentation": "catalog:",
"@opentelemetry/sdk-metrics": "catalog:",
"@opentelemetry/sdk-trace-node": "catalog:",
"@opentelemetry/semantic-conventions": "catalog:",
"@saleor/app-sdk": "catalog:",
"@vercel/otel": "catalog:",
"next": "catalog:",
"urql": "catalog:"
Comment on lines +47 to 49
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

Adding @vercel/otel as a peer dependency means that any package consuming @saleor/apps-otel must install @vercel/otel even if they don't use the tail sampling features. Consider whether tail sampling functionality should be optional or if @vercel/otel should remain only a dev dependency. If TailSamplingFetchInstrumentation is the only export requiring @vercel/otel, you might want to make it an optional peer dependency or split it into a separate package.

Suggested change
"@vercel/otel": "catalog:",
"next": "catalog:",
"urql": "catalog:"
"next": "catalog:",
"urql": "catalog:"
},
"optionalDependencies": {
"@vercel/otel": "catalog:"

Copilot uses AI. Check for mistakes.
}
Expand Down
79 changes: 79 additions & 0 deletions packages/otel/src/deferred-sampler.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { ROOT_CONTEXT, SpanKind, trace, TraceFlags } from "@opentelemetry/api";
import { SamplingDecision } from "@opentelemetry/sdk-trace-node";
import { describe, expect, it } from "vitest";

import { DeferredSampler, SALEOR_SAMPLING_DECISION_ATTR } from "./deferred-sampler";

describe("DeferredSampler", () => {
const traceId = "0af7651916cd43dd8448eb211c80319c";
const spanName = "test-span";
const spanKind = SpanKind.SERVER;
const attributes = {};
const links: never[] = [];

describe("when parent is sampled", () => {
it("should return RECORD_AND_SAMPLED decision", () => {
const sampler = new DeferredSampler();
const parentContext = trace.setSpanContext(ROOT_CONTEXT, {
traceId,
spanId: "b7ad6b7169203331",
traceFlags: TraceFlags.SAMPLED,
isRemote: true,
});

const result = sampler.shouldSample(
parentContext,
traceId,
spanName,
spanKind,
attributes,
links,
);

expect(result.decision).toBe(SamplingDecision.RECORD_AND_SAMPLED);
expect(result.attributes?.[SALEOR_SAMPLING_DECISION_ATTR]).toBe("sampled");
});
});

describe("when parent is not sampled", () => {
it("should return RECORD decision (defer to TailSamplingProcessor)", () => {
const sampler = new DeferredSampler();
const parentContext = trace.setSpanContext(ROOT_CONTEXT, {
traceId,
spanId: "b7ad6b7169203331",
traceFlags: TraceFlags.NONE,
isRemote: true,
});

const result = sampler.shouldSample(
parentContext,
traceId,
spanName,
spanKind,
attributes,
links,
);

expect(result.decision).toBe(SamplingDecision.RECORD);
expect(result.attributes?.[SALEOR_SAMPLING_DECISION_ATTR]).toBe("not_sampled");
});
});

describe("when there is no parent (root span)", () => {
it("should return RECORD decision (defer to TailSamplingProcessor)", () => {
const sampler = new DeferredSampler();

const result = sampler.shouldSample(
ROOT_CONTEXT,
traceId,
spanName,
spanKind,
attributes,
links,
);

expect(result.decision).toBe(SamplingDecision.RECORD);
expect(result.attributes?.[SALEOR_SAMPLING_DECISION_ATTR]).toBe("none");
});
});
});
68 changes: 68 additions & 0 deletions packages/otel/src/deferred-sampler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { Attributes, Context, Link, SpanKind, trace, TraceFlags } from "@opentelemetry/api";
import { Sampler, SamplingDecision, SamplingResult } from "@opentelemetry/sdk-trace-node";

/**
* Attribute key to store Saleor's original sampling decision.
* Used by TailSamplingProcessor to know if Saleor wanted this trace.
*/
export const SALEOR_SAMPLING_DECISION_ATTR = "saleor.sampling.decision";

/**
* Check if SAMPLED flag is set in traceFlags bitmask.
* Uses bitwise AND operation according to OTEL spec
*/
function isSampled(traceFlags: number): boolean {
return (traceFlags & TraceFlags.SAMPLED) !== 0;
}

/**
* A sampler that defers the final sampling decision to span end.
*
* - When parent is SAMPLED → return RECORD_AND_SAMPLED (respects parent)
* - When parent is NOT SAMPLED or NO parent → return RECORD (defer decision to TailSamplingProcessor)
*
* This allows the TailSamplingProcessor to make the final decision
* based on error status or latency at span end.
* Without setting `RECORD` we wouldn't store any span data during runtime
*/
export class DeferredSampler implements Sampler {
constructor() {}

Comment thread
witoszekdev marked this conversation as resolved.
Outdated
// eslint-disable-next-line @typescript-eslint/max-params -- Required by OpenTelemetry Sampler interface
shouldSample(
context: Context,
_traceId: string,
_spanName: string,
_spanKind: SpanKind,
_attributes: Attributes,
_links: Link[],
): SamplingResult {
const parentSpanContext = trace.getSpanContext(context);
const parentSampled = parentSpanContext && isSampled(parentSpanContext.traceFlags);

if (parentSampled) {
// Parent decided to sample - we MUST sample too
return {
decision: SamplingDecision.RECORD_AND_SAMPLED,
attributes: {
[SALEOR_SAMPLING_DECISION_ATTR]: "sampled",
},
};
}
Comment on lines +41 to +49
Copy link

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

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

When a parent span is sampled, this returns RECORD_AND_SAMPLED for the child span, which bypasses TailSamplingProcessor entirely. This means child spans under a sampled parent won't benefit from tail sampling logic (e.g., they won't get the TAIL_SAMPLING_PROMOTED_ATTR attribute or Vercel/Datadog resource attributes added by TailSamplingProcessor). This creates inconsistency where some spans in a trace have these attributes and others don't, which could affect observability and filtering. Consider if TailSamplingProcessor should process all spans, or document this limitation.

Suggested change
if (parentSampled) {
// Parent decided to sample - we MUST sample too
return {
decision: SamplingDecision.RECORD_AND_SAMPLED,
attributes: {
[SALEOR_SAMPLING_DECISION_ATTR]: "sampled",
},
};
}
// Always return RECORD to allow TailSamplingProcessor to make the final export decision,
// even if the parent is sampled. This ensures all spans can be processed for tail sampling
// and receive consistent attributes (e.g., TAIL_SAMPLING_PROMOTED_ATTR).
return {
decision: SamplingDecision.RECORD,
attributes: {
[SALEOR_SAMPLING_DECISION_ATTR]: parentSampled ? "sampled" : (parentSpanContext ? "not_sampled" : "none"),
},
};

Copilot uses AI. Check for mistakes.

/*
* No parent OR parent decided NOT to sample.
* Record span but defer export decision to TailSamplingProcessor.
*/
return {
decision: SamplingDecision.RECORD,
attributes: {
[SALEOR_SAMPLING_DECISION_ATTR]: parentSpanContext ? "not_sampled" : "none",
},
};
}

toString(): string {
return "DeferredSampler";
}
}
Loading
Loading