-
Notifications
You must be signed in to change notification settings - Fork 19
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
Use the Foyle API to report Log Events #1589
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
4d8d400
Start writing the code to log events.
jlewi b70ac9f
Update foyle buf package to pull in the events protos.
jlewi 11b4bda
Rough version of the code.
jlewi 528e083
Update the connect package for Foyle.
jlewi be7841d
Latest version; to try to show the problematic line
jlewi 891ddb8
Bug fix: AIManager doesn't properly get the configuration for baseURL
jlewi af7a3c7
Include context in execution events.
jlewi 306b002
Fix test.
jlewi 10a946d
Define a sessionmanager to manager the context ID so we can attach it…
jlewi fd2f569
Include the selected index in the events.
jlewi 326411f
Log accepted suggestions.
jlewi b04b1ea
Log session starts and ends.
jlewi 5427337
Merge in BaseURLFix
jlewi 78376d0
Revert examples.
jlewi de0a3b5
Initialize the logger properly.
jlewi 0e6e2aa
Add ULID
jlewi e76015b
Add a ULID to events
jlewi f487c58
Merge in upstream/main.
jlewi 361dc8b
Add return
jlewi cf45b35
Add todo
sourishkrout d136c1b
Add tests to AI event logging (#1602)
sourishkrout File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
import { PromiseClient } from '@connectrpc/connect' | ||
import { AIService } from '@buf/jlewi_foyle.connectrpc_es/foyle/v1alpha1/agent_connect' | ||
import { | ||
LogEventsRequest, | ||
LogEventType, | ||
LogEvent, | ||
} from '@buf/jlewi_foyle.bufbuild_es/foyle/v1alpha1/agent_pb' | ||
import * as vscode from 'vscode' | ||
import { ulid } from 'ulidx' | ||
|
||
import { RUNME_CELL_ID } from '../constants' | ||
import getLogger from '../logger' | ||
|
||
import { SessionManager } from './sessions' | ||
import * as converters from './converters' | ||
|
||
// Interface for the event reporter | ||
// This allows us to swap in a null op logger when AI isn't enabled | ||
export interface IEventReporter { | ||
reportExecution(cell: vscode.NotebookCell): Promise<void> | ||
reportEvents(events: LogEvent[]): Promise<void> | ||
} | ||
|
||
// EventReporter handles reporting events to the AI service | ||
export class EventReporter implements IEventReporter { | ||
client: PromiseClient<typeof AIService> | ||
log: ReturnType<typeof getLogger> | ||
|
||
constructor(client: PromiseClient<typeof AIService>) { | ||
this.client = client | ||
this.log = getLogger('AIEventReporter') | ||
} | ||
|
||
async reportExecution(cell: vscode.NotebookCell) { | ||
const contextCells: vscode.NotebookCell[] = [] | ||
|
||
// Include some previous cells as context. | ||
// N.B. In principle we shouldn't need to send any additional context because we | ||
// set the context id. So as soon as we put the focus on the execution cell we should | ||
// start a streaming generate request which will include the entire notebook or a large portion of it. | ||
// However, we still send some additional context here for two reasons | ||
// 1. Help us verify that sending context ids is working correctly. | ||
// 2. Its possible in the future we start rate limiting streaming generate requests and don't want to rely on it | ||
// for providing the context of the cell execution. | ||
let startIndex = cell.index - 1 | ||
if (startIndex < 0) { | ||
startIndex = 0 | ||
} | ||
for (let i = startIndex; i < cell.index; i++) { | ||
contextCells.push(cell.notebook.cellAt(i)) | ||
} | ||
|
||
contextCells.push(cell) | ||
const cells = converters.vsCellsToESProtos(contextCells) | ||
const event = new LogEvent() | ||
event.selectedId = cell.metadata?.[RUNME_CELL_ID] | ||
event.selectedIndex = cell.index | ||
event.type = LogEventType.EXECUTE | ||
event.cells = cells | ||
event.contextId = SessionManager.getManager().getID() | ||
return this.reportEvents([event]) | ||
} | ||
|
||
async reportEvents(events: LogEvent[]) { | ||
const req = new LogEventsRequest() | ||
req.events = events | ||
for (const event of events) { | ||
if (event.eventId === '') { | ||
event.eventId = ulid() | ||
} | ||
} | ||
await this.client.logEvents(req).catch((e) => { | ||
this.log.error(`Failed to log event; error: ${e}`) | ||
}) | ||
} | ||
} | ||
|
||
// NullOpEventReporter is a null op implementation of the event reporter | ||
export class NullOpEventReporter implements IEventReporter { | ||
async reportExecution(_cell: vscode.NotebookCell) { | ||
// Do nothing | ||
} | ||
|
||
async reportEvents(_events: LogEvent[]) { | ||
// Do nothing | ||
} | ||
} | ||
|
||
let _globalReporter = new NullOpEventReporter() | ||
|
||
// getEventReporter returns the global event reporter | ||
export function getEventReporter(): IEventReporter { | ||
return _globalReporter | ||
} | ||
|
||
export function setEventReporter(reporter: IEventReporter) { | ||
_globalReporter = reporter | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
return instead of await here otherwise the call stack won't account for asynchronicity
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.
actually if the return value is irrelevant the
await
will doThere 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 think if we use a return here it will change the return type to Promise
I think Promise is the better return type because I don't think the caller of the reporter should be trying to process any results of the reporter.