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.

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

Merged
merged 5 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 90 additions & 0 deletions components/dashboard/resolver.js
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
Copy link
Member Author

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

node resolver.js <<EOF
<copy paste stack trace here>
EOF

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);
}
12 changes: 11 additions & 1 deletion components/dashboard/src/service/service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand All @@ -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));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prevents hitting very often if we get something like unauthenticated

}
})();
}
Expand Down
26 changes: 23 additions & 3 deletions components/public-api/typescript/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,43 @@
"license": "AGPL-3.0",
"main": "./lib/index.js",
"types": "./lib/index.d.ts",
"module": "./lib/esm/index.js",
"files": [
"lib"
],
"exports": {
".": {
"types": "./lib/index.d.ts",
"import": "./lib/esm/index.js",
"require": "./lib/index.js"
},
"./lib/*": {
"types": "./lib/*.d.ts",
"import": "./lib/esm/*.js",
"require": "./lib/*.js"
},
"./lib/gitpod/experimental/v1": {
"types": "./lib/gitpod/experimental/v1/index.d.ts",
"import": "./lib/esm/gitpod/experimental/v1/index.js",
"require": "./lib/gitpod/experimental/v1/index.js"
}
},
"scripts": {
"build": "mkdir -p lib; tsc",
"build": "yarn run build:cjs && yarn run build:esm",
"build:cjs": "tsc",
"build:esm": "tsc --module es2015 --outDir ./lib/esm",
"watch": "leeway exec --package .:lib --transitive-dependencies --filter-type yarn --components --parallel -- tsc -w --preserveWatchOutput",
"test": "mocha --opts mocha.opts './**/*.spec.ts' --exclude './node_modules/**'",
"test:brk": "yarn test --inspect-brk"
},
"dependencies": {
"@bufbuild/connect": "^0.13.0",
"@bufbuild/protobuf": "^0.1.1",
"@bufbuild/protoc-gen-connect-web": "^0.2.1",
"@bufbuild/protoc-gen-es": "^0.1.1",
"prom-client": "^14.2.0"
},
"devDependencies": {
"@bufbuild/protoc-gen-connect-web": "^0.2.1",
"@bufbuild/protoc-gen-es": "^0.1.1",
"@testdeck/mocha": "0.1.2",
"@types/chai": "^4.1.2",
"@types/node": "^16.11.0",
Expand Down
3 changes: 2 additions & 1 deletion components/public-api/typescript/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"rootDir": "src",
"experimentalDecorators": true,
"outDir": "lib",
"declarationDir": "lib",
"lib": [
"es6",
"esnext.asynciterable",
Expand All @@ -14,7 +15,7 @@
"emitDecoratorMetadata": true,
"strictPropertyInitialization": false,
"downlevelIteration": true,
"module": "commonjs",
"module": "CommonJS",
"moduleResolution": "node",
"target": "es6",
"jsx": "react",
Expand Down
34 changes: 19 additions & 15 deletions components/server/src/api/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to find out a root cause on server

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.error("public api: unexpected internal error", reason);
log.error("public api: unexpected internal error", reason);

// don't leak internal errors to a user
// TODO(ak) instead surface request id
err = ConnectError.from(`please check server logs`, Code.Internal);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the message users will see afterwards? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The 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> {
Expand All @@ -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);
}
})();
}
Expand All @@ -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);
}
})();
};
Expand All @@ -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);
}
Expand Down