getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.97k stars 1.57k forks source link

Tracking issue for @sentry/* SDKs #1281

Closed jan-auer closed 6 years ago

jan-auer commented 6 years ago

Next Generation SDK Discussion

As you might have noticed, we have started pushing out pre-release versions for our next line of JavaScript SDKs. You can identify them by the @sentry/* namespace on NPM. The goal in this new lineup is to provide a more convenient interface and improved consistency between various JavaScript environments.

The SDKs are developed in mono-repository located in packages.

Updated Interface

import { init, captureMessage } from '@sentry/browser';

init({
  dsn: '__DSN__',
  // ...
});

captureMessage('Hello, world!');

Library minimal

A new feature of this SDK lineup is the minimal package. It allows library authors add support for a Sentry SDK without having to bundle the entire SDK or being dependent on a specific platform. If the library is included and a Sentry SDK is present, it will automagically work:

import * as Sentry from '@sentry/minimal';

// Add a breadcrumb for future events
Sentry.addBreadcrumb({
  message: 'My Breadcrumb',
  // ...
});

// Capture exceptions, messages or manual events
Sentry.captureMessage('Hello, world!');
Sentry.captureException(new Error('Good bye'));
Sentry.captureEvent({
  message: 'Manual',
  stacktrace: [
    // ...
  ],
});

We hope to see library authors adopt this feature in the future to facilitate better error tracking across the ecosystem.

Scope concept

We introduced a new concept we called Scope. A Scope holds an isolated state of breadcrumbs, context and other metadata. raven-node and raven-js had a similar feature, ambiguously called "context". There always is a "default" Scope which handles all the stuff as we did before you have to do nothing but we also support pushing new a Scope if you ever want to have isolated context information lets say for example, each request that comes in in a node application should have it's own Scope.

To add extra, tags or user to your event you have to call:

import * as Sentry from '@sentry/browser';

// Set user information, as well as tags and further extras
Sentry.configureScope(scope => {
  scope.setExtra('battery', 0.7);
  scope.setTag('user_mode', 'admin');
  scope.setUser({ id: '4711' });
  // scope.clear();
});

Sentry.captureMessage("Hello World!"); // This event contains all scope information from the global scope

If you want to have isolated information for only on specific event you can to:

import * as Sentry from '@sentry/browser';

Sentry.getDefaultHub().withScope(() => {
   // We are here in an isolated new Scope, we inherited all the stuff from the parent 
   // but after this functions returns the Scope is popped and removed

  Sentry.configureScope(scope => {
    scope.setExtra('battery', 0.9); // We overwrite battery extra
  });

  Sentry.captureMessage("Hello World!"); // This will contain all scope info from before + battery is overwritten just for this message
});
HazAT commented 6 years ago

https://github.com/getsentry/sentry-cordova and https://github.com/getsentry/sentry-electron are both using our new SDKs internally. We already got a lot of feedback and the public API and in general should be pretty safe now.

captbaritone commented 6 years ago

Hey! I maintain raven-for-redux, and I'm starting to look at building a version of that library to support the new SDK. I'm looking forward to having an API that can work consistently for Node (server side rendering) React Native and the Browser. Previous incompatibility has been a source of confusion for my users.

I think the only thing I see missing is a way to lazily configure the scope. With raven-for-redux we let users attach their entire app state as context. Since the entire state may be too large, or may need to redacted in some way, we allow the user to specify a state transform function which we will run before the report is sent. This is too expensive to do on every state update, so previously we relied on Raven. setDataCallback to run the transform lazily (only when it was time to actually submit a message/error).

If you do support registering a callback like this, it would also be good to support unregistering the callback, since in a server side rendering context we may want to (lazily) set this scope once per request.

An API method like Sentry.configureScope, except lazy, could probably get the job done:

import createStore from 'redux';

const store = createStore(/* ... */);

const unsubscribe = Sentry.configureScopeOnError(scope => {
  scope.setExtra('state', transformState(store.getState()));
  scope.setUser(deriveUserFromState(store.getState()));
});

Another option would be to expose some way to register a generic onError callback which will be called before the scope data is collected. With this, we could build our own configureScopeOnError:

import createStore from 'redux';

const store = createStore(/* ... */);

const unsubscribe = Sentry.onError(()=> {
  Sentry.configureScope((scope) => {
    scope.setExtra('state', transformState(store.getState()));
    scope.setUser(deriveUserFromState(store.getState()));
  });
});

These are just the ideas the jump to mind immediately. Maybe you see another way to do it, or a clearer API?

HazAT commented 6 years ago

@captbaritone Thank you for taking the time to look at our new SDK. I hope that I understood the question correctly if so I think I will have a good solution for this.

For the new SDKs we came up with something we call integrations. An Integration basically can be anything that hooks into the flow of the client sending an event, even emitting event.

So I think what you want to do is build an integration. See this integration for example: https://github.com/getsentry/sentry-javascript/blob/master/packages/browser/src/integrations/sdkinformation.ts

We introduced eventProcessor which is basically a "stackable" dataCallback. They will be executed in the order in which they where installed.

so you could do something like this:

public install(): void {
    getCurrentHub().addEventProcessor(async (event: SentryEvent) => {
        // mutate event before sending in here
        // return mutated event OR null of you want to discard it
    });
}

This integration (in node) also configures a scope: https://github.com/getsentry/sentry-javascript/blob/master/packages/node/src/integrations/onunhandledrejection.ts

And the usage of you integration would also be super easy:

import * as Sentry from '@sentry/browser';
import { ReduxIntegration } from "raven-for-redux";

init({
  dsn: '__DSN__',
  integrations: (integrations) => {
     // integrations include all default integrations we provide
     // you could do this:
     integrations.push(new ReduxIntegration(/* you can pass anything here e.g. store*/));
     return integrations;
  }
});

I hope you get the idea, integrations are very powerful and make our SDKs super extensible in a logical understandable way. Please let me know if this helps.

kamilogorek commented 6 years ago

We. Are. Doneee. :shipit:

SimonSchick commented 6 years ago

Hey, I'm not sure if this is the right place to ask but I'm currently playing with the new SDK and have some questions:

1: How do we set a custom log level when logging exceptions via .captureException? Previously this was possible and sometimes we log errors as warning if it's not a critical issue, this seems to be no longer possible, perhaps allow us to set the log level on scope?

2: How exactly does scope work, the docs are not clear on this at all?

I assume this:

Sentry.configureScope(scope => {
  scope.setUser({ id: 'superuser' });
  Sentry.captureException(err);
});

2.1: Do I need to need to explictly call scope.clear() to avoid the configuration to leak into other captured events? 2.1.1: If scope settings persist, how do I define global scope settings? 2.2: Is configureScope called synchronously?

SimonSchick commented 6 years ago

Furthermore, how we do we set global tags now?

SimonSchick commented 6 years ago

So, I played around with this for a bit, calling clear after using captureException doesn't attach ANY tags/user/etc. whatsoever, I don't understand how you log a single exception while attaching user specific data in the new node SDK at this point.

HazAT commented 6 years ago

@SimonSchick Thanks for your feedback, also I like to mention that we are working the docs as we speak, it's planned that we ship the new docs today which should clear up everything but nevertheless I'd like to give you answers to your questions directly.

1.: We dropped the "arbitrary" second parameter in captureException since it was unclear for many users what to pass in there, level worked, but not all properties of an event did, so we decided to drop it. captureMessage on the other hand now has a second parameter which only takes the level, which is now the preferred use-case for the scenario you described. There is another way to set the level, even for a captureException call but I'd like to know if for your specific case, captureMessage is sufficient. (you can enable attachStacktrace you will get the stacktrace also with captureMessage).

2.: Like I mentioned, scope docs are in the works, but you can think of it like a stack which is holding context information. The example you provided is almost correct and works the way as you expect.

2.1.: For this case (while not documented here yet), we introduced a way to push and pop a scope. Every SDK after init creates the "root" scope, but if you want to send specific context information with an event you should push a new scope like this:

Sentry.getCurrentHub().pushScope();
Sentry.configureScope(scope => {
  scope.setUser({ id: 'superuser' });
});
Sentry.captureException(err);
Sentry.getCurrentHub().pushScope();

In this code example we push a new scope (which is a clone of the current scope) onto the stack, all capture calls will have this context information set in configureScope until you call pop again.

2.1.1: A scope lives in memory and is not persisted to disk or something like that, defining global context can be done in the root scope.

2.2.: Yes, configureScope is synchronous.


You can set a global tag simply with

Sentry.configureScope(scope => {
  scope.setTag(bla, 'blub');
});

Calling clear in a scope, resets everything in there, tags, extra, breadcrumbs fingerprints, clear is helpful in scenarios where you push a new scope, want a fresh and empty state, call clear in the new pushed scope and after popping it, you will still have the global scope keeping all your stuff set before.

This was a bit of a long comment but I hope this makes stuff more clear.

HazAT commented 6 years ago

@SimonSchick We are disscussing if we should add something like:

captureException(exception, scope => {
  scope.setTag('bla', 'blub');
  scope.setLevel('warning');
});

will keep you posted.

HazAT commented 6 years ago

Update: It's already in master and the next rc will contain it. We exposed withScope and added level on the scope, so the final code looks like this:


Sentry.withScope(scope => {
  scope.setTag(bla, 'blub').setLevel('warning');
  Sentry.captureException(new Error('bla'));
});

This example will have the tag and level only set for this exception. If you want to set something globally, you still need to use configureScope.

SimonSchick commented 6 years ago

Thanks for the replies, can you explain the difference between .withScope vs. .configureScope I assume withScope auto pushes and pops?

Furthermore, I worked around the limitation by instantiating a NodeClient and Scope manually and then clone the scope for each event since that seems to be public API as well.

The code looks roughly like this:

    const client = new NodeClient({
      dsn,
      release: dInfo.getCommit(),
    });
    client.install();

    const scope = new Scope();
    scope.setTag('buildId', dInfo.getBuild());
    log.addStream({
      level: cfg.remote.level,
      stream: new SentryWriteStream(client, scope), // as `baseScope`
    });
...
// insode of the event logging setup
    const scope = Scope.clone(this.baseScope);
    if (record.user) {
      scope.setUser({
        email: record.user.email,
        id: record.user.id.toString(),
        username: record.user.name,
      });
    }
    scope.setTag('hostname', record.processInfo.hostname.toString());
    scope.setTag('pid', record.processInfo.pid.toString());
    for (const [key, value] of Object.entries(record.tags)) {
      scope.setTag(key, value.toString());
    }

    for (const [key, value] of Object.entries(record.fields)) {
      scope.setExtra(key, value);
    }
...
client.captureException(err, undefined, this.setupScope(record))

Looking forward to not having to do that :)

SimonSchick commented 6 years ago

Again, thanks for the swift reply, I understand this is an RC so I really appreciate you guys reacting to quickly!

HazAT commented 6 years ago

@SimonSchick Correct, withScope pushes and pops for you, while configureScope only gets you the current scope to change. While we generally support creating an custom client, we discourage this API since there are some drawbacks that come with it, e.g.: no integrations by default, we guide people to use init. So you really need to know what you are doing. Also, we'd like to so it not as part of our "public API" contract. What you did in your example is basically what we do with withScope.

SimonSchick commented 6 years ago

While we generally support creating an custom client, we discourage this API since there are some drawbacks that come with it, e.g.: no integrations by default, we guide people to use init.

Yea that's what I thought, I will use the new API once the package is released :)

kblcuk commented 5 years ago

Hi there!

I'm currently migrating our codebase to use new sdk, and trying to figure out what would be good way to replicated parseUser behaviour from legacy client?

While withScope probably works for manual error capturing, I'm wondering how could I achieve the same (passing extra user data parsed from request that ended up with error) for a global error handler.

So far I ended up with having a beforeSend which tries to extract and decode token information from event.request, and then (in case of success) just appends it to event before returning it. However, I'm not entirely sure if it's the best way to implement this, hence asking here.

Or should I open separate issue altogether?

Thanks in advance!

SimonSchick commented 5 years ago

You usually want to set the user on the global scope upon login (single page app) or set it on page load.

kblcuk commented 5 years ago

@SimonSchick I'm talking about parseUser method from legacy node client, and migrating to @sentry/node, forgot to mention that. For the front-end, obviously, what you said.

SimonSchick commented 5 years ago

Can you tell us a little bit more about your use-case?

If you have a system with middlewares it should be easy enough to just use scope.setUser in the respective error handler.

kamilogorek commented 5 years ago

@kblcuk you have an access to the request itself in all middlewares, thus you can parse user manually in any way you need

// express example

function yourCustomUserParser (req, res, next) {
  Sentry.configureScope((scope) => scope.setUser({
    username: req.specialUserField.username,
    id: req.specialUserField.getId()
  }));

  next();
}

// order matters
app.use(Sentry.Handlers.requestHandler());
app.use(yourCustomUserParser);

app.get('/', function mainHandler(req, res) {
  throw new Error('Broke!');
});

app.use(Sentry.Handlers.errorHandler());
app.listen(3000);
SimonSchick commented 5 years ago

@kamilogorek isn't Sentry.configureScope inside of a middleware extremely prone to race conditions?

kamilogorek commented 5 years ago

@SimonSchick not when used alongside requestHandler, as it's creating a Sentry Hub on the new domain for each request. And then getCurrentHub, which is used internally by configureScope is detecting it: https://github.com/getsentry/sentry-javascript/blob/d9bb595ca6b450332ead51b750c37f63e69860ba/packages/hub/src/hub.ts#L326-L360

The main part: https://github.com/getsentry/sentry-javascript/blob/d9bb595ca6b450332ead51b750c37f63e69860ba/packages/hub/src/hub.ts#L354-L358

brianmock commented 5 years ago

I'm attempting to use configureScope to set user context during a session. According to the doc, it should be as simple as

Sentry.configureScope((scope) => {
  scope.setUser({"email": "john.doe@example.com"});
});

This didn't work for me. I also attempted using slightly modified code posted by @HazAT above:

Sentry.getCurrentHub().pushScope();
Sentry.configureScope(scope => {
  scope.setUser({ id: 'superuser' });
});
Sentry.getCurrentHub().pushScope();

This also did not work for me. Do I need to configureScope every time captureException is called? I thought I could just set it once and it would be set for the entirety of the user's session unless otherwise modified.

I inspected the Sentry module itself and saw there is a setUser function I can call directly:

Sentry.setUser({
  email: user.email,
  id: user.id,
  type: user.current_profile.type,
  firstName: user.current_profile.first_name,
  lastName: user.current_profile.last_name,
});

This one did work for me and I am seeing user info attached to issues in the event log. Could someone help explain what I might be doing wrong when trying to use the recommended configuration?

HazAT commented 5 years ago

@brianmock This is really strange, all three of them should work and they do here: https://codesandbox.io/s/sentrybrowser-simple-capturemessage-r8xfn

Not sure what's going on. Can you confirm it works in the example I posted?

brianmock commented 5 years ago

@HazAT works in the example posted and I figured out my issue, it was my fault. There was a race condition between the exception and setting the user context so sometimes user info was there and sometimes it was not and I had happened to see it work with setUser instead of configureScope.

Thank you for your time tho.

willisplummer commented 3 years ago

@brianmock what did you do to address the race condition? i'm running into similar issues but it doesn't seem like setUser returns a promise or something to enable awaiting its completion before moving on

ashwinkhode commented 2 years ago

Can we set the user scope under useEffect in _app.js in nextjs or is there a better way to do it?