Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instructions for adding initial scope when using BrowserClient #5345

Closed
yuval-sentry opened this issue Jul 26, 2022 · 9 comments
Closed

Comments

@yuval-sentry
Copy link
Member

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();
@getsentry-release
Copy link
Contributor

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

@ba0708
Copy link

ba0708 commented May 23, 2023

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.

@Lms24
Copy link
Member

Lms24 commented May 24, 2023

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
Copy link
Member

Lms24 commented May 24, 2023

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: getsentry/sentry-javascript#3544 and
getsentry/sentry-react-native#1661 (comment).

@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?

@timfish
Copy link
Collaborator

timfish commented May 24, 2023

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" },
    }
  })],
});

@Lms24
Copy link
Member

Lms24 commented May 24, 2023

I can't speak for Sentry React-Native

Seems like they deprecated this option already getsentry/sentry-react-native#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 😅

@timfish
Copy link
Collaborator

timfish commented May 24, 2023

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

@timfish
Copy link
Collaborator

timfish commented May 24, 2023

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.

@mydea
Copy link
Member

mydea commented Sep 6, 2024

Since we changed a bunch of things around this, I am going to close this. If this is still an issue, please feel free to reopen!

@mydea mydea closed this as completed Sep 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants