feat(core): Deprecate session APIs on hub and add global replacements #10054

Lms24 · 2024-01-04T12:12:40Z

Deprecates and adds top-level replacements for

hub.startSession -> Sentry.startSession
hub.endSession -> Sentry.endSession
hub.captureException -> Sentry.captureSession

These APIs were mostly used in browser and node startSessionTracking functions which were called on Sentry.init. I updated these usages with the global replacements and it seems integration tests are happy with it.
For now, we not only put the session onto the isolation scope but also onto the current scope to avoid changing event processing and stuff like the ANR worker, as well as avoid "soft-breaking" users who use hub.(get|set)Session.

Also, it looks like we didn't have unit tests for session APIs before (but we did have integration tests). Therefore

added integration test, explicitly testing the deprecated hub.startSession API
previously existing session integration tests now test the new top level API
added basic unit tests for the deprecated API
added basic unit tests for the new top level API

ref #10033

Lms24 · 2024-01-04T12:16:24Z

packages/core/src/baseclient.ts

@@ -721,7 +721,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
          throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log');
        }

-        const session = scope && scope.getSession();
+        const session = (scope && scope.getSession()) || getIsolationScope().getSession();


This makes me a bit uncomfortable tbh, not sure if we for now should simply still use hub.startSession in browser/node startSessionTracking. But this change seems to be necessary for browser, since scope.getSession() returned undefined

hmm, is this correct in the new implementation, I wonder? Or should we simply always take the one from the isolation scope? 🤔 because the scope here would generally be the final scope being applied and can even be a temporary scope, like new Scope() 🤔

thinking about this, I guess it doesn't hurt, if you pass a scope that actually has a session on it I guess it makes sense that we use this. But it will/should use the isolationScope in 99% of cases I guess?

hmm, is this correct in the new implementation, I wonder?

You mean that we set it onto the isolation scope? I think that should be correct 🤔
EDIT: But not "complete" (see answer below)

Or should we simply always take the one from the isolation scope? 🤔

I guess it would be more correct if we're okay with not supporting people adding their own session to their current scope 🤔 Not sure if that's even a thing...

Oh wait, for v7 we probably still need to set the session also on the current scope, right? So that getCurrentHub().getScope().getSession() still returns the session

This probably makes calling getIsolationScope obsolete here

hmm I guess so, you're right! :(

Reworked the top level APIs to also modify the current scope's session. For now this will work and spare us mocking with event processing

github-actions · 2024-01-04T12:19:56Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	76.26 KB (+0.16% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	67.64 KB (+0.18% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	61.26 KB (+0.19% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	32.28 KB (+0.47% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	30.62 KB (+0.42% 🔺)
@sentry/browser - Webpack (gzipped)	22.37 KB (+0.75% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	73.71 KB (+0.17% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	65.36 KB (+0.21% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	31.53 KB (+0.35% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	23.49 KB (+0.57% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	204.96 KB (+0.24% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	94.85 KB (+0.52% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	69.71 KB (+0.71% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	34.48 KB (+0.26% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	68.02 KB (+0.14% 🔺)
@sentry/react - Webpack (gzipped)	22.4 KB (+0.74% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	84.75 KB (+0.19% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	49.36 KB (+0.34% 🔺)
@sentry-internal/feedback - Webpack (gzipped)	16.73 KB (0%)

mydea · 2024-01-04T13:10:29Z

packages/core/src/exports.ts

+  const client = getClient();
+  const isolationScope = getIsolationScope();
+
+  const { release, environment = DEFAULT_ENVIRONMENT } = (client && client.getOptions()) || {};


l: should we maybe just bail out if no client is found? Doesn't matter probably, but for clarity...

Hmm thinking about this, if we do so, we have to return undefined which I'm not sure we want to do. Also this means the signature of the replacement would slightly differ from the deprecated hub method. Since we're only taking env and release from the client I think it's okay to continue. Obviously, no
session will be sent but to me this aligns more with the concept of no-op spans for example.

ok, sounds good to me!

mydea · 2024-01-04T13:11:24Z

packages/core/src/exports.ts

+  });
+
+  // End existing session if there's one
+  const currentSession = isolationScope.getSession && isolationScope.getSession();


l: IMHO no need to check for getSession existance, if you have code without this everything will fail here anyhow I guess xD

packages/node/src/integrations/anr/index.ts

…#10054) Deprecates and adds top-level replacements for * `hub.startSession` -> `Sentry.startSession` * `hub.endSession` -> `Sentry.endSession` * `hub.captureException` -> `Sentry.captureSession` These APIs were mostly used in browser and node `startSessionTracking` functions which were called on `Sentry.init`. I updated these usages with the global replacements and it seems integration tests are happy with it. For now, we not only put the session onto the isolation scope but also onto the current scope to avoid changing event processing and stuff like the ANR worker, as well as avoid "soft-breaking" users who use `hub.(get|set)Session`.

Lms24 commented Jan 4, 2024

View reviewed changes

Lms24 requested review from mydea and lforst January 4, 2024 12:34

mydea reviewed Jan 4, 2024

View reviewed changes

Lms24 self-assigned this Jan 4, 2024

Lms24 marked this pull request as ready for review January 4, 2024 15:13

Lms24 requested review from AbhiPrasad and mydea January 4, 2024 15:13

Lms24 force-pushed the lms/feat-start-end-session branch 2 times, most recently from 7c8f841 to f392633 Compare January 4, 2024 16:51

mydea reviewed Jan 5, 2024

View reviewed changes

packages/node/src/integrations/anr/index.ts Outdated Show resolved Hide resolved

mydea approved these changes Jan 5, 2024

View reviewed changes

Lms24 added 9 commits January 5, 2024 13:41

feat(core): Deprecate session APIs on hub and add global replacements

76fbf9d

s/getCurrentScope/getIsolationScope

9302f38

fix node 8 test

103cd17

fix passing session to ANR worker

70b2cec

bring back currentScope.(get|set)Session

29ffcef

add migration entry

e9de0d1

add integration test for deprecated API

5ce536a

add unit tests for deprecated API

5a83b95

fix anr getCurrentScope

566016d

Lms24 force-pushed the lms/feat-start-end-session branch from 24181c7 to 566016d Compare January 5, 2024 12:45

lint lint lint

86fb99c

Lms24 enabled auto-merge (squash) January 5, 2024 12:57

Lms24 merged commit ecb4f7f into develop Jan 5, 2024

Lms24 deleted the lms/feat-start-end-session branch January 5, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Deprecate session APIs on hub and add global replacements #10054

feat(core): Deprecate session APIs on hub and add global replacements #10054

Lms24 commented Jan 4, 2024 •

edited

Loading

Lms24 Jan 4, 2024

mydea Jan 4, 2024

mydea Jan 4, 2024

Lms24 Jan 4, 2024 •

edited

Loading

Lms24 Jan 4, 2024

Lms24 Jan 4, 2024

mydea Jan 4, 2024

Lms24 Jan 4, 2024

github-actions bot commented Jan 4, 2024 •

edited

Loading

mydea Jan 4, 2024

Lms24 Jan 4, 2024 •

edited

Loading

mydea Jan 5, 2024

mydea Jan 4, 2024

feat(core): Deprecate session APIs on hub and add global replacements #10054

feat(core): Deprecate session APIs on hub and add global replacements #10054

Conversation

Lms24 commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Lms24 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 commented Jan 4, 2024 •

edited

Loading

Lms24 Jan 4, 2024 •

edited

Loading

github-actions bot commented Jan 4, 2024 •

edited

Loading

Lms24 Jan 4, 2024 •

edited

Loading