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.

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.

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()", () => {})
 })

packages/nodejs/src/__tests__/span.test.ts

@@ -2,50 +2,39 @@ 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()
   })
 
   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()
     expect(spanId).toEqual(internal.span_id)
   })
 
   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,17 +54,17 @@ 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)
   })
 
   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()
   })

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,21 +58,21 @@ 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()
       })
     })
 
     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()

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<String> }
+  attributes?: { [key: string]: string }
 }

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 {}
   }
 }

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)
   }
 }