Skip to content
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.

Sign up for GitHub

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

Jump to bottom

quieter provider errors with better origin information #207

Merged
merged 1 commit into from
Sep 20, 2024
Merged

quieter provider errors with better origin information #207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 6 additions & 61 deletions client/vscode-lib/src/controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { createCodeLensProvider } from './ui/editor/codeLens.js'
import { createHoverProvider } from './ui/editor/hover.js'
import { createShowFileItemsList } from './ui/fileItemsList.js'
import { createStatusBarItem } from './ui/statusBarItem.js'
import { Cache, bestEffort } from './util/cache.js'
import { ErrorReporterController, UserAction } from './util/errorReporter.js'
import { importProvider } from './util/importHelpers.js'
import { observeWorkspaceConfigurationChanges, toEventEmitter } from './util/observable.js'
Expand Down Expand Up @@ -122,18 +121,15 @@ export function createController({
})
disposables.push(client)

const errorLog = (error: any) => {
console.error(error)
outputChannel.appendLine(error)
}

// errorReporter contains a lot of logic and state on how we notify and log
// errors, as well as state around if we should turn off a feature (see
// skipIfImplicitAction)
const errorReporter = new ErrorReporterController(
createErrorNotifier(outputChannel, extensionId, client),
errorLog,
)
const errorReporter = new ErrorReporterController((error: any, providerUri: string | undefined) => {
const prefix = `OpenCtx error (from provider ${providerUri ?? 'unknown'}): `
console.error(prefix, error)
const errorDetail = error.stack ? `${error} (stack trace follows)\n${error.stack}` : error
outputChannel.appendLine(`${prefix}${errorDetail}`)
})
disposables.push(errorReporter)

// Note: We distingiush between an explicit user action and an implicit
Expand Down Expand Up @@ -211,54 +207,3 @@ function ignoreDoc(params: AnnotationsParams): boolean {
function makeRange(range: Range): vscode.Range {
return new vscode.Range(range.start.line, range.start.character, range.end.line, range.end.character)
}

function createErrorNotifier(
outputChannel: vscode.OutputChannel,
extensionId: string,
client: Pick<VSCodeClient, 'meta'>,
) {
// Fetching the name can be slow or fail. So we use a cache + timeout when
// getting the name of a provider.
const nameCache = new Cache<string>({ ttlMs: 10 * 1000 })
const getName = async (providerUri: string | undefined) => {
if (providerUri === undefined) {
return undefined
}
const fill = async () => {
const meta = await bestEffort(client.meta({}, { providerUri: providerUri }), {
defaultValue: [],
delay: 250,
})
return meta.pop()?.name ?? providerUri
}
return await nameCache.getOrFill(providerUri, fill)
}

const actionItems = [
{
title: 'Show Logs',
do: () => {
outputChannel.show()
},
},
{
title: 'Open Settings',
do: () => {
vscode.commands.executeCommand('workbench.action.openSettings', {
query: `@ext:${extensionId} openctx.providers`,
})
},
},
] satisfies (vscode.MessageItem & { do: () => void })[]

return async (providerUri: string | undefined, error: any) => {
const name = await getName(providerUri)
const message = name
? `Error from OpenCtx provider "${name}": ${error}`
: `Error from OpenCtx: ${error}`
const action = await vscode.window.showErrorMessage(message, ...actionItems)
if (action) {
action.do()
}
}
}
5 changes: 4 additions & 1 deletion client/vscode-lib/src/util/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ export class Cache<T> {
}
}

/** resolves promise, but will return defaultValue if promise throws or takes longer than delay ms */
/**
* Try to resolve {@link promise}, but return {@link opts.defaultValue} if it throws or takes longer than
* {@link opts.delay} ms.
*/
export async function bestEffort<T>(
promise: Promise<T>,
opts: {
Expand Down
41 changes: 2 additions & 39 deletions client/vscode-lib/src/util/errorReporter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import { Observable } from 'observable-fns'
import type * as vscode from 'vscode'
import { type ErrorWaiter, createErrorWaiter } from './errorWaiter.js'

const MIN_TIME_SINCE_LAST_ERROR = 1000 * 60 * 15 /* 15 min */

/**
* The users action affects our behaviour around reporting, so we explicitly
* tell ErrorReporter the intent.
Expand All @@ -29,12 +27,8 @@ export enum UserAction {
*/
export class ErrorReporterController implements vscode.Disposable {
private errorWaiters = new Map<string, ErrorWaiter>()
private errorNotificationVisible = new Set<string>()

constructor(
private showErrorNotification: (providerUri: string | undefined, error: any) => Thenable<any>,
private errorLog: (error: any) => void,
) {}
constructor(private errorLog: (error: any, providerUri: string | undefined) => void) {}

/**
* wraps providerMethod to ensure it reports errors to the user.
Expand Down Expand Up @@ -120,7 +114,7 @@ export class ErrorReporterController implements vscode.Disposable {
errorByUri.set(providerUri, errors)

// Always log the error.
this.errorLog(error)
this.errorLog(error, providerUri)
}

// report takes the seen errors so far and decides if they should be
Expand All @@ -137,21 +131,6 @@ export class ErrorReporterController implements vscode.Disposable {

const errorWaiter = this.getErrorWaiter(providerUri)
errorWaiter.gotError(hasError)

// From here on out it is about notifying for an error
if (!hasError) {
continue
}

// Show an error notification unless we've recently shown one
// (to avoid annoying the user).
const shouldNotify =
userAction === UserAction.Explicit ||
errorWaiter.timeSinceLastError() > MIN_TIME_SINCE_LAST_ERROR
if (shouldNotify) {
const error = errors.length === 1 ? errors[0] : new AggregateError(errors)
this.maybeShowErrorNotification(providerUri, error)
}
}

// Clear out seen errors so that report may be called again.
Expand Down Expand Up @@ -181,22 +160,6 @@ export class ErrorReporterController implements vscode.Disposable {
return errorWaiter
}

private maybeShowErrorNotification(providerUri: string, error: any) {
// We store the notification thenable so we can tell if the last
// notification for providerUri is still showing.
if (this.errorNotificationVisible.has(providerUri)) {
return
}
this.errorNotificationVisible.add(providerUri)
const onfinally = () => this.errorNotificationVisible.delete(providerUri)

// If providerUri is the empty string communicate that via undefined
this.showErrorNotification(providerUri === '' ? undefined : providerUri, error).then(
onfinally,
onfinally,
)
}

public dispose() {
for (const errorWaiter of this.errorWaiters.values()) {
errorWaiter.dispose()
Expand Down
Loading