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

Avoid eager read of the stack when captured by a span #2798

Merged
merged 1 commit into from
May 21, 2024
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
5 changes: 5 additions & 0 deletions .changeset/gorgeous-ties-tan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"effect": patch
---

Avoid eager read of the stack when captured by a span
3 changes: 2 additions & 1 deletion packages/effect/src/Tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type * as Context from "./Context.js"
import type * as Effect from "./Effect.js"
import type * as Exit from "./Exit.js"
import type * as Fiber from "./Fiber.js"
import type { LazyArg } from "./Function.js"
import * as defaultServices from "./internal/defaultServices.js"
import * as internal from "./internal/tracer.js"
import type * as Option from "./Option.js"
Expand Down Expand Up @@ -92,7 +93,7 @@ export interface SpanOptions {
readonly root?: boolean | undefined
readonly context?: Context.Context<never> | undefined
readonly kind?: SpanKind | undefined
readonly captureStackTrace?: boolean | string | undefined
readonly captureStackTrace?: boolean | LazyArg<string | undefined> | undefined
}

/**
Expand Down
9 changes: 7 additions & 2 deletions packages/effect/src/internal/cause.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as Either from "../Either.js"
import * as Equal from "../Equal.js"
import type * as FiberId from "../FiberId.js"
import { constFalse, constTrue, dual, identity, pipe } from "../Function.js"
import { globalValue } from "../GlobalValue.js"
import * as Hash from "../Hash.js"
import * as HashSet from "../HashSet.js"
import { NodeInspectSymbol, toJSON } from "../Inspectable.js"
Expand Down Expand Up @@ -1052,6 +1053,9 @@ export const prettyErrorMessage = (u: unknown): string => {

const locationRegex = /\((.*)\)/

/** @internal */
export const spanToTrace = globalValue("effect/Tracer/spanToTrace", () => new WeakMap())

const prettyErrorStack = (message: string, stack: string, span?: Span | undefined): string => {
const out: Array<string> = [message]
const lines = stack.split("\n")
Expand All @@ -1072,8 +1076,9 @@ const prettyErrorStack = (message: string, stack: string, span?: Span | undefine
let current: Span | AnySpan | undefined = span
let i = 0
while (current && current._tag === "Span" && i < 10) {
const stack = current.attributes.get("code.stacktrace")
if (typeof stack === "string") {
const stackFn = spanToTrace.get(current)
if (typeof stackFn === "function") {
const stack = stackFn()
const locationMatch = stack.match(locationRegex)
const location = locationMatch ? locationMatch[1] : stack.replace(/^at /, "")
out.push(` at ${current.name} (${location})`)
Expand Down
22 changes: 16 additions & 6 deletions packages/effect/src/internal/core-effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2054,8 +2054,8 @@ export const unsafeMakeSpan = <XA, XE>(
options.kind ?? "internal"
)

if (typeof options.captureStackTrace === "string") {
span.attribute("code.stacktrace", options.captureStackTrace)
if (typeof options.captureStackTrace === "function") {
internalCause.spanToTrace.set(span, options.captureStackTrace)
}

if (annotationsFromEnv._tag === "Some") {
Expand Down Expand Up @@ -2112,6 +2112,9 @@ export const useSpan: {
if (span.status._tag === "Ended") {
return
}
if (core.exitIsFailure(exit) && internalCause.spanToTrace.has(span)) {
span.attribute("code.stacktrace", internalCause.spanToTrace.get(span)!())
}
span.end(timingEnabled ? clock.unsafeCurrentTimeNanos() : bigint0, exit)
}))
})
Expand Down Expand Up @@ -2155,15 +2158,22 @@ export const functionWithSpan = <Args extends Array<any>, Ret extends Effect.Eff
}
): (...args: Args) => Unify<Ret> =>
(function(this: any) {
let captureStackTrace: string | boolean = options.captureStackTrace ?? false
let captureStackTrace: LazyArg<string | undefined> | boolean = options.captureStackTrace ?? false
if (options.captureStackTrace !== false) {
const limit = Error.stackTraceLimit
Error.stackTraceLimit = 2
const error = new Error()
Error.stackTraceLimit = limit
if (error.stack !== undefined) {
const stack = error.stack.trim().split("\n")
captureStackTrace = stack.slice(2).join("\n").trim()
let cache: false | string = false
captureStackTrace = () => {
if (cache !== false) {
return cache
}
if (error.stack) {
const stack = error.stack.trim().split("\n")
cache = stack.slice(2).join("\n").trim()
return cache
}
}
}
// eslint-disable-next-line @typescript-eslint/no-this-alias
Expand Down
22 changes: 15 additions & 7 deletions packages/effect/src/internal/tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,20 @@ export const addSpanStackTrace = (options: Tracer.SpanOptions | undefined): Trac
Error.stackTraceLimit = 3
const traceError = new Error()
Error.stackTraceLimit = limit
if (traceError.stack === undefined) {
return { ...options, captureStackTrace: false }
}
const stack = traceError.stack.split("\n")
if (!stack[3]) {
return { ...options, captureStackTrace: false }
let cache: false | string = false
return {
...options,
captureStackTrace: () => {
if (cache !== false) {
return cache
}
if (traceError.stack !== undefined) {
const stack = traceError.stack.split("\n")
if (stack[3] !== undefined) {
cache = stack[3].trim()
return cache
}
}
}
}
return { ...options, captureStackTrace: stack[3].trim() }
}
33 changes: 26 additions & 7 deletions packages/effect/test/Tracer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe("Tracer", () => {
const span = yield* Effect.withSpan("A")(Effect.currentSpan)
assert.deepEqual(span.name, "A")
assert.deepEqual(span.parent, Option.none())
assert.include(span.attributes.get("code.stacktrace"), "Tracer.test.ts:18")
assert.strictEqual(span.attributes.get("code.stacktrace"), undefined)
}))

it.effect("parent", () =>
Expand Down Expand Up @@ -139,7 +139,7 @@ describe("Tracer", () => {
Effect.gen(function*() {
const span = yield* Effect.scoped(Effect.makeSpanScoped("A"))
assert.deepEqual(span.status._tag, "Ended")
assert.include(span.attributes.get("code.stacktrace"), "Tracer.test.ts:140")
assert.strictEqual(span.attributes.get("code.stacktrace"), undefined)
}))

it.effect("annotateCurrentSpan", () =>
Expand Down Expand Up @@ -176,8 +176,8 @@ describe("Tracer", () => {
const span = yield* Effect.makeSpan("child")
const parent = yield* Option.filter(span.parent, (span): span is Span => span._tag === "Span")
assert.deepEqual(parent.name, "parent")
assert.include(span.attributes.get("code.stacktrace"), "Tracer.test.ts:176")
assert.include(parent.attributes.get("code.stacktrace"), "Tracer.test.ts:184")
assert.strictEqual(span.attributes.get("code.stacktrace"), undefined)
assert.strictEqual(parent.attributes.get("code.stacktrace"), undefined)
}).pipe(
Effect.provide(Layer.unwrapScoped(
Effect.map(
Expand All @@ -195,7 +195,7 @@ describe("Tracer", () => {
Option.getOrThrow
)
assert.strictEqual(parent.name, "parent")
assert.include(parent.attributes.get("code.stacktrace"), "Tracer.test.ts:200")
assert.strictEqual(parent.attributes.get("code.stacktrace"), undefined)
}).pipe(
Effect.provide(Layer.span("parent"))
))
Expand Down Expand Up @@ -238,7 +238,7 @@ describe("Tracer", () => {
const layer = Layer.effectDiscard(Effect.gen(function*() {
const span = yield* Effect.currentSpan
assert.strictEqual(span.name, "span")
assert.include(span.attributes.get("code.stacktrace"), "Tracer.test.ts:243")
assert.strictEqual(span.attributes.get("code.stacktrace"), undefined)
})).pipe(
Layer.withSpan("span", {
onEnd: (span, _exit) =>
Expand Down Expand Up @@ -274,6 +274,25 @@ it.effect("withTracerEnabled", () =>
assert.deepEqual(spanB.name, "B")
}))

it.effect("includes trace when errored", () =>
Effect.gen(function*() {
let maybeSpan: undefined | Span
const getSpan = Effect.functionWithSpan({
body: (_id: string) =>
Effect.currentSpan.pipe(Effect.flatMap((span) => {
maybeSpan = span
return Effect.fail("error")
})),
options: (id) => ({
name: `span-${id}`,
attributes: { id }
})
})
yield* Effect.flip(getSpan("fail"))
assert.isDefined(maybeSpan)
assert.include(maybeSpan!.attributes.get("code.stacktrace"), "Tracer.test.ts:291:24")
}))

describe("functionWithSpan", () => {
const getSpan = Effect.functionWithSpan({
body: (_id: string) => Effect.currentSpan,
Expand All @@ -288,7 +307,7 @@ describe("functionWithSpan", () => {
const span = yield* getSpan("A")
assert.deepEqual(span.name, "span-A")
assert.deepEqual(span.parent, Option.none())
assert.include(span.attributes.get("code.stacktrace"), "Tracer.test.ts:288")
assert.strictEqual(span.attributes.get("code.stacktrace"), undefined)
}))

it.effect("parent", () =>
Expand Down