Skip to content

Commit 59d5a54

Browse files
akosyakovakosyakov
committed
[public-api] improve request exception logging
1 parent 3e79652 commit 59d5a54

File tree

7 files changed

+77
-42
lines changed

7 files changed

+77
-42
lines changed

‎components/dashboard/src/service/metrics.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,16 +53,19 @@ export function instrumentWebSocket(ws: WebSocket, origin: string) {
5353
export function reportError(...args: any[]) {
5454
let err = undefined;
5555
let details = undefined;
56+
let requestContext = undefined;
5657
if (args[0] instanceof Error) {
5758
err = args[0];
5859
details = args[1];
60+
requestContext = (args[0] as any)["requestContext"];
5961
} else if (typeof args[0] === "string") {
6062
err = new Error(args[0]);
6163
if (args[1] instanceof Error) {
6264
err.message += ": " + args[1].message;
6365
err.name = args[1].name;
6466
err.stack = args[1].stack;
6567
details = args[2];
68+
requestContext = (args[1] as any)["requestContext"];
6669
} else if (typeof args[1] === "string") {
6770
err.message += ": " + args[1];
6871
details = args[2];
@@ -71,6 +74,10 @@ export function reportError(...args: any[]) {
7174
}
7275
}
7376

77+
if (!err) {
78+
return;
79+
}
80+
7481
let data = {};
7582
if (details && typeof details === "object") {
7683
data = Object.assign(
@@ -90,8 +97,9 @@ export function reportError(...args: any[]) {
9097
),
9198
);
9299
}
93-
94-
if (err) {
95-
metricsReporter.reportError(err, data);
100+
if (requestContext && typeof requestContext === "object") {
101+
data = Object.assign(data, requestContext);
96102
}
103+
104+
metricsReporter.reportError(err, data);
97105
}

‎components/dashboard/src/service/public-api.ts

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import { JsonRpcVerificationClient } from "./json-rpc-verification-client";
4040
import { VerificationService } from "@gitpod/public-api/lib/gitpod/v1/verification_connect";
4141
import { JsonRpcInstallationClient } from "./json-rpc-installation-client";
4242
import { InstallationService } from "@gitpod/public-api/lib/gitpod/v1/installation_connect";
43+
import { v4 } from "uuid";
4344

4445
const transport = createConnectTransport({
4546
baseUrl: `${window.location.protocol}//${window.location.host}/public-api`,
@@ -48,7 +49,7 @@ const transport = createConnectTransport({
4849

4950
export const converter = new PublicAPIConverter();
5051

51-
export const helloService = createPromiseClient(HelloService, transport);
52+
export const helloService = createServiceClient(HelloService);
5253
export const personalAccessTokensService = createPromiseClient(TokensService, transport);
5354
/**
5455
* @deprecated use configurationClient instead
@@ -193,19 +194,32 @@ function createServiceClient<T extends ServiceType>(
193194
}
194195
return jsonRpcOptions.client;
195196
}
196-
/**
197-
* The original application error is retained using gRPC metadata to ensure that existing error handling remains intact.
198-
*/
199-
function handleError(e: any): unknown {
200-
if (e instanceof ConnectError) {
201-
throw converter.fromError(e);
202-
}
203-
throw e;
204-
}
205197
return (...args: any[]) => {
198+
const callOptions: CallOptions = { ...args[1] };
199+
const headers = new Headers(callOptions.headers);
200+
// we cannot rely on only response headers as they are not available for all network conditions
201+
const requestId = headers.get("x-request-id") || v4();
202+
headers.set("x-request-id", requestId);
203+
callOptions.headers = headers;
204+
args = [args[0], callOptions];
205+
206+
function handleError(e: any): unknown {
207+
if (e instanceof ConnectError) {
208+
e = converter.fromError(e);
209+
}
210+
211+
Object.assign(e, {
212+
requestContext: {
213+
requestId,
214+
requestMethod: `${type.typeName}/${prop as string}`,
215+
},
216+
});
217+
throw e;
218+
}
219+
206220
const method = type.methods[prop as string];
207221
if (!method) {
208-
throw new ConnectError("unimplemented", Code.Unimplemented);
222+
handleError(new ConnectError("unimplemented", Code.Unimplemented));
209223
}
210224

211225
// TODO(ak) default timeouts

‎components/dashboard/src/service/service.tsx

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ function testPublicAPI(service: any): void {
103103
try {
104104
return await target[propKey](...args);
105105
} finally {
106-
const grpcType = "unary";
107106
// emulates frequent unary calls to public API
108107
const isTest = await getExperimentsClient().getValueAsync(
109108
"public_api_dummy_reliability_test",
@@ -114,13 +113,7 @@ function testPublicAPI(service: any): void {
114113
},
115114
);
116115
if (isTest) {
117-
helloService.sayHello({}).catch((e) => {
118-
console.error(e, {
119-
userId: user?.id,
120-
workspaceId: args[0],
121-
grpcType,
122-
});
123-
});
116+
helloService.sayHello({}).catch((e) => console.error(e));
124117
}
125118
}
126119
}
@@ -129,7 +122,6 @@ function testPublicAPI(service: any): void {
129122
},
130123
});
131124
(async () => {
132-
const grpcType = "server-stream";
133125
const MAX_BACKOFF = 60000;
134126
const BASE_BACKOFF = 3000;
135127
let backoff = BASE_BACKOFF;
@@ -162,10 +154,7 @@ function testPublicAPI(service: any): void {
162154
backoff = BASE_BACKOFF;
163155
} else {
164156
backoff = Math.min(2 * backoff, MAX_BACKOFF);
165-
console.error(e, {
166-
userId: user?.id,
167-
grpcType,
168-
});
157+
console.error(e);
169158
}
170159
}
171160
} else {

‎components/public-api/typescript-common/README.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
11
# Public API TypeScript Common
22

3+
## Overview
4+
5+
This package serves as a bridge for code conversion between two distinct Gitpod packages: @gitpod/gitpod-protocol and @gitpod/public-api. Its primary responsibility is to ensure seamless translation of application data structures from the gitpod-protocol format to the public-api gRPC format, and vice versa.
6+
7+
## Allowed Usage
8+
9+
Use this package exclusively for tasks that require data structure from @gitpod/gitpod-protocol and @gitpod/public-api. It's important not to introduce dependencies on this package from gitpod-protocol or public-api to ensure changes in one package don't trigger rebuilds of unrelated components such as ws-manager-bridge and supervisor-frontend.
10+
311
## Golden tests
412

513
Golden tests are used to test the output of a function against a known good output. The output is stored in a file with the same name as the test file but with a `.golden` extension. The test will fail if the output does not match the golden file.

‎components/public-api/typescript-common/src/public-api-converter.spec.ts

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,16 @@ describe("PublicAPIConverter", () => {
367367
expect(appError.message).to.equal("too many running workspaces");
368368
});
369369

370+
it("BAD_REQUEST", () => {
371+
const connectError = converter.toError(new ApplicationError(ErrorCodes.BAD_REQUEST, "bad request"));
372+
expect(connectError.code).to.equal(Code.InvalidArgument);
373+
expect(connectError.rawMessage).to.equal("bad request");
374+
375+
const appError = converter.fromError(connectError);
376+
expect(appError.code).to.equal(ErrorCodes.BAD_REQUEST);
377+
expect(appError.message).to.equal("bad request");
378+
});
379+
370380
it("NOT_FOUND", () => {
371381
const connectError = converter.toError(new ApplicationError(ErrorCodes.NOT_FOUND, "not found"));
372382
expect(connectError.code).to.equal(Code.NotFound);
@@ -481,14 +491,15 @@ describe("PublicAPIConverter", () => {
481491
});
482492

483493
it("Any other error is internal", () => {
484-
const error = new Error("unknown");
485-
const connectError = converter.toError(error);
494+
const reason = new Error("unknown");
495+
const connectError = converter.toError(reason);
486496
expect(connectError.code).to.equal(Code.Internal);
487-
expect(connectError.rawMessage).to.equal("unknown");
497+
expect(connectError.rawMessage).to.equal("Oops! Something went wrong.");
498+
expect(connectError.cause).to.equal(reason);
488499

489500
const appError = converter.fromError(connectError);
490501
expect(appError.code).to.equal(ErrorCodes.INTERNAL_SERVER_ERROR);
491-
expect(appError.message).to.equal("unknown");
502+
expect(appError.message).to.equal("Oops! Something went wrong.");
492503
});
493504
});
494505
});

‎components/public-api/typescript-common/src/public-api-converter.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,9 @@ export class PublicAPIConverter {
360360
reason,
361361
);
362362
}
363+
if (reason.code === ErrorCodes.BAD_REQUEST) {
364+
return new ConnectError(reason.message, Code.InvalidArgument, undefined, undefined, reason);
365+
}
363366
if (reason.code === ErrorCodes.NOT_FOUND) {
364367
return new ConnectError(reason.message, Code.NotFound, undefined, undefined, reason);
365368
}
@@ -386,10 +389,13 @@ export class PublicAPIConverter {
386389
}
387390
return new ConnectError(reason.message, Code.Unknown, undefined, undefined, reason);
388391
}
389-
return ConnectError.from(reason, Code.Internal);
392+
return new ConnectError(`Oops! Something went wrong.`, Code.Internal, undefined, undefined, reason);
390393
}
391394

392395
fromError(reason: ConnectError): ApplicationError {
396+
if (reason.code === Code.InvalidArgument) {
397+
return new ApplicationError(ErrorCodes.BAD_REQUEST, reason.rawMessage);
398+
}
393399
if (reason.code === Code.NotFound) {
394400
return new ApplicationError(ErrorCodes.NOT_FOUND, reason.rawMessage);
395401
}

‎components/server/src/api/server.ts

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,10 @@ export class API {
159159
get(target, prop) {
160160
return (...args: any[]) => {
161161
const connectContext = args[1] as HandlerContext;
162+
const requestId = connectContext.requestHeader.get("x-request-id") || v4();
163+
connectContext.responseHeader.set("x-request-id", requestId);
162164
const requestContext: RequestContextSeed = {
163-
requestId: v4(),
165+
requestId,
164166
requestKind: "public-api",
165167
requestMethod: `${grpc_service}/${prop as string}`,
166168
startTime: performance.now(),
@@ -192,6 +194,9 @@ export class API {
192194
grpc_type = "bidi_stream";
193195
}
194196

197+
const isException = (err: ConnectError) =>
198+
err.code === Code.Internal || err.code === Code.Unknown || err.code === Code.DataLoss;
199+
195200
grpcServerStarted.labels(grpc_service, grpc_method, grpc_type).inc();
196201
const stopTimer = grpcServerHandling.startTimer({ grpc_service, grpc_method, grpc_type });
197202
const done = (err?: ConnectError) => {
@@ -201,15 +206,9 @@ export class API {
201206
log.debug("public api: done", { grpc_code });
202207
};
203208
const handleError = (reason: unknown) => {
204-
let err = self.apiConverter.toError(reason);
205-
if (reason != err && err.code === Code.Internal) {
206-
log.error("public api: unexpected internal error", reason);
207-
err = new ConnectError(
208-
`Oops! Something went wrong.`,
209-
Code.Internal,
210-
// pass metadata to preserve the application error
211-
err.metadata,
212-
);
209+
const err = self.apiConverter.toError(reason);
210+
if (isException(err)) {
211+
log.error("public api exception reason:", reason);
213212
}
214213
done(err);
215214
throw err;

0 commit comments

Comments
 (0)