Replies: 8 comments
-
|
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! |
Beta Was this translation helpful? Give feedback.
-
|
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 "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Beta Was this translation helpful? Give feedback.
-
|
@AbhiPrasad 😭 😭 😭 😭 😭 😭 😭 😭 |
Beta Was this translation helpful? Give feedback.
-
|
Sorry @yordis, fell through the cracks over the holidays ... reopening! 🐭 |
Beta Was this translation helpful? Give feedback.
-
|
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!
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;
}
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.
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 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.
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.
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. |
Beta Was this translation helpful? Give feedback.
-
|
Thank you so much for the updates, really excited to see where you are heading. The details on how it could be accomplished are where I have some thoughts, so Let me a fork, and see if some thoughts I have are helpful since I want to be closer to what it would happen as possible. Hopefully before you go GA with v6 |
Beta Was this translation helpful? Give feedback.
-
|
Working on the codebase in a PoC #4381 ... I will strongly recommend adding A simple Being said, that is just my opinion as an outsider ... obviously, at some point you get used to it and it is not that important 😄 |
Beta Was this translation helpful? Give feedback.
-
Yeah we should probably do this, unfortunately, it's a breaking change so we can't do it until the major. I've added it to the major release plan: #4240 |
Beta Was this translation helpful? Give feedback.
-
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-segmentorsentry-fullstoryfrom 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
utilsor 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
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.
Beta Was this translation helpful? Give feedback.
All reactions