feat: Add first party eve instrumentation#2203
Conversation
…rumentation # Conflicts: # js/src/logger.ts
Abhijeet Prasad (AbhiPrasad)
left a comment
There was a problem hiding this comment.
no orchestrion config? Or iitm setup?
| recordInputs: false, | ||
| recordOutputs: false, |
There was a problem hiding this comment.
why are these false? Should we make them user configurable?
There was a problem hiding this comment.
These are options that defineInstrumentation() uses. When they are enabled, the otel data when turned on contains input and output data. Since awe are grabbing the input and output data from hook calls instead of instrumentation it doesn't matter.
| private sessionsById = new Map<string, SessionState>(); | ||
| private completedToolKeys = new Set<string>(); | ||
| private toolsByCallKey = new Map<string, ToolState>(); | ||
| private turnsByKey = new Map<string, TurnState>(); |
There was a problem hiding this comment.
should we bound? Like use an LRU to manage memory?
| this.state.update((current) => ({ | ||
| ...normalizeEveTraceState(current), | ||
| stepStarts: [ | ||
| ...state.stepStarts, |
There was a problem hiding this comment.
should we use current here? Ditto with some other places where we reference state instead of current.
| } | ||
| try { | ||
| await this.handleEvent(event, ctx, hookMetadata); | ||
| await this.flushInstrumentation(); |
There was a problem hiding this comment.
should we only call flushInstrumentation for events we handle?
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "braintrust": patch | |||
There was a problem hiding this comment.
should be a minor!
| } | ||
| } | ||
|
|
||
| class EveBridge { |
There was a problem hiding this comment.
kind feels weird there is no createTurn and endTurn methods on the bridge here.
There was a problem hiding this comment.
We have handleTurnStarted and handleTurnCompleted and handleTurnFailed. Is that sufficient?
There was a problem hiding this comment.
Hmm in general the lifecycle here felt hard for me to follow. I didn't think too deeply about it though.
| turnId: string, | ||
| stepIndex: number, | ||
| ): Promise<string> { | ||
| return deterministicEveId("eve:step", sessionId, turnId, String(stepIndex)); |
There was a problem hiding this comment.
I think we can have
type EveEntityKind = "session" | "step" | "subagent" | "tool" | "turn";
async function generateEveIds(
kind: EveEntityKind,
...parts: string[]
): Promise<{ rowId: string; spanId: string }> {
const [rowId, spanId] = await Promise.all([
deterministicEveId(`eve:row:${kind}`, ...parts),
deterministicEveId(`eve:${kind}`, ...parts),
]);
return { rowId, spanId };
}and take care of most of the use cases, and then we just need rootSpanIdForSession and spanIdForSubagent?
| type ToolState = SpanState & { | ||
| endedByTurn?: boolean; | ||
| kind: "subagent" | "tool"; | ||
| stepIndex?: number; | ||
| turnKey: string; | ||
| }; |
There was a problem hiding this comment.
clanker tells me ToolState.kind and ToolState.stepIndex were written at every creation site but never read.
Ok this instrumentation is a bit interesting:
The way someone instruments their app is via:
plus