Skip to content

Commit ca7e5e6

Browse files
s1gr1ds1gr1d
committed
add review changes
1 parent 941722d commit ca7e5e6

4 files changed

Lines changed: 122 additions & 14 deletions

File tree

‎packages/core/src/tracing/trace.ts‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
7070
const parentSpan = getParentSpan(scope, customParentSpan);
7171
const client = getClient();
7272

73-
const shouldSkipSpan = options.onlyIfParent && !parentSpan;
74-
const activeSpan = shouldSkipSpan
73+
const missingRequiredParent = options.onlyIfParent && !parentSpan;
74+
const activeSpan = missingRequiredParent
7575
? new SentryNonRecordingSpan()
7676
: createChildOrRootSpan({
7777
parentSpan,
@@ -80,7 +80,7 @@ export function startSpan<T>(options: StartSpanOptions, callback: (span: Span) =
8080
scope,
8181
});
8282

83-
if (shouldSkipSpan) {
83+
if (missingRequiredParent) {
8484
client?.recordDroppedEvent('no_parent_span', 'span');
8585
}
8686

@@ -137,8 +137,8 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
137137
const scope = getCurrentScope();
138138
const parentSpan = getParentSpan(scope, customParentSpan);
139139

140-
const shouldSkipSpan = options.onlyIfParent && !parentSpan;
141-
const activeSpan = shouldSkipSpan
140+
const missingRequiredParent = options.onlyIfParent && !parentSpan;
141+
const activeSpan = missingRequiredParent
142142
? new SentryNonRecordingSpan()
143143
: createChildOrRootSpan({
144144
parentSpan,
@@ -147,7 +147,7 @@ export function startSpanManual<T>(options: StartSpanOptions, callback: (span: S
147147
scope,
148148
});
149149

150-
if (shouldSkipSpan) {
150+
if (missingRequiredParent) {
151151
getClient()?.recordDroppedEvent('no_parent_span', 'span');
152152
}
153153

@@ -206,9 +206,9 @@ export function startInactiveSpan(options: StartSpanOptions): Span {
206206
const parentSpan = getParentSpan(scope, customParentSpan);
207207
const client = getClient();
208208

209-
const shouldSkipSpan = options.onlyIfParent && !parentSpan;
209+
const missingRequiredParent = options.onlyIfParent && !parentSpan;
210210

211-
if (shouldSkipSpan) {
211+
if (missingRequiredParent) {
212212
client?.recordDroppedEvent('no_parent_span', 'span');
213213
return new SentryNonRecordingSpan();
214214
}

‎packages/core/test/lib/tracing/trace.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -600,6 +600,16 @@ describe('startSpan', () => {
600600
expect(span).toBeInstanceOf(SentrySpan);
601601
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
602602
});
603+
604+
it('does not record no_parent_span client report when onlyIfParent is not set', () => {
605+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
606+
607+
startSpan({ name: 'root span without onlyIfParent' }, span => {
608+
return span;
609+
});
610+
611+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
612+
});
603613
});
604614

605615
describe('parentSpanIsAlwaysRootSpan', () => {
@@ -1220,6 +1230,17 @@ describe('startSpanManual', () => {
12201230
expect(span).toBeInstanceOf(SentrySpan);
12211231
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
12221232
});
1233+
1234+
it('does not record no_parent_span client report when onlyIfParent is not set', () => {
1235+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
1236+
1237+
startSpanManual({ name: 'root span without onlyIfParent' }, span => {
1238+
span.end();
1239+
return span;
1240+
});
1241+
1242+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
1243+
});
12231244
});
12241245

12251246
describe('parentSpanIsAlwaysRootSpan', () => {
@@ -1626,6 +1647,15 @@ describe('startInactiveSpan', () => {
16261647
expect(span).toBeInstanceOf(SentrySpan);
16271648
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
16281649
});
1650+
1651+
it('does not record no_parent_span client report when onlyIfParent is not set', () => {
1652+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
1653+
1654+
const span = startInactiveSpan({ name: 'root span without onlyIfParent' });
1655+
span.end();
1656+
1657+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
1658+
});
16291659
});
16301660

16311661
describe('parentSpanIsAlwaysRootSpan', () => {

‎packages/opentelemetry/src/trace.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,10 @@ function _startSpan<T>(options: OpenTelemetrySpanContext, callback: (span: Span)
4848

4949
return wrapper(() => {
5050
const activeCtx = getContext(options.scope, options.forceTransaction);
51-
const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx);
52-
const ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;
51+
const missingRequiredParent = options.onlyIfParent && !trace.getSpan(activeCtx);
52+
const ctx = missingRequiredParent ? suppressTracing(activeCtx) : activeCtx;
5353

54-
if (shouldSkipSpan) {
54+
if (missingRequiredParent) {
5555
getClient()?.recordDroppedEvent('no_parent_span', 'span');
5656
}
5757

@@ -155,10 +155,10 @@ export function startInactiveSpan(options: OpenTelemetrySpanContext): Span {
155155

156156
return wrapper(() => {
157157
const activeCtx = getContext(options.scope, options.forceTransaction);
158-
const shouldSkipSpan = options.onlyIfParent && !trace.getSpan(activeCtx);
159-
let ctx = shouldSkipSpan ? suppressTracing(activeCtx) : activeCtx;
158+
const missingRequiredParent = options.onlyIfParent && !trace.getSpan(activeCtx);
159+
let ctx = missingRequiredParent ? suppressTracing(activeCtx) : activeCtx;
160160

161-
if (shouldSkipSpan) {
161+
if (missingRequiredParent) {
162162
getClient()?.recordDroppedEvent('no_parent_span', 'span');
163163
}
164164

‎packages/opentelemetry/test/trace.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,32 @@ describe('trace', () => {
558558
expect(isSpan(span)).toBe(true);
559559
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
560560
});
561+
562+
it('does not record no_parent_span client report when onlyIfParent is not set', () => {
563+
const client = getClient()!;
564+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
565+
566+
context.with(ROOT_CONTEXT, () => {
567+
startSpan({ name: 'root span without onlyIfParent' }, span => {
568+
return span;
569+
});
570+
});
571+
572+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
573+
});
574+
575+
it('does not record no_parent_span client report when onlyIfParent is false even without a parent', () => {
576+
const client = getClient()!;
577+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
578+
579+
context.with(ROOT_CONTEXT, () => {
580+
startSpan({ name: 'root span', onlyIfParent: false }, span => {
581+
return span;
582+
});
583+
});
584+
585+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
586+
});
561587
});
562588
});
563589

@@ -857,6 +883,30 @@ describe('trace', () => {
857883
expect(isSpan(span)).toBe(true);
858884
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
859885
});
886+
887+
it('does not record no_parent_span client report when onlyIfParent is not set', () => {
888+
const client = getClient()!;
889+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
890+
891+
context.with(ROOT_CONTEXT, () => {
892+
const span = startInactiveSpan({ name: 'root span without onlyIfParent' });
893+
span.end();
894+
});
895+
896+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
897+
});
898+
899+
it('does not record no_parent_span client report when onlyIfParent is false even without a parent', () => {
900+
const client = getClient()!;
901+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
902+
903+
context.with(ROOT_CONTEXT, () => {
904+
const span = startInactiveSpan({ name: 'root span', onlyIfParent: false });
905+
span.end();
906+
});
907+
908+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
909+
});
860910
});
861911

862912
it('includes the scope at the time the span was started when finished', async () => {
@@ -1238,6 +1288,34 @@ describe('trace', () => {
12381288
expect(isSpan(span)).toBe(true);
12391289
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
12401290
});
1291+
1292+
it('does not record no_parent_span client report when onlyIfParent is not set', () => {
1293+
const client = getClient()!;
1294+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
1295+
1296+
context.with(ROOT_CONTEXT, () => {
1297+
startSpanManual({ name: 'root span without onlyIfParent' }, span => {
1298+
span.end();
1299+
return span;
1300+
});
1301+
});
1302+
1303+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
1304+
});
1305+
1306+
it('does not record no_parent_span client report when onlyIfParent is false even without a parent', () => {
1307+
const client = getClient()!;
1308+
const spyOnDroppedEvent = vi.spyOn(client, 'recordDroppedEvent');
1309+
1310+
context.with(ROOT_CONTEXT, () => {
1311+
startSpanManual({ name: 'root span', onlyIfParent: false }, span => {
1312+
span.end();
1313+
return span;
1314+
});
1315+
});
1316+
1317+
expect(spyOnDroppedEvent).not.toHaveBeenCalledWith('no_parent_span', 'span');
1318+
});
12411319
});
12421320
});
12431321

0 commit comments

Comments
 (0)