Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
26 changes: 26 additions & 0 deletions bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,15 @@
"@aws-sdk/client-bedrock-agentcore": "^3.1092.0",
"@aws-sdk/client-bedrock-agentcore-control": "^3.1079.0",
"@aws-sdk/client-iam": "^3.1080.0",
"@opentelemetry/api": "^1.9.1",
"@opentelemetry/exporter-metrics-otlp-http": "^0.221.0",
"@opentelemetry/resources": "^2.10.0",
"@opentelemetry/sdk-metrics": "^2.10.0",
"@smithy/core": "3.29.3",
"@tanstack/react-query": "^5.101.2",
"cli-truncate": "^6.1.1",
"commander": "^15.0.0",
"handlebars": "^4.7.9",
"ink": "^7.1.0",
"ink-scroll-view": "^0.3.7",
"lodash": "^4.18.1",
Expand All @@ -65,7 +70,6 @@
"string-width": "^8.2.2",
"winston": "^3.19.0",
"winston-daily-rotate-file": "^5.0.0",
"handlebars": "^4.7.9",
"zod": "^4.4.3"
}
}
130 changes: 129 additions & 1 deletion src/telemetry/client.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ import os, { tmpdir } from "node:os";
import { DefaultTelemetryClient } from "./client";
import { createFileLogger, type Logger } from "../logging";
import { LOG_LEVEL } from "../logging";
import { assertLogsMatch, TestGlobalConfigAccessor } from "../testing";
import { assertLogsMatch, createSilentLogger, TestGlobalConfigAccessor } from "../testing";
import type { MetricSink } from "./types";
import { FileSystemSink } from "./fileSystemSink";
import { DEFAULT_GLOBAL_CONFIG } from "../globalConfig";
import { PACKAGE_VERSION } from "../constants";

describe("DefaultTelemetryClient", () => {
Expand All @@ -28,9 +29,20 @@ describe("DefaultTelemetryClient", () => {

test("emits complete metrics to configured JSONL filesystem sinks", async () => {
const auditFilePath = join(tempDir, "telemetry", "audit.jsonl");
const sinkResourceAttributes = {
"service.name": "agentcore-cli" as const,
"service.version": "0.0.0",
"agentcore-cli.installation_id": "00000000-0000-0000-0000-000000000000",
"agentcore-cli.session_id": "00000000-0000-0000-0000-000000000000",
"os.type": os.type(),
"os.version": os.release(),
"host.arch": os.arch(),
"node.version": process.version,
};
const fileSystemSink = new FileSystemSink({
logger: logger.child({ module: "fileSystemSink" }),
filePath: auditFilePath,
resourceAttributes: sinkResourceAttributes,
});
const sessionId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
const globalConfigAccessor = new TestGlobalConfigAccessor();
Expand Down Expand Up @@ -191,6 +203,16 @@ describe("DefaultTelemetryClient", () => {
const sink = new FileSystemSink({
logger: logger.child({ module: "fileSystemSink" }),
filePath: tempDir,
resourceAttributes: {
"service.name": "agentcore-cli",
"service.version": "0.0.0",
"agentcore-cli.installation_id": "00000000-0000-0000-0000-000000000000",
"agentcore-cli.session_id": "00000000-0000-0000-0000-000000000000",
"os.type": os.type(),
"os.version": os.release(),
"host.arch": os.arch(),
"node.version": process.version,
},
});

const client = new DefaultTelemetryClient({
Expand Down Expand Up @@ -274,3 +296,109 @@ describe("DefaultTelemetryClient", () => {
]);
});
});

describe("OtelHistogramSink", () => {
let testCollector: ReturnType<typeof Bun.serve>;
let receivedBodies: any[];

const logger = createSilentLogger();

beforeEach(async () => {
receivedBodies = [];
testCollector = Bun.serve({
port: 0,
async fetch(req) {
const body = await req.json();
receivedBodies.push(body);
return new Response("", { status: 200 });
},
});
});

afterEach(async () => {
testCollector.stop(true);
});

test.each([
{ enabled: true, expectRequests: true },
{ enabled: false, expectRequests: false },
])(
"telemetry.enabled=$enabled → collector receives requests=$expectRequests",
async ({ enabled, expectRequests }) => {
const sessionId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
const exitReason = "success";
const commandPath = "/agentcore";
const metricName = "cli.command_run";
const scopeName = "agentcore-cli";
const serviceName = "agentcore-cli";
const globalConfigAccessor = new TestGlobalConfigAccessor({
initialConfigData: {
...DEFAULT_GLOBAL_CONFIG,
telemetry: {
enabled,
audit: false,
endpoint: `http://localhost:${testCollector.port}`,
},
},
});

const client = new DefaultTelemetryClient({
logger,
sessionId,
globalConfigAccessor,
});

const event = client.createMetricEvent(metricName, {
exit_reason: exitReason,
command_path: commandPath,
});
await event.emit(100);
await client.shutdown();

if (expectRequests) {
expect(receivedBodies.length).toBeGreaterThan(0);

const body = receivedBodies[0];
expect(body).toMatchObject({
resourceMetrics: [
{
resource: {
attributes: expect.arrayContaining([
{ key: "service.name", value: { stringValue: serviceName } },
{
key: "agentcore-cli.session_id",
value: { stringValue: sessionId },
},
{ key: "os.type", value: { stringValue: os.type() } },
{ key: "host.arch", value: { stringValue: os.arch() } },
]),
},
scopeMetrics: [
{
scope: { name: scopeName },
metrics: [
{
name: metricName,
histogram: {
dataPoints: [
{
attributes: expect.arrayContaining([
{ key: "exit_reason", value: { stringValue: exitReason } },
{ key: "command_path", value: { stringValue: commandPath } },
]),
},
],
},
},
],
},
],
},
],
});
} else {
expect(receivedBodies).toHaveLength(0);
}
},
);
});
12 changes: 12 additions & 0 deletions src/telemetry/client.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import type { GlobalConfigAccessor } from "../globalConfig";
import { FileSystemSink } from "./fileSystemSink";
import path from "path";
import { OtelHistogramSink } from "./otelSink";
import { PACKAGE_VERSION } from "../constants";

export type DefaultTelemetryClientConfig = {
Expand Down Expand Up @@ -72,6 +73,7 @@ export class DefaultTelemetryClient implements TelemetryClient {

private getMetricSinks: () => Promise<MetricSink[]> = once(async () => {
if (this.metricSinksOverride) return this.metricSinksOverride;
const resourceAttributes = await this.getResourceAttributes();

const metricSinks = [];

Expand All @@ -82,6 +84,16 @@ export class DefaultTelemetryClient implements TelemetryClient {
new FileSystemSink({
logger: this.logger.child({ module: "fileSystemSink" }),
filePath: this.auditFilePath,
resourceAttributes,
}),
);

if (globalConfig.telemetry.enabled)
metricSinks.push(
new OtelHistogramSink({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems like a wrong/malformed endpoint makes getMetricSinks() reject, and shutdown() propagates that rejection, erroring out in the CLI command. Is that understanding correct? can we make it best-effort?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we (the dev team) should be the only ones modifying the endpoint for testing purposes. In which case, I think the ideal behavior is that we reject early.

If a user decides to go into the global config and add an invalid override, I think rejecting is reasonable.

logger: this.logger.child({ module: "otelCollectorSink" }),
collectorEndpoint: globalConfig.telemetry.endpoint,
resourceAttributes,
}),
);

Expand Down
7 changes: 6 additions & 1 deletion src/telemetry/fileSystemSink.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import type { Logger } from "../logging";
import type { ResourceAttributes } from "./shapes";
import type { MetricSink } from "./types";
import { mkdir, appendFile } from "fs/promises";
import { dirname } from "path";

export type FileSystemSinkConfig = {
logger: Logger;
filePath: string;
resourceAttributes: ResourceAttributes;
};

/** An implementation of {@link MetricSink} that sends all data to the specified file in JSONL format **/
Expand All @@ -15,13 +17,16 @@ export class FileSystemSink implements MetricSink {
private readonly filePath: string;
private logger: Logger;

private readonly resourceAttributes: ResourceAttributes;

/* a chain of promises describing the pending writes to the audit file */
private pendingWrite: Promise<void>;

constructor(config: FileSystemSinkConfig) {
this.filePath = config.filePath;
this.logger = config.logger.child({ fsSinkFilePath: this.filePath });
this.name = new.target.name;
this.resourceAttributes = config.resourceAttributes;

this.pendingWrite = Promise.resolve();
}
Expand All @@ -32,7 +37,7 @@ export class FileSystemSink implements MetricSink {
attributes: Record<string, string | number | boolean>,
): void {
this.pendingWrite = this.pendingWrite.then(() =>
this.appendEntry({ metricName, value, attrs: attributes }),
this.appendEntry({ metricName, value, attrs: { ...this.resourceAttributes, ...attributes } }),
);
}

Expand Down
97 changes: 97 additions & 0 deletions src/telemetry/otelSink.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
import { MeterProvider, PeriodicExportingMetricReader } from "@opentelemetry/sdk-metrics";
import { resourceFromAttributes } from "@opentelemetry/resources";
import type { Logger } from "../logging";
import { type MetricSink } from "./types";
import { OTLPMetricExporter } from "@opentelemetry/exporter-metrics-otlp-http";
import { type Histogram, type Meter } from "@opentelemetry/api";
import type { ResourceAttributes } from "./shapes";

export type OtelCollectorSinkConfig = {
collectorEndpoint: string;
logger: Logger;
/** The resource attributes to attach to all metrics.**/
resourceAttributes: ResourceAttributes;
/** The time period between export flushes **/
exportIntervalMs?: number;
/** Describes the maximum time to wait when flushing metrics to the sink.**/
flushTimeoutMs?: number;
/** Describes the maximum time to wait when shutting down the sink.**/
shutdownTimeoutMs?: number;
/**
* Describes the scope to attach to the given metrics. Defaults to `agentcore-cli`
* See https://opentelemetry.io/docs/concepts/instrumentation-scope/ for more information.
**/
instrumentationScope?: string;
};

/**
* An implementation of {@link MetricSink} that sends data to a collector using an otel histogram.
*/
export class OtelHistogramSink implements MetricSink {
private readonly name: string;
private readonly endpoint: string;
private meterProvider: MeterProvider;
private histograms: Map<string, Histogram>;
private logger: Logger;

private readonly flushTimeoutMs: number;
private readonly shutdownTimeoutMs: number;
private readonly scopedMeter: Meter;

constructor(config: OtelCollectorSinkConfig) {
this.endpoint = config.collectorEndpoint;
this.logger = config.logger.child({ telemetryEndpoint: config.collectorEndpoint });
this.name = new.target.name;

this.flushTimeoutMs = config.flushTimeoutMs ?? 500;
this.shutdownTimeoutMs = config.shutdownTimeoutMs ?? 500;

this.meterProvider = new MeterProvider({
resource: resourceFromAttributes(config.resourceAttributes),
readers: [
new PeriodicExportingMetricReader({
exporter: new OTLPMetricExporter({
url: this.endpoint.endsWith("/v1/metrics")
? this.endpoint
: `${this.endpoint}/v1/metrics`,
}),
exportIntervalMillis: config.exportIntervalMs ?? 5_000,
}),
],
});
this.scopedMeter = this.meterProvider.getMeter(config.instrumentationScope ?? "agentcore-cli");
this.histograms = new Map<string, Histogram>();
}

private getHistogram(metricName: string): Histogram {
if (!this.histograms.has(metricName)) {
this.histograms.set(metricName, this.scopedMeter.createHistogram(metricName));
}
return this.histograms.get(metricName)!;
}

send(metricName: string, value: number, attributes: Record<string, string | number>): void {
this.logger
.child({ metricName, metricValue: value, metricAttributes: attributes })
.info(`sending telemetry metric to collector`);

this.getHistogram(metricName).record(value, attributes);
}

async shutdown(): Promise<void> {
try {
await this.meterProvider.forceFlush({ timeoutMillis: this.flushTimeoutMs });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

outside this try block, there is meterProvider.shutdown() which also flushes the metric again.

https://opentelemetry.io/docs/specs/otel/metrics/sdk/#shutdown

can we ensure one export somehow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method provides a way for provider to do any cleanup required.

Shutdown MUST be called only once for each MeterProvider instance. After the call to Shutdown, subsequent attempts to get a Meter are not allowed. SDKs SHOULD return a valid no-op Meter for these calls, if possible.

Shutdown SHOULD provide a way to let the caller know whether it succeeded, failed or timed out.

Shutdown SHOULD complete or abort within some timeout. Shutdown MAY be implemented as a blocking API or an asynchronous API which notifies the caller via a callback or an event. [OpenTelemetry SDK](https://opentelemetry.io/docs/specs/otel/overview/#sdk) authors MAY decide if they want to make the shutdown timeout configurable.

Shutdown MUST be implemented at least by invoking Shutdown on all registered [MetricReader](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#metricreader) and [MetricExporter](https://opentelemetry.io/docs/specs/otel/metrics/sdk/#metricexporter) instances.

from https://opentelemetry.io/docs/specs/otel/metrics/sdk/#shutdown.

I don't see any explicit lines in the protocol linked for shutdown that it also flushes. Based on some testing, I think it does internally, but I think its safer to make that behavior explicit. If we flush twice, its a no-op anyway.

} catch (e) {
const error = e instanceof Error ? e : new Error(String(e));
this.logger
.child({ errorName: error.name, errorMessage: error.message })
.warn(`failed to flush metrics to ${this.getName()}`);
// don't let flush failures prevent shutdown
}
await this.meterProvider.shutdown({ timeoutMillis: this.shutdownTimeoutMs });
}

getName(): string {
return this.name;
}
}
Loading