Skip to content

Commit 2f49ea8

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

File tree

7 files changed

+78
-42
lines changed

7 files changed

+78
-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: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ const transport = createConnectTransport({
4848

4949
export const converter = new PublicAPIConverter();
5050

51-
export const helloService = createPromiseClient(HelloService, transport);
51+
export const helloService = createServiceClient(HelloService);
5252
export const personalAccessTokensService = createPromiseClient(TokensService, transport);
5353
/**
5454
* @deprecated use configurationClient instead
@@ -193,19 +193,35 @@ function createServiceClient<T extends ServiceType>(
193193
}
194194
return jsonRpcOptions.client;
195195
}
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-
}
205196
return (...args: any[]) => {
197+
const requestContext = {
198+
requestMethod: `${type.typeName}/${prop as string}`,
199+
};
200+
const callOptions: CallOptions = { ...args[1] };
201+
const originalOnHeader = callOptions.onHeader;
202+
callOptions.onHeader = (headers) => {
203+
if (originalOnHeader) {
204+
originalOnHeader(headers);
205+
}
206+
const requestId = headers.get("x-request-id") || undefined;
207+
if (requestId) {
208+
Object.assign(requestContext, { requestId });
209+
}
210+
};
211+
args = [args[0], callOptions];
212+
213+
function handleError(e: any): unknown {
214+
if (e instanceof ConnectError) {
215+
e = converter.fromError(e);
216+
}
217+
218+
Object.assign(e, { requestContext });
219+
throw e;
220+
}
221+
206222
const method = type.methods[prop as string];
207223
if (!method) {
208-
throw new ConnectError("unimplemented", Code.Unimplemented);
224+
handleError(new ConnectError("unimplemented", Code.Unimplemented));
209225
}
210226

211227
// 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: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,8 @@ export class API {
147147
* - authentication
148148
* - server-side observability
149149
* - logging context
150-
* TODO(ak):
151150
* - rate limitting
151+
* TODO(ak):
152152
* - tracing
153153
* - cancellation
154154
*/
@@ -167,6 +167,7 @@ export class API {
167167
signal: connectContext.signal,
168168
headers: connectContext.requestHeader,
169169
};
170+
connectContext.responseHeader.set("x-request-id", requestContext.requestId!);
170171

171172
const withRequestContext = <T>(fn: () => T): T => runWithRequestContext(requestContext, fn);
172173

@@ -192,6 +193,9 @@ export class API {
192193
grpc_type = "bidi_stream";
193194
}
194195

196+
const isException = (err: ConnectError) =>
197+
err.code === Code.Internal || err.code === Code.Unknown || err.code === Code.DataLoss;
198+
195199
grpcServerStarted.labels(grpc_service, grpc_method, grpc_type).inc();
196200
const stopTimer = grpcServerHandling.startTimer({ grpc_service, grpc_method, grpc_type });
197201
const done = (err?: ConnectError) => {
@@ -201,15 +205,9 @@ export class API {
201205
log.debug("public api: done", { grpc_code });
202206
};
203207
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-
);
208+
const err = self.apiConverter.toError(reason);
209+
if (isException(err)) {
210+
log.error("public api exception reason:", reason);
213211
}
214212
done(err);
215213
throw err;

0 commit comments

Comments
 (0)