Skip to content

Commit 93933a2

Browse files
committed
fix(core): stop custom metric exporters breaking the metrics export
1 parent 1114d9d commit 93933a2

13 files changed

Lines changed: 518 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: 205 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,205 @@
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+
protected async onForceFlush(): Promise<void> {}
92+
93+
protected async onShutdown(): Promise<void> {
94+
throw new Error("reader shutdown failed");
95+
}
96+
}
97+
98+
class RecordingMetricReader extends MetricReader {
99+
forceFlushCount = 0;
100+
shutdownCount = 0;
101+
102+
protected async onForceFlush(): Promise<void> {
103+
this.forceFlushCount++;
104+
}
105+
106+
protected async onShutdown(): Promise<void> {
107+
this.shutdownCount++;
108+
}
109+
}
110+
111+
function captureConsoleErrors(): { lines: string[]; restore: () => void } {
112+
const lines: string[] = [];
113+
const original = console.error;
114+
115+
console.error = (...args: unknown[]) => {
116+
lines.push(args.map(String).join(" "));
117+
};
118+
119+
return { lines, restore: () => (console.error = original) };
120+
}
121+
122+
describe("TracingSDK when one metric reader fails to flush", () => {
123+
let recordingReader: RecordingMetricReader;
124+
let tracingSDK: TracingSDK;
125+
126+
beforeAll(() => {
127+
recordingReader = new RecordingMetricReader();
128+
129+
tracingSDK = new TracingSDK({
130+
url: "http://localhost:1",
131+
forceFlushTimeoutMillis: 5_000,
132+
diagLogLevel: "none",
133+
metricReaders: [new FailingMetricReader(), recordingReader],
134+
});
135+
});
136+
137+
it("still flushes the readers after it", async () => {
138+
await tracingSDK.flush().catch(() => {});
139+
140+
expect(recordingReader.forceFlushCount).toBeGreaterThan(0);
141+
});
142+
143+
it("still reports the failure to the caller", async () => {
144+
await expect(tracingSDK.flush()).rejects.toThrow("reader flush failed");
145+
});
146+
147+
it("logs the failure as it happens", async () => {
148+
const console = captureConsoleErrors();
149+
150+
await tracingSDK.flush().catch(() => {});
151+
console.restore();
152+
153+
expect(console.lines.join("\n")).toContain("reader flush failed");
154+
});
155+
});
156+
157+
class OverlapRecordingMetricReader extends MetricReader {
158+
static inFlight = 0;
159+
static maxInFlight = 0;
160+
161+
protected async onForceFlush(): Promise<void> {}
162+
163+
protected async onShutdown(): Promise<void> {
164+
OverlapRecordingMetricReader.inFlight++;
165+
OverlapRecordingMetricReader.maxInFlight = Math.max(
166+
OverlapRecordingMetricReader.maxInFlight,
167+
OverlapRecordingMetricReader.inFlight
168+
);
169+
await new Promise((resolve) => setTimeout(resolve, 5));
170+
OverlapRecordingMetricReader.inFlight--;
171+
}
172+
}
173+
174+
describe("TracingSDK shutdown", () => {
175+
it("shuts down each metric reader one at a time", async () => {
176+
OverlapRecordingMetricReader.inFlight = 0;
177+
OverlapRecordingMetricReader.maxInFlight = 0;
178+
179+
const tracingSDK = new TracingSDK({
180+
url: "http://localhost:1",
181+
forceFlushTimeoutMillis: 5_000,
182+
diagLogLevel: "none",
183+
metricReaders: [new OverlapRecordingMetricReader(), new OverlapRecordingMetricReader()],
184+
});
185+
186+
await tracingSDK.shutdown().catch(() => {});
187+
188+
expect(OverlapRecordingMetricReader.maxInFlight).toBe(1);
189+
});
190+
191+
it("still shuts down the readers after one that fails", async () => {
192+
const recordingReader = new RecordingMetricReader();
193+
194+
const tracingSDK = new TracingSDK({
195+
url: "http://localhost:1",
196+
forceFlushTimeoutMillis: 5_000,
197+
diagLogLevel: "none",
198+
metricReaders: [new FailingShutdownMetricReader(), recordingReader],
199+
});
200+
201+
await tracingSDK.shutdown().catch(() => {});
202+
203+
expect(recordingReader.shutdownCount).toBeGreaterThan(0);
204+
});
205+
});

packages/core/src/v3/otel/tracingSDK.ts

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ export class TracingSDK {
100100
private readonly _spanExporter: SpanExporter;
101101
private readonly _traceProvider: NodeTracerProvider;
102102
private readonly _meterProvider: MeterProvider;
103+
private readonly _metricReaders: MetricReader[];
103104

104105
public readonly getLogger: LoggerProvider["getLogger"];
105106
public readonly getTracer: TracerProvider["getTracer"];
@@ -318,6 +319,7 @@ export class TracingSDK {
318319
});
319320

320321
this._meterProvider = meterProvider;
322+
this._metricReaders = metricReaders;
321323
metrics.setGlobalMeterProvider(meterProvider);
322324

323325
if (config.hostMetrics) {
@@ -348,15 +350,47 @@ export class TracingSDK {
348350
await Promise.all([
349351
this._traceProvider.forceFlush(),
350352
this._logProvider.forceFlush(),
351-
this._meterProvider.forceFlush(),
353+
this._flushMetricReadersSerially(),
352354
]);
353355
}
354356

357+
private async _flushMetricReadersSerially() {
358+
await this._eachMetricReaderSerially("flush", (reader) => reader.forceFlush());
359+
}
360+
361+
private async _shutdownMetricReadersSerially() {
362+
try {
363+
await this._eachMetricReaderSerially("shut down", (reader) => reader.shutdown());
364+
} finally {
365+
await this._meterProvider.shutdown();
366+
}
367+
}
368+
369+
private async _eachMetricReaderSerially(
370+
action: string,
371+
run: (reader: MetricReader) => Promise<void>
372+
) {
373+
const errors: unknown[] = [];
374+
375+
for (const reader of this._metricReaders) {
376+
try {
377+
await run(reader);
378+
} catch (error) {
379+
console.error(`Failed to ${action} metric reader ${reader.constructor.name}`, error);
380+
errors.push(error);
381+
}
382+
}
383+
384+
if (errors.length > 0) {
385+
throw errors[0];
386+
}
387+
}
388+
355389
public async shutdown() {
356390
await Promise.all([
357391
this._traceProvider.shutdown(),
358392
this._logProvider.shutdown(),
359-
this._meterProvider.shutdown(),
393+
this._shutdownMetricReadersSerially(),
360394
]);
361395
}
362396
}

0 commit comments

Comments
 (0)