From 32d8054e5613e11a77fa3d98f246c72d3f98efa0 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Thu, 3 Feb 2022 15:32:26 +0100 Subject: [PATCH] Add Span.toObject function, replacing Span.toJSON (#592) The `Span.toJSON` function required us to always parse it manually. Why not do that in the function itself? That's what `Span.toObject` does. Other name suggestions are welcome! This way we don't cheat in tests and try and match on the JSON string with `toMatch(/substring/)`. Now we need to specify which attribute value we want to match. All fields of the SpanData object are optional because if no span data is returned by the extension, all fields are empty. --- .../remove-span-tojson-function.md | 6 ++ packages/nodejs/src/__tests__/scope.test.ts | 5 +- packages/nodejs/src/__tests__/span.test.ts | 76 ++++++++----------- packages/nodejs/src/__tests__/tracer.test.ts | 18 ++--- packages/nodejs/src/interfaces/span.ts | 21 ++++- packages/nodejs/src/noops/span.ts | 6 +- packages/nodejs/src/span.ts | 13 ++-- 7 files changed, 80 insertions(+), 65 deletions(-) create mode 100644 packages/nodejs/.changesets/remove-span-tojson-function.md diff --git a/packages/nodejs/.changesets/remove-span-tojson-function.md b/packages/nodejs/.changesets/remove-span-tojson-function.md new file mode 100644 index 00000000..5cd15459 --- /dev/null +++ b/packages/nodejs/.changesets/remove-span-tojson-function.md @@ -0,0 +1,6 @@ +--- +bump: "patch" +type: "remove" +--- + +Remove the private function `Span.toJSON`. This wasn't previously marked as private, but it was. diff --git a/packages/nodejs/src/__tests__/scope.test.ts b/packages/nodejs/src/__tests__/scope.test.ts index 6003246b..7f7efd4e 100644 --- a/packages/nodejs/src/__tests__/scope.test.ts +++ b/packages/nodejs/src/__tests__/scope.test.ts @@ -98,12 +98,12 @@ describe("ScopeManager", () => { const span = scopeManager.active() expect(span).toBeDefined() - expect(span?.toJSON()).toMatch(/modified/) + expect(span?.toObject().name).toEqual("modified") } scopeManager.withContext(test, span => { span.setName("default") - expect(span.toJSON()).toMatch(/default/) + expect(span.toObject().name).toEqual("default") }) scopeManager.withContext(test, span => { @@ -115,5 +115,6 @@ describe("ScopeManager", () => { }) }) + // TODO: Add tests describe(".emitWithContext()", () => {}) }) diff --git a/packages/nodejs/src/__tests__/span.test.ts b/packages/nodejs/src/__tests__/span.test.ts index cb7bd348..0acc1520 100644 --- a/packages/nodejs/src/__tests__/span.test.ts +++ b/packages/nodejs/src/__tests__/span.test.ts @@ -2,28 +2,13 @@ import { ChildSpan, RootSpan } from "../span" import { span } from "../extension_wrapper" import { BaseClient } from "../client" import { Data } from "../internal/data" - -type SpanData = { - closed: boolean - name?: string - namespace?: string - parent_span_id?: string - span_id?: string - start_time?: number - trace_id?: string - attributes?: { [key: string]: string } -} +import { SpanData } from "../interfaces/span" describe("RootSpan", () => { - let span: RootSpan - let internal: SpanData - - beforeEach(() => { - span = new RootSpan() - internal = JSON.parse(span.toJSON()) - }) - it("creates a RootSpan", () => { + const span = new RootSpan() + const internal = span.toObject() + expect(span).toBeInstanceOf(RootSpan) expect(internal.closed).toBeFalsy() }) @@ -31,14 +16,16 @@ describe("RootSpan", () => { it("creates a RootSpan with a timestamp", () => { const startTime = 1607022684531 - span = new RootSpan({ startTime }) - internal = JSON.parse(span.toJSON()) + const span = new RootSpan({ startTime }) + const internal = span.toObject() expect(internal.start_time).toEqual(1607022685) }) it("exposes a spanId", () => { + const span = new RootSpan() const { spanId } = span + const internal = span.toObject() expect(spanId).toBeDefined() expect(internal.span_id).toBeDefined() @@ -46,6 +33,8 @@ describe("RootSpan", () => { }) it("exposes a traceId", () => { + const span = new RootSpan() + const internal = span.toObject() const { traceId } = span expect(traceId).toBeDefined() @@ -54,6 +43,8 @@ describe("RootSpan", () => { }) it("creates a new ChildSpan", () => { + const span = new RootSpan() + const internal = span.toObject() const child = span.child() expect(child).toBeDefined() @@ -63,8 +54,8 @@ describe("RootSpan", () => { it("belongs to a given namespace", () => { const namespace = "test_namespace" - span = new RootSpan({ namespace }) - internal = JSON.parse(span.toJSON()) + const span = new RootSpan({ namespace }) + const internal = span.toObject() expect(internal.namespace).toEqual(namespace) }) @@ -72,8 +63,8 @@ describe("RootSpan", () => { it("sets the name", () => { const name = "test_span" - span = new RootSpan().setName(name) - internal = JSON.parse(span.toJSON()) + const span = new RootSpan().setName(name) + const internal = span.toObject() expect(internal.name).toEqual(name) }) @@ -82,9 +73,9 @@ describe("RootSpan", () => { const category = "test_category" const span = new RootSpan().setCategory(category) - const internal = JSON.parse(span.toJSON()) + const internal = span.toObject() - expect(internal.attributes["appsignal:category"]).toEqual(category) + expect(internal.attributes!["appsignal:category"]).toEqual(category) }) it("sets attributes", () => { @@ -94,7 +85,7 @@ describe("RootSpan", () => { span.set("boolean_false", false) span.set("int", 123) span.set("float", 123.45) - const internal = JSON.parse(span.toJSON()) + const internal = span.toObject() expect(internal.attributes).toMatchObject({ boolean_false: false, @@ -110,30 +101,30 @@ describe("RootSpan", () => { const sanitizedQuery = "SELECT * FROM users WHERE email = ?" const span = new RootSpan().setSQL(query) - const internal = JSON.parse(span.toJSON()) + const internal = span.toObject() - expect(internal.attributes["appsignal:body"]).toEqual(sanitizedQuery) + expect(internal.attributes!["appsignal:body"]).toEqual(sanitizedQuery) }) it("sets an error with backtrace", () => { const error = new Error("uh oh") const span = new RootSpan().setError(error) - const internal = JSON.parse(span.toJSON()) + const internal = span.toObject() expect(internal.error).toEqual({ name: "Error", message: "uh oh", backtrace: expect.any(String) }) - expect(internal.error.backtrace).toMatch(/^\["Error: uh oh"/) - expect(internal.error.backtrace).toMatch(/span\.test\.ts/) + expect(internal.error!.backtrace).toMatch(/^\["Error: uh oh"/) + expect(internal.error!.backtrace).toMatch(/span\.test\.ts/) }) it("sets an error without backtrace", () => { const error = new Error("uh oh") error.stack = undefined const span = new RootSpan().setError(error) - const internal = JSON.parse(span.toJSON()) + const internal = span.toObject() expect(internal.error).toEqual({ name: "Error", @@ -143,8 +134,8 @@ describe("RootSpan", () => { }) it("closes a span", () => { - span = new RootSpan().close() - internal = JSON.parse(span.toJSON()) + const span = new RootSpan().close() + const internal = span.toObject() expect(internal.closed).toBeTruthy() }) @@ -156,10 +147,10 @@ describe("ChildSpan", () => { beforeEach(() => { span = new ChildSpan({ traceId: "aaaaaaaaaaaaaaaa", spanId: "bbbbbbbb" }) - internal = JSON.parse(span.toJSON()) + internal = span.toObject() }) - it("creates a ChildSpan", () => { + it("creates a ChildSpan without a timestamp", () => { expect(span).toBeInstanceOf(ChildSpan) expect(internal.trace_id).toEqual("aaaaaaaaaaaaaaaa") @@ -174,11 +165,11 @@ describe("ChildSpan", () => { { traceId: "aaaaaaaaaaaaaaaa", spanId: "bbbbbbbb" }, { startTime } ) - - internal = JSON.parse(span.toJSON()) + internal = span.toObject() expect(internal.trace_id).toEqual("aaaaaaaaaaaaaaaa") expect(internal.parent_span_id).toEqual("bbbbbbbb") + // TODO: Fix // expect(internal.start_time).toEqual(1607022685) expect(internal.closed).toBeFalsy() }) @@ -213,7 +204,7 @@ describe("ChildSpan", () => { traceId: "test_trace_id", spanId: "parent_span_id" }).setCategory(category) - internal = JSON.parse(span.toJSON()) + internal = span.toObject() expect(internal.attributes!["appsignal:category"]).toEqual(category) }) @@ -223,8 +214,7 @@ describe("ChildSpan", () => { traceId: "test_trace_id", spanId: "parent_span_id" }).close() - - internal = JSON.parse(span.toJSON()) + internal = span.toObject() expect(internal.closed).toBeTruthy() }) diff --git a/packages/nodejs/src/__tests__/tracer.test.ts b/packages/nodejs/src/__tests__/tracer.test.ts index e4d9ad9c..2cb52842 100644 --- a/packages/nodejs/src/__tests__/tracer.test.ts +++ b/packages/nodejs/src/__tests__/tracer.test.ts @@ -13,14 +13,14 @@ describe("Tracer", () => { describe(".createSpan()", () => { it("assigns the spans properly", () => { const rootSpan = tracer.createSpan().setName("rootSpan") - const rootSpanData = JSON.parse(rootSpan.toJSON()) + const rootSpanData = rootSpan.toObject() expect(rootSpan).toBeInstanceOf(RootSpan) expect(rootSpanData.parent_span_id).toEqual("") expect(rootSpanData.name).toEqual("rootSpan") const childSpan = tracer.createSpan().setName("childSpan") - const childSpanData = JSON.parse(childSpan.toJSON()) + const childSpanData = childSpan.toObject() expect(childSpan).toBeInstanceOf(ChildSpan) expect(childSpanData.parent_span_id).toEqual(rootSpanData.span_id) @@ -29,7 +29,7 @@ describe("Tracer", () => { const spanFromSpan = tracer .createSpan(undefined, childSpan) .setName("spanFromSpan") - const spanFromSpanData = JSON.parse(spanFromSpan.toJSON()) + const spanFromSpanData = spanFromSpan.toObject() expect(spanFromSpan).toBeInstanceOf(ChildSpan) expect(spanFromSpanData.parent_span_id).toEqual(childSpanData.span_id) @@ -58,11 +58,11 @@ describe("Tracer", () => { rootSpan.setCategory("bar") rootSpan.set("pod", 42) - const rootSpanData = JSON.parse(rootSpan.toJSON()) + const rootSpanData = rootSpan.toObject() expect(rootSpanData.name).toEqual("foo") - expect(rootSpanData.attributes["appsignal:category"]).toEqual("bar") - expect(rootSpanData.attributes.pod).toEqual(42) + expect(rootSpanData.attributes!["appsignal:category"]).toEqual("bar") + expect(rootSpanData.attributes!.pod).toEqual(42) return done() }) @@ -70,9 +70,9 @@ describe("Tracer", () => { it("adds the given error to the span", done => { tracer.sendError(err, rootSpan => { - const rootSpanData = JSON.parse(rootSpan.toJSON()) + const rootSpanData = rootSpan.toObject() - expect(rootSpanData.error.message).toEqual("FooBarError") + expect(rootSpanData.error!.message).toEqual("FooBarError") return done() }) @@ -84,7 +84,7 @@ describe("Tracer", () => { const rootSpan = tracer.createSpan().setName(name) await tracer.withSpan(rootSpan, async span => { - const internal = JSON.parse(span.toJSON()) + const internal = span.toObject() expect(internal.name).toEqual(name) span.close() diff --git a/packages/nodejs/src/interfaces/span.ts b/packages/nodejs/src/interfaces/span.ts index 15b16048..c0b494a3 100644 --- a/packages/nodejs/src/interfaces/span.ts +++ b/packages/nodejs/src/interfaces/span.ts @@ -116,7 +116,24 @@ export interface Span { close(endTime?: number): this /** - * Returns a JSON string representing the internal Span in the agent. + * Returns a SpanData object representing the internal Span in the extension. + * + * @private */ - toJSON(): string + toObject(): SpanData +} + +/** + * The internal data structure of a `Span` inside the AppSignal Extension. + */ +export type SpanData = { + closed?: boolean + name?: string + namespace?: string + parent_span_id?: string + span_id?: string + start_time?: number + trace_id?: string + error?: { name: string; message: string; backtrace: Array } + attributes?: { [key: string]: string } } diff --git a/packages/nodejs/src/noops/span.ts b/packages/nodejs/src/noops/span.ts index fc8f270e..0899b64a 100644 --- a/packages/nodejs/src/noops/span.ts +++ b/packages/nodejs/src/noops/span.ts @@ -1,5 +1,5 @@ import { HashMap } from "@appsignal/types" -import { Span } from "../interfaces" +import { Span, SpanData } from "../interfaces" export class NoopSpan implements Span { public get traceId(): string { @@ -52,7 +52,7 @@ export class NoopSpan implements Span { return this } - public toJSON(): string { - return "{}" + public toObject(): SpanData { + return {} } } diff --git a/packages/nodejs/src/span.ts b/packages/nodejs/src/span.ts index f25be7d9..94fcdf94 100644 --- a/packages/nodejs/src/span.ts +++ b/packages/nodejs/src/span.ts @@ -1,6 +1,5 @@ import { HashMap, HashMapValue } from "@appsignal/types" - -import { Span, SpanOptions, SpanContext } from "./interfaces" +import { Span, SpanOptions, SpanContext, SpanData } from "./interfaces" import { span } from "./extension_wrapper" import { Data } from "./internal/data" @@ -190,19 +189,21 @@ export class BaseSpan implements Span { } /** - * Returns a JSON string representing the internal Span in the agent. + * Returns a SpanData object representing the internal Span in the extension. + * + * @private */ - public toJSON(): string { + public toObject(): SpanData { const json = span.spanToJSON(this._ref) // if this is true, then the span has been garbage collected // @TODO: i feel that this could have better ergonomics on the agent // side. come up with something better here later. if (json.trim() === "") { - return JSON.stringify({ closed: true }) + return { closed: true } } - return json + return JSON.parse(json) } }