feat(tel): populate base attributes for cli.command_run metrics. - #1870
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #1870 +/- ##
============================================
- Coverage 95.87% 95.86% -0.01%
============================================
Files 198 198
Lines 9379 9417 +38
============================================
+ Hits 8992 9028 +36
- Misses 387 389 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a8615b3 to
adaf6b5
Compare
tejaskash
left a comment
There was a problem hiding this comment.
Only one comment worth looking at
| c.action(async (...actionArgs: unknown[]) => { | ||
| const command = actionArgs[actionArgs.length - 1] as Command; | ||
| const merged = command.optsWithGlobals(); | ||
|
|
||
| const telemetryAttributesRecorder = ctx.value(TelemetryAttributesRecorderKey); | ||
| const commandPath = ctx.require(PathKey); | ||
|
|
||
| telemetryAttributesRecorder?.record({ | ||
| command_path: commandPath, | ||
| }); | ||
|
|
There was a problem hiding this comment.
This records the command path inside Commander’s action callback. Commander rejects missing mandatory flags, unknown options, and help requests before invoking that callback. Verified this as below:
agentcore config telemetry.audit true
agentcore runtime invoke || true
jq '.attrs' "$(ls -t ~/.agentcore/telemetry/*.jsonl | head -n 1)"
Verified output includes:
{
"exit_reason": "failure",
"command_path": "unknown"
}
There was a problem hiding this comment.
Good catch, was able to handle this by intercepting the error on the way up. Since double recording is a no-op, this should be a safe approach. I wasn't able to find a single place to move this such that it always worked, but lmk if you have any ideas.
| /** Adds a compile time check that the given object has no free-form fields to prevent PII leakage **/ | ||
| function safeSchema<T extends Record<string, EnumeratedField>>(shape: T) { | ||
| return z.object(shape); | ||
| } |
There was a problem hiding this comment.
I think this only protects the declared schema at compile time, so it can still be bypassed at the sink boundary. I tested a typed attributes value containing prompt: "review-secret" through DefaultTelemetryClient.emit, and the sink received prompt because emit never parses attributeSchema. I think emit should probably parse and forward the validated attributes, what do you think? This is obviously a two way door.
There was a problem hiding this comment.
Yeah I think the issue is that there is an implicit assumption that the client's emit is called with the recorder's output. The choices are to make that assumption explicit, or to parse the schema twice.
I think I'm going to make it explicit by passing the recorder in directly to avoid this.
There was a problem hiding this comment.
refactored this one more time. I moved emit onto the recorder (which is now called the metric event). I think this is much simpler since the interface becomes, createMetricEvent on the client then emit on the event for callers, instead of having to pass the metric/recorder around.
| // TODO: add error details to telemetry recorder; | ||
| commandRunTelemetryRecorder.record({ exit_reason: "failure" }); | ||
|
|
||
| commandRunTelemetryRecorder.record({ |
There was a problem hiding this comment.
--help currently gets recorded as a failed command even though it exits 0. With audit enabled I got exit_reason: "failure" and error_source: "internal". Should CommanderError be handled using its exit code so help remains successful and nonzero usage errors are classified as user failures? Unless is there a reason that you didn't go with that?
There was a problem hiding this comment.
yeah good catch. I don't think we handle commander errors at the root level classification so they end up being unknown. I can pick that up as a follow-up. I think we just need some logic in AgentCoreCLIError.fromError to check if its a commander error and map its fields to our fields.
meant to just leave as comment since these are really just questions
03aa1dc to
da404c0
Compare
da404c0 to
da68951
Compare
6459e65 to
0139beb
Compare
Problem
Solution
Passing the Recorder Down
We pass the attributes recorder into the router directly instead of the root handler. This allows us to add the command path as an attribute BEFORE flags/args are validated. Note that middleware is evaluated after flags/args are validated, so we can't inject it there.
For detecting TUI behavior, we add the attribute at the root TUI entrypoint leveraging the already injected recorder from the context.
Defining Shapes
For best security practice, we want protections against bugs in the CLI leaking PII into telemetry. Therefore, we define a factory function
safeSchemathat adds a compile time check to our schema that all fields are explicitly enumerated (boolean, enum, literal, etc.).However, we declare two exceptions to this rule:
AgentCoreCLIErrors, we know that their names must describe a subclass of the base error and therefore be an error we defined. We still provide minimal validation to our input in case the name field is overwritten with user input by mistake.Both fields provide fallback values that be used to detect bugs in the implementation.
Testing
also tested e2e with audit enabled:
Then
cat ~/.agentcore/telemetry/* | jqgives us:Notes
If the TUI fails to fetch data, that error doesn't hit the root handler, and therefore marks the result as success. This is reasonable because the TUI did open, and the command isn't valid. Opened a thread on this, and will change behavior in a follow-up if we think this doesn't make sense.