-
Notifications
You must be signed in to change notification settings - Fork 87
feat(telemetry): print a post-command notice on first run #2156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,3 @@ | ||
| export { DefaultTelemetryClient } from "./client"; | ||
| export { printFirstRunNotice } from "./notice"; | ||
| export { type AttributesOf, type MetricEvent } from "./types"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { test, describe, expect } from "bun:test"; | ||
| import { printFirstRunNotice } from "./notice"; | ||
|
|
||
| describe("printFirstRunNotice", () => { | ||
| test.each([ | ||
| [true, true, 1], | ||
| [true, false, 0], | ||
| [false, true, 0], | ||
| [false, false, 0], | ||
| ])( | ||
| "isFirstRun=%p telemetryEnabled=%p writes the notice %p time(s)", | ||
| (isFirstRun, telemetryEnabled, expectedWrites) => { | ||
| const written: string[] = []; | ||
|
|
||
| printFirstRunNotice(isFirstRun, telemetryEnabled, { | ||
| write: (text) => void written.push(text), | ||
| }); | ||
|
|
||
| expect(written).toHaveLength(expectedWrites); | ||
| if (expectedWrites > 0) { | ||
| expect(written[0]).toContain("collects aggregated, anonymous usage analytics"); | ||
| expect(written[0]).toContain("agentcore config telemetry.enabled false"); | ||
| } | ||
| }, | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /** | ||
| * Writes the telemetry-collection notice to the given stream on the first run of | ||
| * the CLI, unless telemetry is already disabled. | ||
| */ | ||
| export function printFirstRunNotice( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we shift this function under the handler file since it's only used by the handler?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was hoping to keep the root level entrypoint clean, and splitting it out made it easier/more natural to add unit tests, but I agree exporting this from telemetry module for a single consumer adds some indirection that shouldn't be necessary. |
||
| isFirstRun: boolean, | ||
| telemetryEnabled: boolean, | ||
| out: { write(text: string): void }, | ||
| ): void { | ||
| if (!isFirstRun || !telemetryEnabled) return; | ||
|
|
||
| out.write( | ||
| [ | ||
| "", | ||
| "The AgentCore CLI collects aggregated, anonymous usage analytics to help improve the tool.", | ||
| "To opt out: agentcore config telemetry.enabled false", | ||
| "To audit: agentcore config telemetry.audit true", | ||
| "", | ||
| ].join("\n"), | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to return
isFirstRunin the output here? Would that avoid having to make many of these other changes?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this approach, (and actually implemented it initially), but I didn't like the behavior of
getwriting to the config as a side effect since a caller would expect a strict-read here.However, I can see the argument that from the perspective of the caller, it is a strict-read still.
Let me swap to the simpler approach and if we see issues with the side-effect, we can revisit.