Skip to content

Commit

Permalink
Add createRootSpan function (#656)
Browse files Browse the repository at this point in the history
By using the createRootSpan function in the HTTP incoming
instrumentation, we ensure ensure that every request always create a new
RootSpan. Between the requests in express, the context could be the same
(having the same executionAsyncId) as the previous request.

The problem that could occur (especially in our new OpenTelemetry
instrumentation), is that spans from new requests could continue to be
tracked on the RootSpan of the previous request. Which could cause us to
missout on these requests. Or, in the case of the OpenTelemetry
instrumentation, create a new RootSpan for the child span, because the
`spanId` and `traceId` would be empty, because the RootSpan was already
closed.

We removed the if-statement in the scope manager. When creating a new
RootSpan for a context that already has a RootSpan, with the
if-statement intact, the `rootSpan` function would return the new
RootSpan, but `currentSpan` function would return the old RootSpan. We
need to update the scopes in the ScopeManager to also set the new
RootSpan on the "scopes" for the context, even if it's already set
before.
  • Loading branch information
tombruijn authored May 10, 2022
1 parent 02755d2 commit 33f7864
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 19 deletions.
6 changes: 6 additions & 0 deletions packages/nodejs/.changesets/add-createrootspan-function.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: "patch"
type: "add"
---

Add the `createRootSpan` function to the Tracer to allow explicit creation of RootSpans even if another RootSpan already exists and is tracked as the current RootSpan. Make sure to not forget about the previous RootSpan, and close it as well at some point when using this function.
12 changes: 0 additions & 12 deletions packages/nodejs/src/__tests__/scope.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,6 @@ describe("ScopeManager", () => {
expect(scopeManager.root()).toBe(span)
expect(scopeManager.active()).toBe(span)
})

describe("when there is an active span", () => {
it("does not set the active span for the current process", () => {
const rootSpan = new RootSpan()
const childSpan = new ChildSpan(rootSpan)

scopeManager.withContext(childSpan, () => {
scopeManager.setRoot(rootSpan)
expect(scopeManager.active()).toBe(childSpan)
})
})
})
})

describe(".withContext()", () => {
Expand Down
16 changes: 16 additions & 0 deletions packages/nodejs/src/__tests__/tracer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,22 @@ describe("Tracer", () => {
})
})

describe(".createRootSpan()", () => {
it("creates a new span and assigns it as a root span", () => {
const rootSpan1 = tracer.createRootSpan()
expect(rootSpan1).toBeInstanceOf(RootSpan)
rootSpan1.close()
expect(tracer.rootSpan()).toEqual(rootSpan1)
expect(tracer.currentSpan()).toEqual(rootSpan1)

const rootSpan2 = tracer.createRootSpan()
expect(rootSpan2).toBeInstanceOf(RootSpan)
rootSpan2.close()
expect(tracer.rootSpan()).toEqual(rootSpan2)
expect(tracer.currentSpan()).toEqual(rootSpan2)
})
})

describe(".sendError()", () => {
const err = new Error("FooBarError")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function incomingRequest(
}

const rootSpan = tracer
.createSpan()
.createRootSpan()
/**
* For our processor to work, root `Span`s must have a groupable, non-dynamic
* name to be easily grouped into performance samples.
Expand Down
7 changes: 7 additions & 0 deletions packages/nodejs/src/interfaces/tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ export interface Tracer {
*/
createSpan(options?: Partial<SpanOptions>, context?: SpanContext): Span

/**
* Creates a new `Span` instance that is always the new RootSpan in the current
* async context. If a previous RootSpan existed, it's ignored from this point on.
* Make sure it's closed beforehand or handled by another part of the app.
*/
createRootSpan(options?: Partial<SpanOptions>): Span

/**
* Returns the current Span.
*
Expand Down
4 changes: 4 additions & 0 deletions packages/nodejs/src/noops/tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ export class NoopTracer implements Tracer {
return new NoopSpan()
}

public createRootSpan(_options?: Partial<SpanOptions>): Span {
return new NoopSpan()
}

public currentSpan(): Span {
return new NoopSpan()
}
Expand Down
4 changes: 1 addition & 3 deletions packages/nodejs/src/scope.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,7 @@ export class ScopeManager {
public setRoot(rootSpan: Span) {
const uid = asyncHooks.executionAsyncId()
this.#roots.set(uid, rootSpan)
if (!this.#scopes.has(uid)) {
this.#scopes.set(uid, rootSpan)
}
this.#scopes.set(uid, rootSpan)
}

/*
Expand Down
10 changes: 7 additions & 3 deletions packages/nodejs/src/tracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,18 @@ export class BaseTracer implements Tracer {
if (spanOrContext) {
return new ChildSpan(spanOrContext, options)
} else if (activeRootSpan instanceof NoopSpan) {
const rootSpan = new RootSpan(options)
this.#scopeManager.setRoot(rootSpan)
return rootSpan
return this.createRootSpan(options)
} else {
return new ChildSpan(activeRootSpan, options)
}
}

public createRootSpan(options?: Partial<SpanOptions>): Span {
const rootSpan = new RootSpan(options)
this.#scopeManager.setRoot(rootSpan)
return rootSpan
}

/**
* Returns the current Span.
*
Expand Down

0 comments on commit 33f7864

Please sign in to comment.