Skip to content

Commit

Permalink
Import OpenTelemetry types rather than duplicate (#659)
Browse files Browse the repository at this point in the history
Use Typescript's `import type` and `export type` to include the
OpenTelemetry types into our package without having to copy the code
into our project directly.

This was suggested by @unflxw in PR
#651 (comment)

I'm not sure if this is more or less fragile. If the OpenTelemetry
package changes the interface we'd be shipping an older version that
doesn't work with older versions anymore.

The package from which we import is a dev dependency only, and used
during the compile step, and won't ship as a dependency in the final
release.

I got an issue when using the OpenTelemetry SpanProcessor type for the
`onEnd` function. It doesn't have a second argument.

The `onStart` also needed another type, from `SpanContext` to `Context`.
  • Loading branch information
tombruijn authored May 16, 2022
1 parent 15a1476 commit ee1ea8b
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 62 deletions.
121 changes: 108 additions & 13 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
bump: "patch"
type: "change"
---

Use the OpenTelemetry SpanProcessor interface to build our own SpanProcessor. We previously copied the SpanProcessor code into our package, but instead we now use the OpenTelemetry interface directly. This should make our processor match the expected type better.
3 changes: 2 additions & 1 deletion packages/nodejs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"@types/semver": "*",
"nock": "^13.2.2",
"pg": "*",
"redis": "*"
"redis": "*",
"@opentelemetry/sdk-trace-base": "^1.2.0"
},
"scripts": {
"build": "tsc -p tsconfig.json",
Expand Down
49 changes: 6 additions & 43 deletions packages/nodejs/src/interfaces/span_processor.ts
Original file line number Diff line number Diff line change
@@ -1,44 +1,7 @@
import { HrTime, Span, SpanAttributes, SpanContext } from "@opentelemetry/api"
import type {
Span,
ReadableSpan,
SpanProcessor
} from "@opentelemetry/sdk-trace-base"

/**
* OpenTelemetrySpanProcessor is based on OpenTelemetry's internal span processor.
*/
export interface OpenTelemetrySpanProcessor {
/**
* Forces to export all finished spans
*/
forceFlush(): Promise<void>

/**
* Called when an OpenTelemetry {@link Span} is started, if the
* `span.isRecording()` returns true.
* @param span the OpenTelemetry Span that just started.
*/
onStart(_span: Span, _parentContext: SpanContext): void

/**
* Called when an OpenTelemetry {@link Span} is ended, if the
* `span.isRecording()` returns true.
* @param span the Span that just ended.
*/
onEnd(_span: ReadableSpan, _parentContext: SpanContext): void

/**
* Shuts down the processor. Called when the OpenTelemetry SDK is shut down.
* This is an opportunity for processor to do any cleanup required.
*/
shutdown(): Promise<void>
}

/**
* ReadableSpan is based on a subset of OpenTelemetry's internal "readable"
* spans, which have readonly attributes for exporting.
*/
export interface ReadableSpan {
readonly name: string
readonly spanContext: () => SpanContext
readonly startTime: HrTime
readonly endTime: HrTime
readonly attributes: SpanAttributes
readonly instrumentationLibrary: { name: string }
}
export type { Span, ReadableSpan, SpanProcessor }
11 changes: 6 additions & 5 deletions packages/nodejs/src/span_processor.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { Span, SpanContext } from "@opentelemetry/api"
import { Context } from "@opentelemetry/api"
import {
OpenTelemetrySpanProcessor,
ReadableSpan
Span,
ReadableSpan,
SpanProcessor as OpenTelemetrySpanProcessor
} from "./interfaces/span_processor"
import { BaseClient as Client } from "./client"
import { NoopSpan } from "./noops"
Expand All @@ -18,9 +19,9 @@ export class SpanProcessor implements OpenTelemetrySpanProcessor {
}

// eslint-disable-next-line @typescript-eslint/no-empty-function
onStart(_span: Span, _parentContext: SpanContext): void {}
onStart(_span: Span, _parentContext: Context): void {}

onEnd(span: ReadableSpan, _parentContext: SpanContext): void {
onEnd(span: ReadableSpan): void {
const appsignalSpan = this.client.tracer().currentSpan()

if (!(appsignalSpan instanceof NoopSpan)) {
Expand Down

0 comments on commit ee1ea8b

Please sign in to comment.