From 5a65879d0f9bd825905080cf481b77f314d98bd8 Mon Sep 17 00:00:00 2001 From: Nicolas Hrubec Date: Thu, 29 Oct 2026 14:04:08 +0100 Subject: [PATCH] fix(server-utils): Use onlyIfParent for Firestore spans Co-Authored-By: GPT-6 --- .../suites/orchestrion-firebase/test.ts | 47 ------------------- .../node-firebase/firestore-app/src/app.ts | 12 ----- .../node-firebase/tests/spans.test.ts | 26 +--------- .../src/integrations/firebase/firestore.ts | 1 + .../integrations/firebase/instrumentation.ts | 21 ++++----- 5 files changed, 11 insertions(+), 96 deletions(-) diff --git a/dev-packages/deno-integration-tests/suites/orchestrion-firebase/test.ts b/dev-packages/deno-integration-tests/suites/orchestrion-firebase/test.ts index abc063529480..cb131997dca9 100644 --- a/dev-packages/deno-integration-tests/suites/orchestrion-firebase/test.ts +++ b/dev-packages/deno-integration-tests/suites/orchestrion-firebase/test.ts @@ -7,7 +7,6 @@ import { assert } from 'https://deno.land/std@0.212.0/assert/assert.ts'; import { assertExists } from 'https://deno.land/std@0.212.0/assert/assert_exists.ts'; import { assertEquals } from 'https://deno.land/std@0.212.0/assert/assert_equals.ts'; import { getSpanOp, resetGlobals, spanSink, withTimeout } from '../../src/index.ts'; -import { DB_QUERY } from '@sentry/conventions/op'; Deno.test('firebase instrumentation: included in default integrations (Deno 2.8.0+)', () => { resetGlobals(); @@ -73,49 +72,3 @@ Deno.test('firebase instrumentation: orchestrion @firebase/firestore:add-doc cha assertEquals(fsSpan.attributes['firebase.firestore.options.projectId']?.value, 'demo-project'); assertEquals(fsSpan.attributes['sentry.origin']?.value, 'auto.db.firebase.firestore'); }); - -Deno.test('firebase instrumentation: only creates firestore db spans with an active parent span', async () => { - resetGlobals(); - const sink = spanSink(); - init({ - dsn: 'https://username@domain/123', - tracesSampleRate: 1, - transport: sink.transport, - }); - - const channel = tracingChannel('orchestrion:@firebase/firestore:add-doc'); - - function addDoc(path: string): void { - const reference = { - path, - type: 'collection', - firestore: { - app: { name: '[DEFAULT]', options: { projectId: 'demo-project', appId: 'demo-app' } }, - toJSON: () => ({ settings: {} }), - }, - }; - const ctx: Record = { arguments: [reference] }; - channel.start.runStores(ctx, () => undefined); - channel.end.publish(ctx); - ctx.result = {}; - channel.asyncStart.runStores(ctx, () => undefined); - channel.asyncEnd.publish(ctx); - } - - addDoc('before_parent'); - startSpan({ name: 'parent', op: 'test' }, () => addDoc('inside_parent')); - addDoc('after_parent'); - - await flush(); - - const parent = await withTimeout( - sink.waitFor(span => span.is_segment && span.name === 'parent'), - 5000, - "'parent' segment span", - ); - const dbSpans = sink.spans.filter(span => getSpanOp(span) === DB_QUERY); - assertEquals( - dbSpans.map(span => ({ name: span.name, parentSpanId: span.parent_span_id, isSegment: span.is_segment })), - [{ name: 'addDoc inside_parent', parentSpanId: parent.span_id, isSegment: false }], - ); -}); diff --git a/dev-packages/e2e-tests/test-applications/node-firebase/firestore-app/src/app.ts b/dev-packages/e2e-tests/test-applications/node-firebase/firestore-app/src/app.ts index 83d04bebd136..486aa06b5ffc 100644 --- a/dev-packages/e2e-tests/test-applications/node-firebase/firestore-app/src/app.ts +++ b/dev-packages/e2e-tests/test-applications/node-firebase/firestore-app/src/app.ts @@ -66,18 +66,6 @@ expressApp.get('/test', async function (req, res) { res.send({ version: 'v1' }); }); -expressApp.get('/test-no-parent', async function (req, res) { - const noParentRef = collection(db, 'no-parent'); - await addDoc(noParentRef, { name: 'Without Parent' }); - // Flush before the marker so a span from the call above would reach the proxy before it. - await Sentry.flush(); - await Sentry.startSpan({ name: 'No Parent Marker' }, async () => { - await addDoc(noParentRef, { name: 'With Parent' }); - }); - await Sentry.flush(); - res.send({ version: 'v1' }); -}); - expressApp.listen(port, () => { console.log(`Example app listening on port ${port}`); }); diff --git a/dev-packages/e2e-tests/test-applications/node-firebase/tests/spans.test.ts b/dev-packages/e2e-tests/test-applications/node-firebase/tests/spans.test.ts index 836238fc3add..cd9717ccdea0 100644 --- a/dev-packages/e2e-tests/test-applications/node-firebase/tests/spans.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-firebase/tests/spans.test.ts @@ -1,6 +1,5 @@ import { expect, test } from '@playwright/test'; -import type { SerializedStreamedSpan } from '@sentry-internal/test-utils'; -import { collectStreamedSpansUntilSegment, waitForStreamedSpans } from '@sentry-internal/test-utils'; +import { collectStreamedSpansUntilSegment } from '@sentry-internal/test-utils'; // The orchestrion spans are Sentry-native, so they carry no span-kind attribute (`sentry.kind`). const origin = 'auto.db.firebase.firestore'; @@ -50,26 +49,3 @@ test('should add, set, get and delete document', async ({ baseURL }) => { expect(children).toHaveLength(4); expect(children).toEqual(expect.arrayContaining([spanAddDoc, spanSetDocs, spanGetDocs, spanDeleteDoc])); }); - -test('does not create firestore spans without a parent span', async ({ baseURL }) => { - const seen: SerializedStreamedSpan[] = []; - // A span that is never sent cannot be awaited, so wait for a marker span sent after the Firestore call. - const markerPromise = waitForStreamedSpans('node-firebase', spans => { - seen.push(...spans); - return spans.some(span => span.is_segment && span.name === 'No Parent Marker'); - }); - - const response = await fetch(`${baseURL}/test-no-parent`); - expect(response.ok).toBe(true); - await markerPromise; - - const marker = seen.find(span => span.is_segment && span.name === 'No Parent Marker')!; - // Only the call inside the marker span may produce a span, as its child. - expect(seen.filter(span => span.name === 'addDoc no-parent')).toEqual([ - expect.objectContaining({ - is_segment: false, - parent_span_id: marker.span_id, - trace_id: marker.trace_id, - }), - ]); -}); diff --git a/packages/server-utils/src/integrations/firebase/firestore.ts b/packages/server-utils/src/integrations/firebase/firestore.ts index 155e07797d1a..3154e51869be 100644 --- a/packages/server-utils/src/integrations/firebase/firestore.ts +++ b/packages/server-utils/src/integrations/firebase/firestore.ts @@ -22,6 +22,7 @@ import type { FirebaseApp, FirebaseOptions, FirestoreReference, FirestoreSetting export function startFirestoreSpan(spanName: string, reference: FirestoreReference): Span { return startInactiveSpan({ name: `${spanName} ${reference.path}`, + onlyIfParent: true, attributes: { [SENTRY_OP]: DB_QUERY, [SENTRY_KIND]: 'client', diff --git a/packages/server-utils/src/integrations/firebase/instrumentation.ts b/packages/server-utils/src/integrations/firebase/instrumentation.ts index 246ebd9895d8..b10534aea30a 100644 --- a/packages/server-utils/src/integrations/firebase/instrumentation.ts +++ b/packages/server-utils/src/integrations/firebase/instrumentation.ts @@ -37,18 +37,15 @@ const NOOP = (): void => {}; export function instrumentFirebase() { for (const { channel, spanName, useParent } of FIRESTORE_OPERATIONS) { - bindTracingChannelToSpan( - diagnosticsChannel.tracingChannel(channel), - data => - safeChannelCallback(() => { - const reference = data.arguments[0] as FirestoreReference | undefined; - if (!reference) { - return undefined; - } - const spanReference = useParent ? reference.parent || reference : reference; - return startFirestoreSpan(spanName, spanReference); - }), - { requiresParentSpan: true }, + bindTracingChannelToSpan(diagnosticsChannel.tracingChannel(channel), data => + safeChannelCallback(() => { + const reference = data.arguments[0] as FirestoreReference | undefined; + if (!reference) { + return undefined; + } + const spanReference = useParent ? reference.parent || reference : reference; + return startFirestoreSpan(spanName, spanReference); + }), ); }