-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[public-api] improve error handling #18739
Changes from 4 commits
4a22b88
1520c88
806ce8f
e417a85
66eb7fd
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 |
---|---|---|
@@ -0,0 +1,90 @@ | ||
/** | ||
* Copyright (c) 2023 Gitpod GmbH. All rights reserved. | ||
* Licensed under the GNU Affero General Public License (AGPL). | ||
* See License.AGPL.txt in the project root for license information. | ||
*/ | ||
//@ts-check | ||
const path = require('path'); | ||
const fetch = require('node-fetch').default; | ||
const { SourceMapConsumer } = require('source-map'); | ||
|
||
const sourceMapCache = {}; | ||
|
||
function extractJsUrlFromLine(line) { | ||
const match = line.match(/https?:\/\/[^\s]+\.js/); | ||
return match ? match[0] : null; | ||
} | ||
|
||
async function fetchSourceMap(jsUrl) { | ||
// Use cached source map if available | ||
if (sourceMapCache[jsUrl]) { | ||
return sourceMapCache[jsUrl]; | ||
} | ||
|
||
const jsResponse = await fetch(jsUrl); | ||
const jsContent = await jsResponse.text(); | ||
|
||
// Extract source map URL from the JS file | ||
const mapUrlMatch = jsContent.match(/\/\/#\s*sourceMappingURL=(.+)/); | ||
if (!mapUrlMatch) { | ||
throw new Error('Source map URL not found'); | ||
} | ||
|
||
const mapUrl = new URL(mapUrlMatch[1], jsUrl).href; // Resolve relative URL | ||
const mapResponse = await fetch(mapUrl); | ||
const mapData = await mapResponse.json(); | ||
|
||
// Cache the fetched source map | ||
sourceMapCache[jsUrl] = mapData; | ||
|
||
return mapData; | ||
} | ||
|
||
const BASE_PATH = '/workspace/gitpod/components'; | ||
|
||
async function resolveLine(line) { | ||
const jsUrl = extractJsUrlFromLine(line); | ||
if (!jsUrl) return line; | ||
|
||
const rawSourceMap = await fetchSourceMap(jsUrl); | ||
const matches = line.match(/at (?:([\S]+) )?\(?(https?:\/\/[^\s]+\.js):(\d+):(\d+)\)?/); | ||
|
||
if (!matches) { | ||
return line; | ||
} | ||
|
||
const functionName = matches[1] || ''; | ||
const lineNum = Number(matches[3]); | ||
const colNum = Number(matches[4]); | ||
|
||
const consumer = new SourceMapConsumer(rawSourceMap); | ||
const originalPosition = consumer.originalPositionFor({ line: lineNum, column: colNum }); | ||
|
||
if (originalPosition && originalPosition.source) { | ||
const fullPath = path.join(BASE_PATH, originalPosition.source); | ||
const originalFunctionName = originalPosition.name || functionName; | ||
return ` at ${originalFunctionName} (${fullPath}:${originalPosition.line}:${originalPosition.column})`; | ||
} | ||
|
||
return line; | ||
} | ||
|
||
|
||
let obfuscatedTrace = ''; | ||
|
||
process.stdin.on('data', function(data) { | ||
obfuscatedTrace += data; | ||
}); | ||
|
||
process.stdin.on('end', async function() { | ||
const lines = obfuscatedTrace.split('\n'); | ||
const resolvedLines = await Promise.all(lines.map(resolveLine)); | ||
const resolvedTrace = resolvedLines.join('\n'); | ||
console.log('\nResolved Stack Trace:\n'); | ||
console.log(resolvedTrace); | ||
}); | ||
|
||
if (process.stdin.isTTY) { | ||
console.error("Please provide the obfuscated stack trace either as a multi-line input or from a file."); | ||
process.exit(1); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,10 @@ function testPublicAPI(service: any): void { | |
}); | ||
(async () => { | ||
const grpcType = "server-stream"; | ||
const MAX_BACKOFF = 60000; | ||
const BASE_BACKOFF = 3000; | ||
let backoff = BASE_BACKOFF; | ||
|
||
// emulates server side streaming with public API | ||
while (true) { | ||
const isTest = await getExperimentsClient().getValueAsync("public_api_dummy_reliability_test", false, { | ||
|
@@ -121,15 +125,21 @@ function testPublicAPI(service: any): void { | |
let previousCount = 0; | ||
for await (const reply of helloService.lotsOfReplies({ previousCount })) { | ||
previousCount = reply.count; | ||
backoff = BASE_BACKOFF; | ||
} | ||
} catch (e) { | ||
console.error(e, { | ||
userId: user?.id, | ||
grpcType, | ||
}); | ||
backoff = Math.min(2 * backoff, MAX_BACKOFF); | ||
} | ||
} else { | ||
backoff = BASE_BACKOFF; | ||
} | ||
await new Promise((resolve) => setTimeout(resolve, 3000)); | ||
const jitter = Math.random() * 0.3 * backoff; | ||
const delay = backoff + jitter; | ||
await new Promise((resolve) => setTimeout(resolve, delay)); | ||
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. prevents hitting very often if we get something like unauthenticated |
||
} | ||
})(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
"emitDecoratorMetadata": true, | ||
"strictPropertyInitialization": false, | ||
"downlevelIteration": true, | ||
"module": "commonjs", | ||
"module": "ES6", | ||
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. actual fix on client side, to import only like es6, not both commonjs and es6 |
||
"moduleResolution": "node", | ||
"target": "es6", | ||
"jsx": "react", | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -24,7 +24,6 @@ import { APIStatsService } from "./stats"; | |||||
import { APITeamsService } from "./teams"; | ||||||
import { APIUserService } from "./user"; | ||||||
import { APIWorkspacesService } from "./workspaces"; | ||||||
import { Deferred } from "@gitpod/gitpod-protocol/lib/util/deferred"; | ||||||
|
||||||
function service<T extends ServiceType>(type: T, impl: ServiceImpl<T>): [T, ServiceImpl<T>] { | ||||||
return [type, impl]; | ||||||
|
@@ -55,7 +54,7 @@ export class API { | |||||
this.register(app); | ||||||
|
||||||
const server = app.listen(3001, () => { | ||||||
log.info(`Connect Public API server listening on port: ${(server.address() as AddressInfo).port}`); | ||||||
log.info(`public api: listening on port: ${(server.address() as AddressInfo).port}`); | ||||||
}); | ||||||
|
||||||
return server; | ||||||
|
@@ -126,13 +125,22 @@ export class API { | |||||
|
||||||
grpcServerStarted.labels(grpc_service, grpc_method, grpc_type).inc(); | ||||||
const stopTimer = grpcServerHandling.startTimer({ grpc_service, grpc_method, grpc_type }); | ||||||
const deferred = new Deferred<ConnectError | undefined>(); | ||||||
// eslint-disable-next-line @typescript-eslint/no-floating-promises | ||||||
deferred.promise.then((err) => { | ||||||
const done = (err?: ConnectError) => { | ||||||
const grpc_code = err ? Code[err.code] : "OK"; | ||||||
grpcServerHandled.labels(grpc_service, grpc_method, grpc_type, grpc_code).inc(); | ||||||
stopTimer({ grpc_code }); | ||||||
}); | ||||||
}; | ||||||
const handleError = (reason: unknown) => { | ||||||
let err = ConnectError.from(reason, Code.Internal); | ||||||
if (reason != err && err.code === Code.Internal) { | ||||||
console.error("public api: unexpected internal error", reason); | ||||||
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. to find out a root cause on server 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.
Suggested change
|
||||||
// don't leak internal errors to a user | ||||||
// TODO(ak) instead surface request id | ||||||
err = ConnectError.from(`please check server logs`, Code.Internal); | ||||||
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. Is this the message users will see afterwards? 🤔 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. yes, I want to add a request id here later that if reported we can look it up on our side, do you think it would be better if we propagate unexpected server error to the client? |
||||||
} | ||||||
done(err); | ||||||
throw err; | ||||||
}; | ||||||
|
||||||
const context = args[1] as HandlerContext; | ||||||
async function call<T>(): Promise<T> { | ||||||
|
@@ -146,12 +154,10 @@ export class API { | |||||
try { | ||||||
const promise = await call<Promise<any>>(); | ||||||
const result = await promise; | ||||||
deferred.resolve(undefined); | ||||||
done(); | ||||||
return result; | ||||||
} catch (e) { | ||||||
const err = ConnectError.from(e); | ||||||
deferred.resolve(e); | ||||||
throw err; | ||||||
handleError(e); | ||||||
} | ||||||
})(); | ||||||
} | ||||||
|
@@ -161,11 +167,9 @@ export class API { | |||||
for await (const item of generator) { | ||||||
yield item; | ||||||
} | ||||||
deferred.resolve(undefined); | ||||||
done(); | ||||||
} catch (e) { | ||||||
const err = ConnectError.from(e); | ||||||
deferred.resolve(err); | ||||||
throw err; | ||||||
handleError(e); | ||||||
} | ||||||
})(); | ||||||
}; | ||||||
|
@@ -174,7 +178,7 @@ export class API { | |||||
} | ||||||
|
||||||
private async verify(context: HandlerContext) { | ||||||
const user = await this.sessionHandler.verify(context.requestHeader.get("cookie")); | ||||||
const user = await this.sessionHandler.verify(context.requestHeader.get("cookie") || ""); | ||||||
geropl marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (!user) { | ||||||
throw new ConnectError("unauthenticated", Code.Unauthenticated); | ||||||
} | ||||||
|
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.
it allows to reconstruct stack traces from obfuscated errors, use like