Skip to content

Commit

Permalink
feat(v8/core): Remove span.finish call (#10773)
Browse files Browse the repository at this point in the history
  • Loading branch information
@AbhiPrasad
AbhiPrasad authored Feb 23, 2024
1 parent b194be4 commit 1eeea62
Show file tree
Hide file tree
Showing 8 changed files with 40 additions and 80 deletions.
9 changes: 0 additions & 9 deletions packages/core/src/tracing/sentrySpan.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,15 +397,6 @@ export class SentrySpan implements SpanInterface {
return this;
}

/**
* @inheritDoc
*
* @deprecated Use `.end()` instead.
*/
public finish(endTimestamp?: number): void {
return this.end(endTimestamp);
}

/** @inheritdoc */
public end(endTimestamp?: SpanTimeInput): void {
// If already ended, skip
Expand Down
2 changes: 1 addition & 1 deletion packages/node-experimental/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ const span = Sentry.startInactiveSpan({ description: 'non-active span' });

doSomethingSlow();

span.finish();
span.end();
```

Finally you can also get the currently active span, if you need to do more with it:
Expand Down
2 changes: 1 addition & 1 deletion packages/profiling-node/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const transaction = Sentry.startTransaction({ name: 'some workflow' });

// The code between startTransaction and transaction.finish will be profiled

transaction.finish();
transaction.end();
```

### Building the package from source
Expand Down
12 changes: 5 additions & 7 deletions packages/profiling-node/src/hubextensions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,12 @@ export function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: S
}, maxProfileDurationMs);

// We need to reference the original finish call to avoid creating an infinite loop
// eslint-disable-next-line deprecation/deprecation
const originalFinish = transaction.finish.bind(transaction);
const originalEnd = transaction.end.bind(transaction);

// Wrap the transaction finish method to stop profiling and set the profile on the transaction.
function profilingWrappedTransactionFinish(): void {
function profilingWrappedTransactionEnd(): void {
if (!profile_id) {
return originalFinish();
return originalEnd();
}

// We stop the handler first to ensure that the timeout is cleared and the profile is stopped.
Expand All @@ -207,11 +206,10 @@ export function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: S
// @ts-expect-error profile is not part of metadata
// eslint-disable-next-line deprecation/deprecation
transaction.setMetadata({ profile });
return originalFinish();
return originalEnd();
}

// eslint-disable-next-line deprecation/deprecation
transaction.finish = profilingWrappedTransactionFinish;
transaction.end = profilingWrappedTransactionEnd;
return transaction;
};
}
Expand Down
39 changes: 13 additions & 26 deletions packages/profiling-node/test/hubextensions.hub.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = Sentry.getCurrentHub().startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);
expect(transportSpy.mock.calls?.[0]?.[0]?.[1]?.[0]?.[1]).toMatchObject({ environment: 'test-environment' });
Expand Down Expand Up @@ -138,8 +137,7 @@ describe('hubextensions', () => {

// eslint-disable-next-line deprecation/deprecation
const transaction = Sentry.getCurrentHub().startTransaction({ name: 'profile_hub' });
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand Down Expand Up @@ -188,8 +186,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = Sentry.getCurrentHub().startTransaction({ name: 'profile_hub', traceId: 'boop' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);
expect(logSpy.mock?.calls?.[6]?.[0]).toBe('[Profiling] Invalid traceId: ' + 'boop' + ' on profiled event');
Expand All @@ -211,8 +208,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand All @@ -232,8 +228,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand Down Expand Up @@ -285,8 +280,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand All @@ -308,8 +302,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand All @@ -336,8 +329,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand All @@ -360,8 +352,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand Down Expand Up @@ -392,8 +383,7 @@ describe('hubextensions', () => {
expect(stopProfilingSpy).toHaveBeenCalledTimes(1);
expect((stopProfilingSpy.mock.calls[startProfilingSpy.mock.calls.length - 1]?.[0] as string).length).toBe(32);

// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();
expect(stopProfilingSpy).toHaveBeenCalledTimes(1);
});
});
Expand All @@ -409,10 +399,8 @@ describe('hubextensions', () => {

// eslint-disable-next-line deprecation/deprecation
const transaction = Sentry.getCurrentHub().startTransaction({ name: 'txn' });
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();
transaction.end();
expect(stopProfilingSpy).toHaveBeenCalledTimes(1);
});

Expand Down Expand Up @@ -456,8 +444,7 @@ describe('hubextensions', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = hub.startTransaction({ name: 'profile_hub' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(1000);

Expand Down
24 changes: 12 additions & 12 deletions packages/profiling-node/test/hubextensions.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ function makeTransactionMock(options = {}): Transaction {
tags: {},
sampled: true,
contexts: {},
startChild: () => ({ finish: () => void 0 }),
finish() {
startChild: () => ({ end: () => void 0 }),
end() {
return;
},
toContext: () => {
Expand Down Expand Up @@ -74,7 +74,7 @@ describe('hubextensions', () => {

const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, {});
transaction.finish();
transaction.end();

expect(startTransaction).toHaveBeenCalledTimes(1);
expect(startProfilingSpy).not.toHaveBeenCalled();
Expand All @@ -87,7 +87,7 @@ describe('hubextensions', () => {

const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, {});
transaction.finish();
transaction.end();

expect(startTransaction).toHaveBeenCalledTimes(1);
expect(startProfilingSpy).not.toHaveBeenCalled();
Expand All @@ -102,7 +102,7 @@ describe('hubextensions', () => {

const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, {});
transaction.finish();
transaction.end();

expect(startTransaction).toHaveBeenCalledTimes(1);
expect(startProfilingSpy).not.toHaveBeenCalled();
Expand All @@ -118,7 +118,7 @@ describe('hubextensions', () => {

const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, {});
transaction.finish();
transaction.end();

expect(startTransaction).toHaveBeenCalledTimes(1);
expect(startProfilingSpy).toHaveBeenCalledTimes(1);
Expand All @@ -136,7 +136,7 @@ describe('hubextensions', () => {

const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, {});
transaction.finish();
transaction.end();

expect(startTransaction).toHaveBeenCalledTimes(1);
expect(startProfilingSpy).not.toHaveBeenCalledTimes(1);
Expand All @@ -150,7 +150,7 @@ describe('hubextensions', () => {
const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const samplingContext = { beep: 'boop' };
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext);
transaction.finish();
transaction.end();

const startProfilingSpy = jest.spyOn(CpuProfilerBindings, 'startProfiling');
expect(startProfilingSpy).not.toHaveBeenCalled();
Expand All @@ -171,7 +171,7 @@ describe('hubextensions', () => {
const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const samplingContext = { beep: 'boop' };
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext);
transaction.finish();
transaction.end();

expect(options.profilesSampler).toHaveBeenCalled();
expect(startProfilingSpy).not.toHaveBeenCalled();
Expand All @@ -192,7 +192,7 @@ describe('hubextensions', () => {
const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const samplingContext = { beep: 'boop' };
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext);
transaction.finish();
transaction.end();

expect(options.profilesSampler).toHaveBeenCalled();
expect(startProfilingSpy).not.toHaveBeenCalled();
Expand All @@ -212,7 +212,7 @@ describe('hubextensions', () => {
const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const samplingContext = { beep: 'boop' };
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext);
transaction.finish();
transaction.end();

expect(options.profilesSampler).toHaveBeenCalledWith({
...samplingContext,
Expand All @@ -235,7 +235,7 @@ describe('hubextensions', () => {
const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction);
const samplingContext = { beep: 'boop' };
const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext);
transaction.finish();
transaction.end();

expect(startProfilingSpy).toHaveBeenCalled();
});
Expand Down
23 changes: 8 additions & 15 deletions packages/profiling-node/test/index.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ describe('Sentry - Profiling', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = Sentry.startTransaction({ name: 'title' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();

await Sentry.flush(500);
expect(findProfile(transport)).not.toBe(null);
Expand All @@ -108,10 +107,8 @@ describe('Sentry - Profiling', () => {
const t2 = Sentry.startTransaction({ name: 'inner' });
await wait(500);

// eslint-disable-next-line deprecation/deprecation
t2.finish();
// eslint-disable-next-line deprecation/deprecation
t1.finish();
t2.end();
t1.end();

await Sentry.flush(500);

Expand All @@ -133,17 +130,15 @@ describe('Sentry - Profiling', () => {
// eslint-disable-next-line deprecation/deprecation
const t2 = Sentry.startTransaction({ name: 'same-title' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
t2.finish();
// eslint-disable-next-line deprecation/deprecation
t1.finish();
t2.end();
t1.end();

await Sentry.flush(500);
expect(findAllProfiles(transport)).toHaveLength(2);
expect(findProfile(transport)).not.toBe(null);
});

it('does not crash if finish is called multiple times', async () => {
it('does not crash if end is called multiple times', async () => {
const [client, transport] = makeClientWithoutHooks();
// eslint-disable-next-line deprecation/deprecation
const hub = Sentry.getCurrentHub();
Expand All @@ -153,10 +148,8 @@ describe('Sentry - Profiling', () => {
// eslint-disable-next-line deprecation/deprecation
const transaction = Sentry.startTransaction({ name: 'title' });
await wait(500);
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
// eslint-disable-next-line deprecation/deprecation
transaction.finish();
transaction.end();
transaction.end();

await Sentry.flush(500);
expect(findAllProfiles(transport)).toHaveLength(1);
Expand Down
9 changes: 0 additions & 9 deletions packages/types/src/span.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,15 +245,6 @@ export interface Span extends Omit<SpanContext, 'name' | 'op' | 'status' | 'orig
*/
spanContext(): SpanContextData;

/**
* Sets the finish timestamp on the current span.
*
* @param endTimestamp Takes an endTimestamp if the end should not be the time when you call this function.
*
* @deprecated Use `.end()` instead.
*/
finish(endTimestamp?: number): void;

/**
* End the current span.
*/
Expand Down

0 comments on commit 1eeea62

Please sign in to comment.