Skip to content

Commit fa7eea3

Browse files
authored
fix(core): stop custom metric exporters breaking the metrics export (#4613)
## Summary Projects that configure their own `metricExporters` or `metricReaders` in `trigger.config.ts` were losing task metrics on nearly every run, and seeing an unexplained `Failed to flush tracingSDK` alongside `OTLPExporterError: Bad Request` in their run logs. Spans and logs kept working, so the runs otherwise looked healthy. ## Root cause and fix Every configured exporter gets its own `PeriodicExportingMetricReader`, and `meterProvider.forceFlush()` fans out across all readers with `Promise.all`, so two collections can land on the same millisecond. `@opentelemetry/host-metrics` divides by the elapsed interval to compute `process.cpu.utilization` ([common.ts](https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/packages/host-metrics/src/stats/common.ts)), so a zero interval yields `0/0`. `JSON.stringify(NaN)` is `null`, and a collector rejects `"asDouble": null` with a 400 that drops the **entire** request, not just the offending point. `flush()` and `shutdown()` now walk the metric readers one at a time, so collections can no longer share a timestamp. Each reader is isolated, so one failing reader cannot skip the readers behind it, and every failure is logged with the reader that produced it. The first error is still rethrown, so callers see failures exactly as before. As a second layer, non-finite data points are dropped just before our own export, so a metric that divides by zero cannot take the rest of the batch with it. Exporters and readers supplied through `trigger.config.ts` are untouched by that filter and still receive raw data. The trade-off is that configured exporters now flush after the built-in one rather than alongside it, so flush latency is the sum rather than the max. An internal test package's dependency on core was replaced with a local helper, because core now needs that package in `devDependencies` and the two together formed a workspace cycle. ## Verification Tested against a real collector in a container: a batch containing a `NaN` reading is rejected with a 400 without the fix and accepted with it, and a single flush is asserted to collect from one reader at a time.
1 parent 1114d9d commit fa7eea3

13 files changed

Lines changed: 544 additions & 8 deletions

File tree

.changeset/lucky-pillows-invite.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@trigger.dev/core": patch
3+
---
4+
5+
Task metrics no longer go missing for projects that configure their own `metricExporters` or `metricReaders`, and the flush error that came with it is gone.

.github/workflows/unit-tests-packages.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ jobs:
9999
pull redis:7.2
100100
pull testcontainers/ryuk:0.14.0
101101
pull electricsql/electric:1.2.4
102+
pull otel/opentelemetry-collector-k8s:0.158.0@sha256:c09130a633196a5becee164411473a0932ecf223f94fda6dab5f22798ff9f376
102103
echo "Image pre-pull complete"
103104
104105
- name: 📥 Download deps

internal-packages/testcontainers/package.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
"@internal/run-ops-database": "workspace:*",
2323
"@testcontainers/postgresql": "^11.14.0",
2424
"@testcontainers/redis": "^11.14.0",
25-
"@trigger.dev/core": "workspace:*",
2625
"std-env": "^3.9.0",
2726
"testcontainers": "^11.14.0",
2827
"tinyexec": "^0.3.0"

internal-packages/testcontainers/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import {
2929
} from "./utils";
3030

3131
export { assertNonNullable, createPostgresContainer } from "./utils";
32+
export { OtelCollectorContainer, StartedOtelCollectorContainer } from "./otelCollector";
3233
export { laggingReplica, type LaggingModel } from "./laggingReplica";
3334
export { logCleanup };
3435
export type { MinIOConnectionConfig };
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import type { StartedTestContainer } from "testcontainers";
2+
import { AbstractStartedContainer, GenericContainer, Wait } from "testcontainers";
3+
4+
const OTLP_HTTP_PORT = 4318;
5+
const CONFIG_PATH = "/etc/otelcol-config.yaml";
6+
7+
const CONFIG = `receivers:
8+
otlp:
9+
protocols:
10+
http:
11+
endpoint: 0.0.0.0:${OTLP_HTTP_PORT}
12+
exporters:
13+
debug: {}
14+
service:
15+
telemetry:
16+
logs:
17+
level: WARN
18+
pipelines:
19+
traces:
20+
receivers: [otlp]
21+
exporters: [debug]
22+
metrics:
23+
receivers: [otlp]
24+
exporters: [debug]
25+
logs:
26+
receivers: [otlp]
27+
exporters: [debug]
28+
`;
29+
30+
export class OtelCollectorContainer extends GenericContainer {
31+
constructor(
32+
image = "otel/opentelemetry-collector-k8s:0.158.0@sha256:c09130a633196a5becee164411473a0932ecf223f94fda6dab5f22798ff9f376"
33+
) {
34+
super(image);
35+
this.withExposedPorts(OTLP_HTTP_PORT);
36+
this.withCopyContentToContainer([{ content: CONFIG, target: CONFIG_PATH }]);
37+
this.withCommand([`--config=${CONFIG_PATH}`]);
38+
this.withWaitStrategy(Wait.forHttp("/v1/metrics", OTLP_HTTP_PORT).forStatusCode(405));
39+
this.withStartupTimeout(120_000);
40+
}
41+
42+
public override async start(): Promise<StartedOtelCollectorContainer> {
43+
return new StartedOtelCollectorContainer(await super.start());
44+
}
45+
}
46+
47+
export class StartedOtelCollectorContainer extends AbstractStartedContainer {
48+
constructor(startedTestContainer: StartedTestContainer) {
49+
super(startedTestContainer);
50+
}
51+
52+
public getPort(): number {
53+
return super.getMappedPort(OTLP_HTTP_PORT);
54+
}
55+
56+
/**
57+
* Base URL for OTLP/HTTP, without a signal path.
58+
* Example: `http://localhost:32768`
59+
*/
60+
public getOtlpHttpUrl(): string {
61+
return `http://${this.getHost()}:${this.getPort()}`;
62+
}
63+
}

internal-packages/testcontainers/src/utils.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { PostgreSqlContainer } from "@testcontainers/postgresql";
44
import type { StartedRedisContainer } from "@testcontainers/redis";
55
import { RedisContainer } from "@testcontainers/redis";
66
import { PrismaClient } from "@trigger.dev/database";
7-
import { tryCatch } from "@trigger.dev/core";
87
import Redis from "ioredis";
98
import path from "path";
109
import { isDebug } from "std-env";
@@ -16,6 +15,14 @@ import { ClickHouseContainer, runClickhouseMigrations } from "./clickhouse";
1615
import { MinIOContainer } from "./minio";
1716
import { getContainerMetadata, getTaskMetadata, logCleanup, logSetup } from "./logs";
1817

18+
async function tryCatch<T, E = Error>(promise: Promise<T>): Promise<[E, null] | [null, T]> {
19+
try {
20+
return [null, await promise];
21+
} catch (error) {
22+
return [error as E, null];
23+
}
24+
}
25+
1926
/** Returns the container's connection URI with the database path swapped to `database`. */
2027
export function postgresUriWithDatabase(uri: string, database: string): string {
2128
const url = new URL(uri);

packages/core/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,7 @@
233233
"@ai-sdk/provider-utils": "^1.0.22",
234234
"@arethetypeswrong/cli": "^0.18.5",
235235
"@epic-web/test-server": "^0.1.0",
236+
"@internal/testcontainers": "workspace:*",
236237
"@trigger.dev/database": "workspace:*",
237238
"@types/humanize-duration": "^3.27.1",
238239
"@types/lodash.get": "^4.4.9",
Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
import {
2+
OtelCollectorContainer,
3+
type StartedOtelCollectorContainer,
4+
} from "@internal/testcontainers";
5+
6+
import { metrics } from "@opentelemetry/api";
7+
import { ExportResultCode } from "@opentelemetry/core";
8+
import {
9+
MetricReader,
10+
type PushMetricExporter,
11+
type ResourceMetrics,
12+
} from "@opentelemetry/sdk-metrics";
13+
import { afterAll, beforeAll, describe, expect, it } from "vitest";
14+
import { TracingSDK } from "./tracingSDK.js";
15+
16+
class NoopMetricExporter implements PushMetricExporter {
17+
forceFlushCount = 0;
18+
19+
export(_metrics: ResourceMetrics, resultCallback: (result: { code: number }) => void): void {
20+
resultCallback({ code: ExportResultCode.SUCCESS });
21+
}
22+
23+
async forceFlush(): Promise<void> {
24+
this.forceFlushCount++;
25+
}
26+
27+
async shutdown(): Promise<void> {}
28+
}
29+
30+
describe("TracingSDK with an external metric exporter", () => {
31+
let collector: StartedOtelCollectorContainer;
32+
let tracingSDK: TracingSDK;
33+
34+
beforeAll(async () => {
35+
collector = await new OtelCollectorContainer().start();
36+
37+
process.env.TRIGGER_OTEL_METRICS_COLLECTION_INTERVAL_MILLIS = "600000";
38+
39+
tracingSDK = new TracingSDK({
40+
url: collector.getOtlpHttpUrl(),
41+
forceFlushTimeoutMillis: 30_000,
42+
diagLogLevel: "none",
43+
metricExporters: [new NoopMetricExporter()],
44+
hostMetrics: true,
45+
hostMetricGroups: ["process.cpu", "process.memory"],
46+
nodejsRuntimeMetrics: true,
47+
});
48+
}, 180_000);
49+
50+
afterAll(async () => {
51+
await tracingSDK?.shutdown();
52+
await collector?.stop();
53+
delete process.env.TRIGGER_OTEL_METRICS_COLLECTION_INTERVAL_MILLIS;
54+
});
55+
56+
it("flushes without the collector rejecting a batch containing a NaN reading", async () => {
57+
const gauge = metrics.getMeter("test").createObservableGauge("test.utilization");
58+
gauge.addCallback((result) => result.observe(NaN));
59+
60+
await expect(tracingSDK.flush()).resolves.toBeUndefined();
61+
});
62+
63+
it("collects from each metric reader one at a time", async () => {
64+
let inFlight = 0;
65+
let maxInFlight = 0;
66+
67+
const gauge = metrics.getMeter("test").createObservableGauge("test.concurrency");
68+
gauge.addCallback(async (result) => {
69+
inFlight++;
70+
maxInFlight = Math.max(maxInFlight, inFlight);
71+
await new Promise((resolve) => setTimeout(resolve, 5));
72+
result.observe(1);
73+
inFlight--;
74+
});
75+
76+
await tracingSDK.flush();
77+
78+
expect(maxInFlight).toBe(1);
79+
});
80+
});
81+
82+
class FailingMetricReader extends MetricReader {
83+
protected async onForceFlush(): Promise<void> {
84+
throw new Error("reader flush failed");
85+
}
86+
87+
protected async onShutdown(): Promise<void> {}
88+
}
89+
90+
class FailingShutdownMetricReader extends MetricReader {
91+
shutdownAttempts = 0;
92+
93+
protected async onForceFlush(): Promise<void> {}
94+
95+
protected async onShutdown(): Promise<void> {
96+
this.shutdownAttempts++;
97+
throw new Error(`reader shutdown failed (attempt ${this.shutdownAttempts})`);
98+
}
99+
}
100+
101+
class RecordingMetricReader extends MetricReader {
102+
forceFlushCount = 0;
103+
shutdownCount = 0;
104+
105+
protected async onForceFlush(): Promise<void> {
106+
this.forceFlushCount++;
107+
}
108+
109+
protected async onShutdown(): Promise<void> {
110+
this.shutdownCount++;
111+
}
112+
}
113+
114+
function captureConsoleErrors(): { lines: string[]; restore: () => void } {
115+
const lines: string[] = [];
116+
const original = console.error;
117+
118+
console.error = (...args: unknown[]) => {
119+
lines.push(args.map(String).join(" "));
120+
};
121+
122+
return { lines, restore: () => (console.error = original) };
123+
}
124+
125+
describe("TracingSDK when one metric reader fails to flush", () => {
126+
let recordingReader: RecordingMetricReader;
127+
let tracingSDK: TracingSDK;
128+
129+
beforeAll(() => {
130+
recordingReader = new RecordingMetricReader();
131+
132+
tracingSDK = new TracingSDK({
133+
url: "http://localhost:1",
134+
forceFlushTimeoutMillis: 5_000,
135+
diagLogLevel: "none",
136+
metricReaders: [new FailingMetricReader(), recordingReader],
137+
});
138+
});
139+
140+
it("still flushes the readers after it", async () => {
141+
await tracingSDK.flush().catch(() => {});
142+
143+
expect(recordingReader.forceFlushCount).toBeGreaterThan(0);
144+
});
145+
146+
it("still reports the failure to the caller", async () => {
147+
await expect(tracingSDK.flush()).rejects.toThrow("reader flush failed");
148+
});
149+
150+
it("logs the failure as it happens", async () => {
151+
const console = captureConsoleErrors();
152+
153+
await tracingSDK.flush().catch(() => {});
154+
console.restore();
155+
156+
expect(console.lines.join("\n")).toContain("reader flush failed");
157+
});
158+
});
159+
160+
class OverlapRecordingMetricReader extends MetricReader {
161+
static inFlight = 0;
162+
static maxInFlight = 0;
163+
164+
protected async onForceFlush(): Promise<void> {}
165+
166+
protected async onShutdown(): Promise<void> {
167+
OverlapRecordingMetricReader.inFlight++;
168+
OverlapRecordingMetricReader.maxInFlight = Math.max(
169+
OverlapRecordingMetricReader.maxInFlight,
170+
OverlapRecordingMetricReader.inFlight
171+
);
172+
await new Promise((resolve) => setTimeout(resolve, 5));
173+
OverlapRecordingMetricReader.inFlight--;
174+
}
175+
}
176+
177+
describe("TracingSDK shutdown", () => {
178+
it("shuts down each metric reader one at a time", async () => {
179+
OverlapRecordingMetricReader.inFlight = 0;
180+
OverlapRecordingMetricReader.maxInFlight = 0;
181+
182+
const tracingSDK = new TracingSDK({
183+
url: "http://localhost:1",
184+
forceFlushTimeoutMillis: 5_000,
185+
diagLogLevel: "none",
186+
metricReaders: [new OverlapRecordingMetricReader(), new OverlapRecordingMetricReader()],
187+
});
188+
189+
await tracingSDK.shutdown().catch(() => {});
190+
191+
expect(OverlapRecordingMetricReader.maxInFlight).toBe(1);
192+
});
193+
194+
it("still shuts down the readers after one that fails", async () => {
195+
const recordingReader = new RecordingMetricReader();
196+
197+
const tracingSDK = new TracingSDK({
198+
url: "http://localhost:1",
199+
forceFlushTimeoutMillis: 5_000,
200+
diagLogLevel: "none",
201+
metricReaders: [new FailingShutdownMetricReader(), recordingReader],
202+
});
203+
204+
await tracingSDK.shutdown().catch(() => {});
205+
206+
expect(recordingReader.shutdownCount).toBeGreaterThan(0);
207+
});
208+
209+
it("does not retry a metric reader that failed to shut down", async () => {
210+
const failingReader = new FailingShutdownMetricReader();
211+
212+
const tracingSDK = new TracingSDK({
213+
url: "http://localhost:1",
214+
forceFlushTimeoutMillis: 5_000,
215+
diagLogLevel: "none",
216+
metricReaders: [failingReader],
217+
});
218+
219+
await tracingSDK.shutdown().catch(() => {});
220+
221+
expect(failingReader.shutdownAttempts).toBe(1);
222+
});
223+
224+
it("reports the original shutdown failure, not a later one", async () => {
225+
const tracingSDK = new TracingSDK({
226+
url: "http://localhost:1",
227+
forceFlushTimeoutMillis: 5_000,
228+
diagLogLevel: "none",
229+
metricReaders: [new FailingShutdownMetricReader()],
230+
});
231+
232+
await expect(tracingSDK.shutdown()).rejects.toThrow("attempt 1");
233+
});
234+
});

0 commit comments

Comments
 (0)