Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

ref: Move instrumentation handlers into main plugin class #82

Merged
billyvg merged 4 commits intomainfrom
ref/move-core-sdk-handlers
Jun 3, 2022
Merged

ref: Move instrumentation handlers into main plugin class #82
billyvg merged 4 commits intomainfrom
ref/move-core-sdk-handlers

Conversation

@billyvg
Copy link
Copy Markdown
Member

@billyvg billyvg commented Jun 2, 2022

This moves the core SDK instrumentation handlers into the main plugin class as the typings are more accurate and allows better testing of the handler data transforms as well.

billyvg added 2 commits June 2, 2022 16:27
This moves the core SDK instrumentation handlers into the main plugin class as the typings are more accurate and allows better testing of the handler data transforms as well.
Comment on lines -156 to -160
const hub = Sentry.getCurrentHub();
const scope = hub.getScope();

addInstrumentationListeners(scope, this);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshFerge was there a specific reason to have addInstrumentationListeners so early in this setup fn?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not in particular

Comment on lines +283 to +284
scope.addScopeListener(this.handleCoreListener('scope'));
addInstrumentationHandler('dom', this.handleCoreListener('dom'));
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed observing fetch/xhr because there was a bug where we actually weren't using it -- let's keep it simple and continue to not use it. Although a case where we may want to reduce some duplicated logic is that the SDK ignores capturing requests that get sent to our ingestion endpoint.

Comment on lines +402 to +410
/**
* Keep a list of performance entries that will be sent with a replay
*/
handlePerformanceObserver = (
list: PerformanceObserverEntryList
// observer: PerformanceObserver
) => {
this.performanceEvents = [...this.performanceEvents, ...list.getEntries()];
};
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ordered

handlerData.xhr.__sentry_xhr__.startTimestamp = handlerData.startTimestamp;
}
if (handlerData.endTimestamp) {
this.spans.push({
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring this caught a bug where spans is not a member of the replay plugin class. We were using the requests from window.PerformanceObserver.

@billyvg billyvg changed the title ref/move core sdk handlers ref: Move instrumentation handlers into main plugin class Jun 2, 2022
@billyvg billyvg marked this pull request as ready for review June 2, 2022 20:31
Copy link
Copy Markdown
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good improvement 👍🏼 .what would you think about renaming the getXXX functions to XXXhandler // XXXlistener? (getScope e.g. confused me for a second).

@billyvg billyvg merged commit 231d3d5 into main Jun 3, 2022
@billyvg billyvg deleted the ref/move-core-sdk-handlers branch June 3, 2022 13:49
mydea pushed a commit to getsentry/sentry-javascript that referenced this pull request Nov 23, 2022
…sentry-replay#82)

This moves the core SDK instrumentation handlers into the main plugin class as the typings are more accurate and allows better testing of the handler data transforms as well.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants