sentry-docs icon indicating copy to clipboard operation
sentry-docs copied to clipboard

Add instructions for adding initial scope when using BrowserClient

Open yuval-sentry opened this issue 3 years ago • 1 comments

Core or SDK?

Platform/SDK

Which part? Which one?

Javascript

Description

Per our client: "The real problem here is that BrowserClient accepts an initialScope parameter in its constructor and in its TypeScript types but does not actually do anything with it, leading to confusion. This is likely something that should be fixed by either not accepting the initialScope parameter in BrowserClient, or by actually acting on the initialScope parameter. Once I looked at the source code and discovered that initialScope does nothing, I figured out the rest. At least documenting this solution for capturing user sessions with BrowserClient on the docs site would be helpful."

Suggested Solution

Add a reference for when using BrowserClient with a code sample like this one:

const initialScope = new Sentry.Scope();
  initialScope.update({
    user: { id: '1234-test' },
  });

  const sentryHub = new Sentry.Hub(sentryClient, initialScope);
  sentryHub.startSession();
  sentryHub.captureSession();

yuval-sentry avatar Jul 26 '22 21:07 yuval-sentry

Routing to @getsentry/team-web-sdk-frontend for triage. ⏲️

getsentry-release avatar Jul 27 '22 12:07 getsentry-release

Just ran into this. I was migrating code from using Sentry.init() to using BrowserClient and a Hub directly and thought I could get clever and use initialScope as documented here.

It's not a huge deal as the suggested workaround works, but it does lead to some unnecessary confusion and debugging.

ba0708 avatar May 23 '23 15:05 ba0708

I think the underlying issue here is that we restructured Sentry.init and Client options so that they're no longer identical in v7. Based on what I can see here, and on my reproduction to confirm that this still no-ops (it does, see https://github.com/Lms24/gh-docs-6945), I think initialScope doesn't belong to the ClientOptions ~but should only be available for the top-level Sentry.init~. However, this is a breaking change so we can't remove it from ClientOptions until v8.

As for setting an initial scope in a direct hub/client scenario, the code snippet from @yuval-sentry shows how to do it correctly, namely pass it to the Hub constructor (also, see my repro how to fully initialize a client as this is missing in the snippet).

Lms24 avatar May 24 '23 08:05 Lms24

I did some more digging as to why we actually have this option and it seems to be only present in the JS SDK (i.e. is not part of our unified SDK API) because our Electron SDK needs it: https://github.com/getsentry/sentry-javascript/pull/3544 and https://github.com/getsentry/sentry-react-native/issues/1661#issuecomment-984685325.

@timfish can you confirm that a) we still need this option for Electron and b) moving it from ClientOptions to the SDK init options would work for Electron?

Lms24 avatar May 24 '23 08:05 Lms24

In the Electron SDK we support native crash reporting via two integrations.

  • SentryMinidump (default) - Sends minidumps and full event metadata via the envelope endpoint
  • ElectronMinidump - Sends minidumps to the minidump endpoint via Electrons built in uploader

Some customers prefer ElectronMinidump because on paper, it may be more reliable at reporting since it doesn't rely on JavaScript execution after it has been initialised. We found a way to include minimal event context via form data for the Electron uploader but from some processes (I think renderer + GPU), this is set at startup and cannot be changed later.

Customers wanted a means to attach metadata to these native events and from what I remember, initialScope was added as a means for customers to include this initial context:

import { init, Integrations } from '@sentry/electron/main';

init({
  dsn: "__DSN__",
  integrations: [Integrations.ElectronMinidump()],
  initialScope: {
    user: { id: "12345" },
  }
});

I can't speak for Sentry React-Native, but if I was going to add this feature now, specifically for the Electron SDK, I would make it a constructor parameter of the integration rather than part of ClientOptions:

init({
  dsn: "__DSN__",
  integrations: [Integrations.ElectronMinidump({
    initialScope: {
      user: { id: "12345" },
    }
  })],
});

timfish avatar May 24 '23 10:05 timfish

I can't speak for Sentry React-Native

Seems like they deprecated this option already https://github.com/getsentry/sentry-react-native/issues/1925

but if I was going to add this feature now, specifically for the Electron SDK, I would make it a constructor parameter of the integration rather than part of ClientOptions

Sounds even better 😅

Lms24 avatar May 24 '23 14:05 Lms24

I'll open a tracking issue in the Electron repo for this!

timfish avatar May 24 '23 14:05 timfish

I've just done some testing and it's not quite as simple as moving the initialScope to the ElectronMinidump constructor since this only passes the initial scope used by the integration. initialScope is used primarily by the integration but it also sets the initial starting scope used by the rest of the SDK and removing it will impact other events.

To match the previous behaviour you'd need to do something like this:

const initialScope = { user: { id: "12345" }};

init({
  dsn: "__DSN__",
  integrations: [Integrations.ElectronMinidump({ initialScope })],
});

getCurrentHub().setUser(initialScope.user);

I'm not sure if we should remove this since other SDKs have started implementing it and it's use is widespread since the alternative is quite verbose.

timfish avatar May 24 '23 16:05 timfish