feat(telemetry): setup client with logging sink by Hweinstock · Pull Request #1846 · aws/agentcore-cli

Hweinstock · 2026-07-27T23:07:02Z

Problem

telemetry is not existent in this new cli.

Solution

We scaffold the telemetry implementation with three core pieces:

MetricSink: the core abstraction for a destination of data. This could be logs, fs, or the collector. Allows us to decouple transport from orchestration logic.
TelemetryClient: the orchestration layer on top of metric sinks that validates metric shapes, and sends the data to the configured destinations.
TelemetryAttributesRecorder: a strongly typed object builder that allows us to set attributes later in the program execution. In the future we can pass this down in the context, and populate it in the handlers. We'll also want some middleware to populate common fields like command and global flags.

Testing

added tests on the client.
verified e2e by inspecting the logs on both executable and node build using the following:
run any command then

> cat ~/.agentcore/logs/output-2026-07-27.log | tail -n 1 | jq
{
  "level": "info",
  "msg": "recording telemetry",
  "time": 1785192842835,
  "cliSessionId": "d1b9e38c-2ec0-4062-9b04-a2034c6bebbf",
  "module": "loggingSink",
  "metricName": "cli.command_run",
  "metricValue": 25,
  "metricAttributes": {
    "service.name": "agentcore-cli",
    "service.version": "0.0.0",
    "agentcore-cli.installation_id": "42edb4ee-1d41-42cf-96ab-195b0356e05a",
    "agentcore-cli.session_id": "d1b9e38c-2ec0-4062-9b04-a2034c6bebbf",
    "os.type": "Linux",
    "os.version": "6.12.94-123.190.amzn2023.x86_64",
    "host.arch": "x64",
    "node.version": "v24.3.0",
    "exit_reason": "success"
  }
}

also failures shows up as failure under exit_reason.

Notes

version requires some more effort to wire up, since we can no longer read the package.json when in an executable. Going to create a follow-up task for this.

codecov-commenter · 2026-07-27T23:08:46Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.00%. Comparing base (0a43f48) to head (e561a2f).
⚠️ Report is 1 commits behind head on refactor.

Additional details and impacted files

@@             Coverage Diff              @@
##           refactor    #1846      +/-   ##
============================================
+ Coverage     94.91%   95.00%   +0.08%     
============================================
  Files           154      160       +6     
  Lines          7455     7581     +126     
============================================
+ Hits           7076     7202     +126     
  Misses          379      379

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Hweinstock · 2026-07-27T23:22:40Z

+
+  private getMetricSinks(): MetricSink[] {
+    if (this.metricSinks !== undefined) return this.metricSinks;
+


this will be async once we add fs sink for audit, so following lazy init pattern from attributes below.

Hweinstock · 2026-07-27T23:31:16Z

+      "os.type": os.type(),
+      "os.version": os.release(),
+      "host.arch": os.arch(),
+      "node.version": process.version,


if I run this via bun, I get the wrong value here since bun hard-codes it for compatibility. However, when using the build for node, it does return the correct version.

Hweinstock · 2026-07-27T23:38:55Z

+      globalConfigAccessor,
+    });
+
+    const commandRunTelemetryRecorder = new TelemetryAttributesRecorder("cli.command_run", {


this is what we can pass down the context (will wire up in a future PR).

aidandaly24

Overall this looks good to me and clean. A few comments, let me know what you think. I wasn't sure on the exact decisions regarding the telemetry preference and if we always want the telemetry LoggingSink.

aidandaly24 · 2026-07-28T17:54:04Z

+  private getMetricSinks(): MetricSink[] {
+    if (this.metricSinks !== undefined) return this.metricSinks;
+
+    this.metricSinks = [new LoggingSink({ logger: this.logger.child({ module: "loggingSink" }) })];


It seems the default Logging sink is created and invoked still without resolving the telemetry preference. The persisted-config case also records again from a second CLI process, so this is not limited to the command that changes the setting.

CONFIG_HOME="$(mktemp -d /tmp/pr1846-config-test.XXXXXX)" ENV_HOME="$(mktemp -d /tmp/pr1846-env-test.XXXXXX)" # Persist the opt-out. This currently records one telemetry entry. HOME="$CONFIG_HOME" bun run src/index.ts config telemetry.enabled false CONFIG_LOG="$(find "$CONFIG_HOME/.agentcore/logs" -name 'output-*.log' -print -quit)" rg -c '"msg":"recording telemetry"' "$CONFIG_LOG" # 1 # Start another process with the persisted opt-out. It records another entry. HOME="$CONFIG_HOME" bun run src/index.ts config telemetry.enabled rg -c '"msg":"recording telemetry"' "$CONFIG_LOG" # 2 # The environment override is also ignored. HOME="$ENV_HOME" AGENTCORE_TELEMETRY_DISABLED=1 bun run src/index.ts config telemetry.enabled ENV_LOG="$(find "$ENV_HOME/.agentcore/logs" -name 'output-*.log' -print -quit)" rg -c '"msg":"recording telemetry"' "$ENV_LOG" # 1

Shouldn't the resolved default sink list be empty when telemetry is disabled?

I left it on because I think we want users to be able to audit telemetry without enabling it sent to the collector. I.e. maybe i disabled it, but I'm curious what would be sent.

I do think we should disable logging when its not being sent or auditted (since its misleading), but I'll address that when I add the next sink.

aidandaly24 · 2026-07-28T17:58:14Z

+    // value describes duration (ms) of the command
+    valueSchema: z.number().min(0),
+  },
+} satisfies Record<string, { attributeSchema: z.ZodType; valueSchema: z.ZodType }>;


From what I could tell the existing implementation defines safeSchema in src/cli/telemetry/schemas/common-shapes.ts to prevent command attributes from containing free-form strings. For example, InvokeAttrs records has_session_id: z.boolean() rather than the session ID itself. This accepts any z.ZodType. I think the safeSchema was probably helpfu so that the privacy constraint remained enforced by the type system, what do you think?

yeah good question, I was planning to add that back once I get into defining the metric schemas.

aidandaly24 · 2026-07-28T18:06:32Z

+  emit<TMetricName extends MetricName>(
+    metricName: TMetricName,
+    metricValue: ValueOf<TMetricName>,
+    attributes: Partial<AttributesOf<TMetricName>>,


nit: emit accepts partial attributes even though it immediately validates them against a schema with required fields. I tested this with {} it passes typechecking, but runtime validation rejects it, the sink receives nothing, and the error is reduced to an internal warning. The recorder can accumulate partial attributes internally, the final emit contract should probably require a complete AttributesOf<TMetricName> or a validated complete object. What do you think?

yeah I went back and forth on this. The challenge is the recorder doesn't know when its attributes are no longer partial. So making this non-partial would require callers of getData on the recorder to be validating the shape, which feels like the job of the telemetry module.

However, I agree the type signatures are awkward. I'm inclined to move this around so that we explicitly end the recorder lifespan (at which point we validate its accumulated attributes), and get the validated data. Then the telemetry client isn't responsible for any validation, and trusts the type signature.

This is kind of similar to what the built-in span support looks like in otel.

jariy17 · 2026-07-28T18:08:32Z

+import type { Logger } from "../logging";
+import type { MetricSink } from "./types";
+
+type LoggingSinkConfig = {


compliment: I love how you decided to make a config object instead of just passing logger. This allows to add more parameters if we need to.

Agreed I really like that system

jariy17 · 2026-07-28T18:47:09Z

+ */
+export interface MetricSink {
+  /** Send data to the given metric sink **/
+  send(metricName: string, value: number, attributes: Record<string, string | number>): void;


Why isn't MetricSink.send() not returning a promise like shutdown?

This is inherited from how otel does it with their sdk client where emitting/sending data is non-blocking, and batched for us in the background. Ideally emit would be non-async as well, but we need to read from globalConfig (on first invoke)

jariy17 · 2026-07-28T18:58:24Z

+  },
+} satisfies Record<string, { attributeSchema: z.ZodType; valueSchema: z.ZodType }>;
+
+export type MetricName = keyof typeof METRICS & string;


Nit: Redundant check—METRICS already enforces that the key is a string.

jariy17 · 2026-07-28T19:02:34Z

+  send(metricName: string, value: number, attributes: Record<string, string | number>): void;
+  /** Flush and close the given metric sink **/
+  shutdown(): Promise<void>;
+  name: string;


Since this is a interface, lets make this a getter function.

jariy17 · 2026-07-28T19:07:06Z

+  private attributes: Partial<AttributesOf<TMetricName>>;
+
+  constructor(
+    _metricName: TMetricName,


We never store this. It would be nice to store this in a private variable for future middleware use.

github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Jul 27, 2026

Hweinstock force-pushed the feat/telemetry-client branch from eef3fe7 to 3268b99 Compare July 27, 2026 23:10

github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Jul 27, 2026

Hweinstock marked this pull request as ready for review July 27, 2026 23:19

Hweinstock marked this pull request as draft July 27, 2026 23:26

Hweinstock force-pushed the feat/telemetry-client branch from 3268b99 to ca1e62a Compare July 27, 2026 23:33

Hweinstock commented Jul 27, 2026

View reviewed changes

Hweinstock marked this pull request as ready for review July 27, 2026 23:37

Hweinstock commented Jul 27, 2026

View reviewed changes

feat(telemetry): setup client with logging sink

5031ae9

Hweinstock force-pushed the feat/telemetry-client branch from ca1e62a to 5031ae9 Compare July 28, 2026 03:27

chore: merge in mainline

8fc7950

aidandaly24 reviewed Jul 28, 2026

View reviewed changes

jariy17 reviewed Jul 28, 2026

View reviewed changes

Hweinstock added 4 commits July 28, 2026 20:57

refactor(telemetry): move validation to the recorder

7c03c41

refactor(telemetry): swap to input for schema to support defaults

d4bffc9

refactor(telemetry): swap to getter for name attribute

69ca5c4

test(telemetry): add back simple test for validation

e561a2f

jariy17 approved these changes Jul 28, 2026

View reviewed changes

aidandaly24 approved these changes Jul 28, 2026

View reviewed changes

Hweinstock merged commit 21353dd into aws:refactor Jul 28, 2026
8 checks passed

Hweinstock deleted the feat/telemetry-client branch July 28, 2026 22:54


		private getMetricSinks(): MetricSink[] {
		if (this.metricSinks !== undefined) return this.metricSinks;

Uh oh!

Conversation

Hweinstock commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Testing

Notes

Uh oh!

codecov-commenter commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidandaly24 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidandaly24 Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hweinstock commented Jul 27, 2026 •

edited

Loading

codecov-commenter commented Jul 27, 2026 •

edited

Loading

aidandaly24 Jul 28, 2026 •

edited

Loading