-
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] add request context #18853
Changes from all commits
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 |
---|---|---|
|
@@ -24,7 +24,7 @@ export class APIHelloService implements ServiceImpl<typeof HelloService> { | |
} | ||
async *lotsOfReplies(req: LotsOfRepliesRequest, context: HandlerContext): AsyncGenerator<LotsOfRepliesResponse> { | ||
let count = req.previousCount || 0; | ||
while (true) { | ||
while (!context.signal.aborted) { | ||
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. it was a bug and we were leaking server streams here |
||
const response = new LotsOfRepliesResponse(); | ||
response.reply = `Hello ${this.getSubject(context)} ${count}`; | ||
response.count = count; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,9 @@ import { APIStatsService } from "./stats"; | |
import { APITeamsService } from "./teams"; | ||
import { APIUserService } from "./user"; | ||
import { APIWorkspacesService } from "./workspaces"; | ||
import { LogContextOptions, wrapAsyncGenerator, runWithContext } from "../util/log-context"; | ||
import { v4 } from "uuid"; | ||
import { performance } from "perf_hooks"; | ||
|
||
function service<T extends ServiceType>(type: T, impl: ServiceImpl<T>): [T, ServiceImpl<T>] { | ||
return [type, impl]; | ||
|
@@ -90,9 +93,9 @@ export class API { | |
* intercept handles cross-cutting concerns for all calls: | ||
* - authentication | ||
* - server-side observability | ||
* - logging context | ||
* TODO(ak): | ||
* - rate limitting | ||
* - logging context | ||
* - tracing | ||
*/ | ||
private interceptService<T extends ServiceType>(type: T): ProxyHandler<ServiceImpl<T>> { | ||
|
@@ -101,10 +104,22 @@ export class API { | |
return { | ||
get(target, prop) { | ||
return (...args: any[]) => { | ||
const method = type.methods[prop as any]; | ||
const logContext: LogContextOptions & { | ||
requestId?: string; | ||
contextTimeMs: number; | ||
grpc_service: string; | ||
grpc_method: string; | ||
} = { | ||
contextTimeMs: performance.now(), | ||
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. I'm capturing it here because otherwise i cannot handle async generators propely |
||
grpc_service, | ||
grpc_method: prop as string, | ||
}; | ||
const withRequestContext = <T>(fn: () => T): T => runWithContext("public-api", logContext, fn); | ||
|
||
const method = type.methods[prop as string]; | ||
if (!method) { | ||
// Increment metrics for unknown method attempts | ||
console.warn("public api: unknown method", grpc_service, prop); | ||
withRequestContext(() => log.warn("public api: unknown method")); | ||
const code = Code.Unimplemented; | ||
grpcServerStarted.labels(grpc_service, "unknown", "unknown").inc(); | ||
grpcServerHandled.labels(grpc_service, "unknown", "unknown", Code[code]).inc(); | ||
|
@@ -123,55 +138,78 @@ export class API { | |
grpc_type = "bidi_stream"; | ||
} | ||
|
||
logContext.requestId = v4(); | ||
|
||
grpcServerStarted.labels(grpc_service, grpc_method, grpc_type).inc(); | ||
const stopTimer = grpcServerHandling.startTimer({ grpc_service, grpc_method, grpc_type }); | ||
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 }); | ||
let callMs: number | undefined; | ||
if (callStartedAt) { | ||
callMs = performance.now() - callStartedAt; | ||
} | ||
log.debug("public api: done", { grpc_code, verifyMs, callMs }); | ||
// right now p99 for getLoggetInUser is around 100ms, using it as a threshold for now | ||
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. this is going to be removed as soon as we figure out the issue |
||
const slowThreshold = 100; | ||
const totalMs = performance.now() - logContext.contextTimeMs; | ||
if (grpc_type === "unary" && totalMs > slowThreshold) { | ||
log.warn("public api: slow unary call", { grpc_code, callMs, verifyMs }); | ||
} | ||
}; | ||
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); | ||
// don't leak internal errors to a user | ||
// TODO(ak) instead surface request id | ||
err = ConnectError.from(`please check server logs`, Code.Internal); | ||
log.error("public api: unexpected internal error", reason); | ||
err = ConnectError.from( | ||
`Oops! Something went wrong. Please quote the request ID ${logContext.requestId} when reaching out to Gitpod Support.`, | ||
Code.Internal, | ||
); | ||
} | ||
done(err); | ||
throw err; | ||
}; | ||
|
||
let verifyMs: number | undefined; | ||
let callStartedAt: number | undefined; | ||
const context = args[1] as HandlerContext; | ||
async function call<T>(): Promise<T> { | ||
|
||
const apply = async <T>(): Promise<T> => { | ||
const verifyStartedAt = performance.now(); | ||
const user = await self.verify(context); | ||
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. I was wondering rather add a warning and more fine grain logging inside verify generally. But i worry about negative impact form logging itself. It is probably the hottest path. |
||
verifyMs = performance.now() - verifyStartedAt; | ||
context.user = user; | ||
|
||
return (target[prop as any] as Function).apply(target, args); | ||
} | ||
callStartedAt = performance.now(); | ||
return Reflect.apply(target[prop as any], target, args); | ||
}; | ||
if (grpc_type === "unary" || grpc_type === "client_stream") { | ||
return (async () => { | ||
return withRequestContext(async () => { | ||
try { | ||
const promise = await call<Promise<any>>(); | ||
const promise = await apply<Promise<any>>(); | ||
const result = await promise; | ||
done(); | ||
return result; | ||
} catch (e) { | ||
handleError(e); | ||
} | ||
})(); | ||
}); | ||
} | ||
return (async function* () { | ||
try { | ||
const generator = await call<AsyncGenerator<any>>(); | ||
for await (const item of generator) { | ||
yield item; | ||
return wrapAsyncGenerator( | ||
(async function* () { | ||
try { | ||
const generator = await apply<AsyncGenerator<any>>(); | ||
for await (const item of generator) { | ||
yield item; | ||
} | ||
done(); | ||
} catch (e) { | ||
handleError(e); | ||
} | ||
done(); | ||
} catch (e) { | ||
handleError(e); | ||
} | ||
})(); | ||
})(), | ||
withRequestContext, | ||
); | ||
}; | ||
}, | ||
}; | ||
|
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 was a bug introduced with recent changes
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 does not break in gitpod.io but on preview envs trimming prefix breaks host