Skip to content

Commit 2c5b8f8

Browse files
nicohrubeccodex
andauthored
fix(server-utils): Use onlyIfParent for Firestore spans (#25877)
**Why:** Consolidating to use the core `onlyIfParent` logic everywhere so we can get rid of `requiresParentSpan`. Co-authored-by: GPT-6 <codex@openai.com>
1 parent ab1bc56 commit 2c5b8f8

5 files changed

Lines changed: 11 additions & 96 deletions

File tree

‎dev-packages/deno-integration-tests/suites/orchestrion-firebase/test.ts‎

Lines changed: 0 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { assert } from 'https://deno.land/std@0.212.0/assert/assert.ts';
77
import { assertExists } from 'https://deno.land/std@0.212.0/assert/assert_exists.ts';
88
import { assertEquals } from 'https://deno.land/std@0.212.0/assert/assert_equals.ts';
99
import { getSpanOp, resetGlobals, spanSink, withTimeout } from '../../src/index.ts';
10-
import { DB_QUERY } from '@sentry/conventions/op';
1110

1211
Deno.test('firebase instrumentation: included in default integrations (Deno 2.8.0+)', () => {
1312
resetGlobals();
@@ -73,49 +72,3 @@ Deno.test('firebase instrumentation: orchestrion @firebase/firestore:add-doc cha
7372
assertEquals(fsSpan.attributes['firebase.firestore.options.projectId']?.value, 'demo-project');
7473
assertEquals(fsSpan.attributes['sentry.origin']?.value, 'auto.db.firebase.firestore');
7574
});
76-
77-
Deno.test('firebase instrumentation: only creates firestore db spans with an active parent span', async () => {
78-
resetGlobals();
79-
const sink = spanSink();
80-
init({
81-
dsn: 'https://username@domain/123',
82-
tracesSampleRate: 1,
83-
transport: sink.transport,
84-
});
85-
86-
const channel = tracingChannel('orchestrion:@firebase/firestore:add-doc');
87-
88-
function addDoc(path: string): void {
89-
const reference = {
90-
path,
91-
type: 'collection',
92-
firestore: {
93-
app: { name: '[DEFAULT]', options: { projectId: 'demo-project', appId: 'demo-app' } },
94-
toJSON: () => ({ settings: {} }),
95-
},
96-
};
97-
const ctx: Record<string, unknown> = { arguments: [reference] };
98-
channel.start.runStores(ctx, () => undefined);
99-
channel.end.publish(ctx);
100-
ctx.result = {};
101-
channel.asyncStart.runStores(ctx, () => undefined);
102-
channel.asyncEnd.publish(ctx);
103-
}
104-
105-
addDoc('before_parent');
106-
startSpan({ name: 'parent', op: 'test' }, () => addDoc('inside_parent'));
107-
addDoc('after_parent');
108-
109-
await flush();
110-
111-
const parent = await withTimeout(
112-
sink.waitFor(span => span.is_segment && span.name === 'parent'),
113-
5000,
114-
"'parent' segment span",
115-
);
116-
const dbSpans = sink.spans.filter(span => getSpanOp(span) === DB_QUERY);
117-
assertEquals(
118-
dbSpans.map(span => ({ name: span.name, parentSpanId: span.parent_span_id, isSegment: span.is_segment })),
119-
[{ name: 'addDoc inside_parent', parentSpanId: parent.span_id, isSegment: false }],
120-
);
121-
});

‎dev-packages/e2e-tests/test-applications/node-firebase/firestore-app/src/app.ts‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -66,18 +66,6 @@ expressApp.get('/test', async function (req, res) {
6666
res.send({ version: 'v1' });
6767
});
6868

69-
expressApp.get('/test-no-parent', async function (req, res) {
70-
const noParentRef = collection(db, 'no-parent');
71-
await addDoc(noParentRef, { name: 'Without Parent' });
72-
// Flush before the marker so a span from the call above would reach the proxy before it.
73-
await Sentry.flush();
74-
await Sentry.startSpan({ name: 'No Parent Marker' }, async () => {
75-
await addDoc(noParentRef, { name: 'With Parent' });
76-
});
77-
await Sentry.flush();
78-
res.send({ version: 'v1' });
79-
});
80-
8169
expressApp.listen(port, () => {
8270
console.log(`Example app listening on port ${port}`);
8371
});

‎dev-packages/e2e-tests/test-applications/node-firebase/tests/spans.test.ts‎

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { expect, test } from '@playwright/test';
2-
import type { SerializedStreamedSpan } from '@sentry-internal/test-utils';
3-
import { collectStreamedSpansUntilSegment, waitForStreamedSpans } from '@sentry-internal/test-utils';
2+
import { collectStreamedSpansUntilSegment } from '@sentry-internal/test-utils';
43

54
// The orchestrion spans are Sentry-native, so they carry no span-kind attribute (`sentry.kind`).
65
const origin = 'auto.db.firebase.firestore';
@@ -50,26 +49,3 @@ test('should add, set, get and delete document', async ({ baseURL }) => {
5049
expect(children).toHaveLength(4);
5150
expect(children).toEqual(expect.arrayContaining([spanAddDoc, spanSetDocs, spanGetDocs, spanDeleteDoc]));
5251
});
53-
54-
test('does not create firestore spans without a parent span', async ({ baseURL }) => {
55-
const seen: SerializedStreamedSpan[] = [];
56-
// A span that is never sent cannot be awaited, so wait for a marker span sent after the Firestore call.
57-
const markerPromise = waitForStreamedSpans('node-firebase', spans => {
58-
seen.push(...spans);
59-
return spans.some(span => span.is_segment && span.name === 'No Parent Marker');
60-
});
61-
62-
const response = await fetch(`${baseURL}/test-no-parent`);
63-
expect(response.ok).toBe(true);
64-
await markerPromise;
65-
66-
const marker = seen.find(span => span.is_segment && span.name === 'No Parent Marker')!;
67-
// Only the call inside the marker span may produce a span, as its child.
68-
expect(seen.filter(span => span.name === 'addDoc no-parent')).toEqual([
69-
expect.objectContaining({
70-
is_segment: false,
71-
parent_span_id: marker.span_id,
72-
trace_id: marker.trace_id,
73-
}),
74-
]);
75-
});

‎packages/server-utils/src/integrations/firebase/firestore.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import type { FirebaseApp, FirebaseOptions, FirestoreReference, FirestoreSetting
2222
export function startFirestoreSpan(spanName: string, reference: FirestoreReference): Span {
2323
return startInactiveSpan({
2424
name: `${spanName} ${reference.path}`,
25+
onlyIfParent: true,
2526
attributes: {
2627
[SENTRY_OP]: DB_QUERY,
2728
[SENTRY_KIND]: 'client',

‎packages/server-utils/src/integrations/firebase/instrumentation.ts‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,15 @@ const NOOP = (): void => {};
3737

3838
export function instrumentFirebase() {
3939
for (const { channel, spanName, useParent } of FIRESTORE_OPERATIONS) {
40-
bindTracingChannelToSpan(
41-
diagnosticsChannel.tracingChannel<OrchestrionChannelContext>(channel),
42-
data =>
43-
safeChannelCallback(() => {
44-
const reference = data.arguments[0] as FirestoreReference | undefined;
45-
if (!reference) {
46-
return undefined;
47-
}
48-
const spanReference = useParent ? reference.parent || reference : reference;
49-
return startFirestoreSpan(spanName, spanReference);
50-
}),
51-
{ requiresParentSpan: true },
40+
bindTracingChannelToSpan(diagnosticsChannel.tracingChannel<OrchestrionChannelContext>(channel), data =>
41+
safeChannelCallback(() => {
42+
const reference = data.arguments[0] as FirestoreReference | undefined;
43+
if (!reference) {
44+
return undefined;
45+
}
46+
const spanReference = useParent ? reference.parent || reference : reference;
47+
return startFirestoreSpan(spanName, spanReference);
48+
}),
5249
);
5350
}
5451

0 commit comments

Comments
 (0)