Improve SDK API

[yordis]

Following the request from @AbhiPrasad getsentry/sentry-fullstory#60 (comment)

Related to: getsentry/sentry-fullstory#60

Context

What would be the easiest way we can extract most of the code from this package that would allow people to create sentry-segment or sentry-fullstory from scratch?

Right now they are multiple things happening around the Fullstory itself (if you think about it), that some people (like me) feel the need to extend this one because of fear of maintaining the other code and things could break and requires them to be on top of Sentry SDK (except that Sentry owns these stuff right now).

So the simpler we make it for others to take advantage of such boilerplate code, the easier is to justify to them that they can put a tiny JS in their codebase and own it.

Observation

Although once you read the codebase it makes sense the following https://github.com/getsentry/sentry-fullstory having files like utils or some part of the plugin feels like a sign that the SDK is lacking some helper functions to accomplish common things.

For example,

Get the project ID from a Sentry DSN: https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/util.ts#L3-L9

Which is used in another function: https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/SentryFullStory.ts#L36-L56

That would even remove the need for such a function since "getting the Sentry URL" feels like a common thing to do if you are interested in sending data to other places that correlate to Sentry errors somehow such as analytics events.

Or doing things like https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/SentryFullStory.ts#L59-L60

"// Run the integration ONLY when it was installed on the current Hub AND isn't a transaction"

I still don't know what that means precisely but it seems something important, so a combination of documentation and SDK API should help with this use case (it possible exists, so take it with a grain of salt)

Finally,

Get the message and name properties from the original exception:
https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/util.ts#L15-L27

That sounds like something that people would be interested in if they want to send some information about the error to another system, so definitely really handy for systems that are trying to integrate with Sentry.

[AbhiPrasad]

Hey thanks for writing in! We're going to take a couple days to discuss this internally before responding - will get back to you as soon as we can. Appreciate all the help + detail with the ticket!

[github-actions]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[yordis]

@AbhiPrasad 😭 😭 😭 😭 😭 😭 😭 😭

[chadwhitacre]

Sorry @yordis, fell through the cracks over the holidays ... reopening! 🐭

[AbhiPrasad]

Hey @yordis apologies for the delay, the maintainers went on winter vacations and most just got back today. Appreciate the patience while we catch up on things - we really do appreciate your help!

What would be the easiest way we can extract most of the code from this package that would allow people to create sentry-segment or sentry-fullstory from scratch?

I think our goal is that sdk integrations are the way to implement behaviour like this. Integrations look like this:

/** Integration interface */
export interface Integration {
  /**
   * Returns {@link IntegrationClass.id}
   */
  name: string;

  /**
   * Sets the integration up only once.
   * This takes no options on purpose, options should be passed in the constructor
   */
  setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void;
}

setupOnce is called by the Sentry client to set up the integration, and they are given an event processor (the ability to mutate events to add/remove things) and the ability to grab the current hub (so that they can grab options, call methods like captureException on the hub).

This is a relatively lightweight class, and users are free to construct their own integrations and implement custom logic accordingly. Unfortunately, the documentation is not up to par, that's something we need to work on.

Which is used in another function: https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/SentryFullStory.ts#L36-L56

This can essentially become:

const getSentryUrl = () => {
  try {
    // https://github.com/getsentry/sentry-javascript/blob/e5ede4c5f9897728b70aa77eea74b49ad82faa03/packages/core/src/baseclient.ts#L196
    const dsnComponents = Sentry.getCurrentHub().getClient()?.getDsn();
    if (!dsnComponents) {
      console.error('No DSN');
      return 'Could not retrieve url';
    }
    if (!hint) {
      console.error('No event hint');
      return 'Could not retrieve url';
    }
    const projectId = dsnComponents.projectId
    return `${this.baseSentryUrl}/organizations/${this.sentryOrg}/issues/?project=${projectId}&query=${hint.event_id}`;
  } catch (err) {
    console.error('Error retrieving project ID from DSN', err);
    //TODO: Could put link to a help here
    return 'Could not retrieve url';
  }
};

We also have the API and Dsn classes in Sentry that help abstract away the logic of parsing dsns and creating sentry URLs (well in 6.16.0 and below they were classes, we recently converted them to functions + objects to help save on bundle size).

It's important to note though that we will not introduce logic like so into the SDK as it's not worth the maintenance effort - but we are open to changing our mind.

return `${this.baseSentryUrl}/organizations/${this.sentryOrg}/issues/?project=${projectId}&query=${hint.event_id}`;

Or doing things like https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/SentryFullStory.ts#L59-L60

You're right that this is a case that our docs need to be better - but the behaviour is fully intended. Event processors are global in the SDK (an unfortunate side effect of the current implementation), so we need to make sure that the event processor only runs on hubs where the integration is installed. This is as a user can set up multiple Sentry hubs on the same page.

Get the message and name properties from the original exception:
https://github.com/getsentry/sentry-fullstory/blob/a5c11c837e3b1a7fe1be244598b227b542eec567/src/util.ts#L15-L27

I don't think we'll ever introduce similar helpers in the SDK.

To wrap up - you pointed out some excellent observations on areas we need to document better. We are currently focusing on improving our bundle size so we won't get time to work on this right now, but as we move onto to focusing on the next major version, we'll make sure to prioritize this.