From 25545406fbe7c1b4f47b9d6ef7c091f4ecff86fe Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Mon, 14 Feb 2022 09:08:11 +0000 Subject: [PATCH 1/4] [server] Trace userAgent --- .../websocket/websocket-connection-manager.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/components/server/src/websocket/websocket-connection-manager.ts b/components/server/src/websocket/websocket-connection-manager.ts index 183e5dab83f1bd..1bfebb93a6e777 100644 --- a/components/server/src/websocket/websocket-connection-manager.ts +++ b/components/server/src/websocket/websocket-connection-manager.ts @@ -63,18 +63,19 @@ export type WebsocketAuthenticationLevel = "user" | "session" | "anonymous"; export interface ClientMetadata { id: string, authLevel: WebsocketAuthenticationLevel, - sessionId?: string; - userId?: string; - type?: WebsocketClientType; + sessionId?: string, + userId?: string, + type?: WebsocketClientType, origin: ClientOrigin, - version?: string; + version?: string, + userAgent?: string, } interface ClientOrigin { workspaceId?: string, instanceId?: string, } export namespace ClientMetadata { - export function from(userId: string | undefined, sessionId?: string, type?: WebsocketClientType, origin?: ClientOrigin, version?: string): ClientMetadata { + export function from(userId: string | undefined, sessionId?: string, data?: Omit): ClientMetadata { let id = "anonymous"; let authLevel: WebsocketAuthenticationLevel = "anonymous"; if (userId) { @@ -84,7 +85,7 @@ export namespace ClientMetadata { id = `session-${sessionId}`; authLevel = "session"; } - return { id, authLevel, userId, sessionId, type, origin: { ...(origin || {}) }, version }; + return { id, authLevel, userId, sessionId, ...data, origin: data?.origin || {}, }; } export function fromRequest(req: any) { @@ -93,13 +94,14 @@ export namespace ClientMetadata { const sessionId = expressReq.session?.id; const type = WebsocketClientType.getClientType(expressReq); const version = takeFirst(expressReq.headers["x-client-version"]); + const userAgent = takeFirst(expressReq.headers["user-agent"]); const instanceId = takeFirst(expressReq.headers["x-workspace-instance-id"]); const workspaceId = getOriginWorkspaceId(expressReq); const origin: ClientOrigin = { instanceId, workspaceId, }; - return ClientMetadata.from(user?.id, sessionId, type, origin, version); + return ClientMetadata.from(user?.id, sessionId, { type, origin, version, userAgent }); } function getOriginWorkspaceId(req: express.Request): string | undefined { @@ -406,6 +408,7 @@ function traceClientMetadata(ctx: TraceContext, clientMetadata: ClientMetadata) type: clientMetadata.type, version: clientMetadata.version, origin: clientMetadata.origin, + userAgent: clientMetadata.userAgent, }, }); } \ No newline at end of file From fe4c3ef006e7fc892d6b6123100ca0cb463167fc Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Mon, 14 Feb 2022 09:43:32 +0000 Subject: [PATCH 2/4] [server] tracing: Avoid using FOLLOWS_FROM references due to lots of errors --- .../gitpod-protocol/src/util/tracing.ts | 23 ++++++++++--------- .../websocket/websocket-connection-manager.ts | 4 ++-- .../src/workspace/gitpod-server-impl.ts | 2 +- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/components/gitpod-protocol/src/util/tracing.ts b/components/gitpod-protocol/src/util/tracing.ts index c5183b698574a3..5c751dbf61485b 100644 --- a/components/gitpod-protocol/src/util/tracing.ts +++ b/components/gitpod-protocol/src/util/tracing.ts @@ -8,7 +8,7 @@ import * as opentracing from 'opentracing'; import { TracingConfig, initTracerFromEnv } from 'jaeger-client'; import { Sampler, SamplingDecision } from './jaeger-client-types'; -import { followsFrom, initGlobalTracer } from 'opentracing'; +import { initGlobalTracer } from 'opentracing'; import { injectable } from 'inversify'; import { ResponseError } from 'vscode-jsonrpc'; import { log, LogContext } from './logging'; @@ -22,18 +22,19 @@ export type TraceContextWithSpan = TraceContext & { export namespace TraceContext { - export function startSpan(operation: string, parentCtx?: TraceContext, ...referencedSpans: (opentracing.Span | undefined)[]): opentracing.Span { + export function startSpan(operation: string, parentCtx?: TraceContext): opentracing.Span { const options: opentracing.SpanOptions = {}; if (parentCtx && parentCtx.span && !!parentCtx.span.context().toSpanId()) { options.childOf = parentCtx.span; } - if (referencedSpans) { - // note: allthough followsForm's type says it takes 'opentracing.Span | opentracing.SpanContext', it only works with SpanContext (typing mismatch) - // note2: we need to filter out debug spans (spanId === "") - options.references = referencedSpans.filter(s => s !== undefined) - .filter(s => !!s!.context().toSpanId()) - .map(s => followsFrom(s!.context())); - } + // TODO(gpl) references lead to a huge amount of errors in prod logs. Avoid those until we have time to figure out how to fix it. + // if (referencedSpans) { + // // note: allthough followsForm's type says it takes 'opentracing.Span | opentracing.SpanContext', it only works with SpanContext (typing mismatch) + // // note2: we need to filter out debug spans (spanId === "") + // options.references = referencedSpans.filter(s => s !== undefined) + // .filter(s => !!s!.context().toSpanId()) + // .map(s => followsFrom(s!.context())); + // } return opentracing.globalTracer().startSpan(operation, options); } @@ -43,14 +44,14 @@ export namespace TraceContext { return { span }; } - export function withSpan(operation: string, callback: () => void, ctx?: TraceContext, ...referencedSpans: (opentracing.Span | undefined)[]): void { + export function withSpan(operation: string, callback: () => void, ctx?: TraceContext): void { // if we don't have a parent span, don't create a trace here as those are not useful. if (!ctx || !ctx.span || !ctx.span.context()) { callback(); return; } - const span = TraceContext.startSpan(operation, ctx, ...referencedSpans); + const span = TraceContext.startSpan(operation, ctx); try { callback(); } catch (e) { diff --git a/components/server/src/websocket/websocket-connection-manager.ts b/components/server/src/websocket/websocket-connection-manager.ts index 1bfebb93a6e777..3baee6ef17a3a5 100644 --- a/components/server/src/websocket/websocket-connection-manager.ts +++ b/components/server/src/websocket/websocket-connection-manager.ts @@ -337,7 +337,7 @@ class GitpodJsonRpcProxyFactory extends JsonRpcProxyFactory increaseApiCallUserCounter(method, "anonymous"); } - const span = TraceContext.startSpan(method, undefined, this.connectionCtx.span); + const span = TraceContext.startSpan(method, undefined); const ctx = { span }; const userId = this.clientMetadata.userId; try { @@ -400,7 +400,7 @@ class GitpodJsonRpcProxyFactory extends JsonRpcProxyFactory } -function traceClientMetadata(ctx: TraceContext, clientMetadata: ClientMetadata) { +export function traceClientMetadata(ctx: TraceContext, clientMetadata: ClientMetadata) { TraceContext.addNestedTags(ctx, { client: { id: clientMetadata.id, diff --git a/components/server/src/workspace/gitpod-server-impl.ts b/components/server/src/workspace/gitpod-server-impl.ts index 036f6c69f4faef..c5587f3a862ec0 100644 --- a/components/server/src/workspace/gitpod-server-impl.ts +++ b/components/server/src/workspace/gitpod-server-impl.ts @@ -160,7 +160,7 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { // to clients who might not otherwise have access to that information. this.disposables.push(this.localMessageBroker.listenForWorkspaceInstanceUpdates( this.user.id, - (ctx, instance) => TraceContext.withSpan("forwardInstanceUpdateToClient", () => this.client?.onInstanceUpdate(this.censorInstance(instance)), ctx, this.connectionCtx?.span) + (ctx, instance) => TraceContext.withSpan("forwardInstanceUpdateToClient", () => this.client?.onInstanceUpdate(this.censorInstance(instance)), ctx) )); } From fa1539762c70fa1ea3e2955e6c6eb84f1f4b9ac3 Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Mon, 14 Feb 2022 10:12:13 +0000 Subject: [PATCH 3/4] [server] Instead of FOLLOWS_FROM, enhance notifications with clientMetadata directly --- .../gitpod-protocol/src/util/tracing.ts | 6 ++-- .../ee/src/workspace/gitpod-server-impl.ts | 33 ++++++++++++------- .../src/workspace/gitpod-server-impl.ts | 9 +++-- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/components/gitpod-protocol/src/util/tracing.ts b/components/gitpod-protocol/src/util/tracing.ts index 5c751dbf61485b..b204904cba7b20 100644 --- a/components/gitpod-protocol/src/util/tracing.ts +++ b/components/gitpod-protocol/src/util/tracing.ts @@ -44,16 +44,16 @@ export namespace TraceContext { return { span }; } - export function withSpan(operation: string, callback: () => void, ctx?: TraceContext): void { + export function withSpan(operation: string, callback: (ctx: TraceContext) => void, ctx?: TraceContext): void { // if we don't have a parent span, don't create a trace here as those are not useful. if (!ctx || !ctx.span || !ctx.span.context()) { - callback(); + callback({}); return; } const span = TraceContext.startSpan(operation, ctx); try { - callback(); + callback({span}); } catch (e) { TraceContext.setError({span}, e); throw e; diff --git a/components/server/ee/src/workspace/gitpod-server-impl.ts b/components/server/ee/src/workspace/gitpod-server-impl.ts index 22ca8f293bb5b4..c016da403f6060 100644 --- a/components/server/ee/src/workspace/gitpod-server-impl.ts +++ b/components/server/ee/src/workspace/gitpod-server-impl.ts @@ -39,7 +39,7 @@ import { GitHubAppSupport } from "../github/github-app-support"; import { GitLabAppSupport } from "../gitlab/gitlab-app-support"; import { Config } from "../../../src/config"; import { SnapshotService, WaitForSnapshotOptions } from "./snapshot-service"; -import { ClientMetadata } from "../../../src/websocket/websocket-connection-manager"; +import { ClientMetadata, traceClientMetadata } from "../../../src/websocket/websocket-connection-manager"; import { BitbucketAppSupport } from "../bitbucket/bitbucket-app-support"; import { URL } from 'url'; @@ -89,9 +89,12 @@ export class GitpodServerEEImpl extends GitpodServerImpl { for (const projectId of projects) { this.disposables.push(this.localMessageBroker.listenForPrebuildUpdates( projectId, - (ctx: TraceContext, update: PrebuildWithStatus) => { + (ctx: TraceContext, update: PrebuildWithStatus) => TraceContext.withSpan("forwardPrebuildUpdateToClient", (ctx) => { + traceClientMetadata(ctx, this.clientMetadata); + TraceContext.setJsonRPCMetadata(ctx, "onPrebuildUpdate"); + this.client?.onPrebuildUpdate(update); - } + }, ctx) )); } @@ -122,12 +125,17 @@ export class GitpodServerEEImpl extends GitpodServerImpl { } this.disposables.push(this.localMessageBroker.listenToCreditAlerts( this.user.id, - async (ctx: TraceContext, creditAlert: CreditAlert) => { - this.client?.onCreditAlert(creditAlert); - if (creditAlert.remainingUsageHours < 1e-6) { - const runningInstances = await this.workspaceDb.trace(ctx).findRegularRunningInstances(creditAlert.userId); - runningInstances.forEach(async instance => await this.stopWorkspace(ctx, instance.workspaceId)); - } + (ctx: TraceContext, creditAlert: CreditAlert) => { + TraceContext.withSpan("forwardCreditAlertToClient", async (ctx) => { + traceClientMetadata(ctx, this.clientMetadata); + TraceContext.setJsonRPCMetadata(ctx, "onCreditAlert"); + + this.client?.onCreditAlert(creditAlert); + if (creditAlert.remainingUsageHours < 1e-6) { + const runningInstances = await this.workspaceDb.trace(ctx).findRegularRunningInstances(creditAlert.userId); + runningInstances.forEach(async instance => await this.stopWorkspace(ctx, instance.workspaceId)); + } + }, ctx); } )); } @@ -1559,9 +1567,12 @@ export class GitpodServerEEImpl extends GitpodServerImpl { // update client registration for the logged in user this.disposables.push(this.localMessageBroker.listenForPrebuildUpdates( project.id, - (ctx: TraceContext, update: PrebuildWithStatus) => { + (ctx: TraceContext, update: PrebuildWithStatus) => TraceContext.withSpan("forwardPrebuildUpdateToClient", (ctx) => { + traceClientMetadata(ctx, this.clientMetadata); + TraceContext.setJsonRPCMetadata(ctx, "onPrebuildUpdate"); + this.client?.onPrebuildUpdate(update); - } + }, ctx) )); return project; } diff --git a/components/server/src/workspace/gitpod-server-impl.ts b/components/server/src/workspace/gitpod-server-impl.ts index c5587f3a862ec0..ff63302cd387de 100644 --- a/components/server/src/workspace/gitpod-server-impl.ts +++ b/components/server/src/workspace/gitpod-server-impl.ts @@ -54,7 +54,7 @@ import { CachingBlobServiceClientProvider } from '@gitpod/content-service/lib/su import { IDEOptions } from '@gitpod/gitpod-protocol/lib/ide-protocol'; import { IDEConfigService } from '../ide-config'; import { PartialProject } from '@gitpod/gitpod-protocol/src/teams-projects-protocol'; -import { ClientMetadata } from '../websocket/websocket-connection-manager'; +import { ClientMetadata, traceClientMetadata } from '../websocket/websocket-connection-manager'; import { ConfigurationService } from '../config/configuration-service'; import { ProjectEnvVar } from '@gitpod/gitpod-protocol/src/protocol'; import { InstallationAdminSettings } from '@gitpod/gitpod-protocol'; @@ -160,7 +160,12 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { // to clients who might not otherwise have access to that information. this.disposables.push(this.localMessageBroker.listenForWorkspaceInstanceUpdates( this.user.id, - (ctx, instance) => TraceContext.withSpan("forwardInstanceUpdateToClient", () => this.client?.onInstanceUpdate(this.censorInstance(instance)), ctx) + (ctx, instance) => TraceContext.withSpan("forwardInstanceUpdateToClient", (ctx) => { + traceClientMetadata(ctx, this.clientMetadata); + TraceContext.setJsonRPCMetadata(ctx, "onInstanceUpdate"); + + this.client?.onInstanceUpdate(this.censorInstance(instance)); + }, ctx) )); } From f3db54d7d314e118b8746dfd82e74f8011d59f30 Mon Sep 17 00:00:00 2001 From: Gero Posmyk-Leinemann Date: Mon, 14 Feb 2022 11:28:45 +0000 Subject: [PATCH 4/4] [installer, helm] Ensure jaeger is disabled by default --- chart/templates/_helpers.tpl | 3 +++ installer/pkg/common/common.go | 1 + 2 files changed, 4 insertions(+) diff --git a/chart/templates/_helpers.tpl b/chart/templates/_helpers.tpl index 7e8f42c1586e9d..52ae736e7fdfe7 100644 --- a/chart/templates/_helpers.tpl +++ b/chart/templates/_helpers.tpl @@ -272,6 +272,9 @@ env: value: {{ $tracing.samplerType }} - name: JAEGER_SAMPLER_PARAM value: "{{ $tracing.samplerParam }}" +{{- else }} +- name: JAEGER_DISABLED + value: "true" {{- end }} {{- end -}} diff --git a/installer/pkg/common/common.go b/installer/pkg/common/common.go index b45bd6416834b8..d5288362833ddd 100644 --- a/installer/pkg/common/common.go +++ b/installer/pkg/common/common.go @@ -68,6 +68,7 @@ func DefaultEnv(cfg *config.Config) []corev1.EnvVar { func TracingEnv(context *RenderContext) (res []corev1.EnvVar) { if context.Config.Observability.Tracing == nil { + res = append(res, corev1.EnvVar{Name: "JAEGER_DISABLED", Value: "true"}) return }