-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Add span creators to @sentry/tracing package #2736
feat: Add span creators to @sentry/tracing package #2736
Conversation
@@ -96,13 +136,40 @@ export class BrowserTracing implements Integration { | |||
public setupOnce(_: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { | |||
this._getCurrentHub = getCurrentHub; | |||
|
|||
const { routingInstrumentation, startTransactionOnLocationChange, startTransactionOnPageLoad } = this.options; | |||
if (this._emitOptionsWarning) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_emitOptionsWarning
is based on tracingOrigins
only, which we have access to here. Not sure if it's necessary to store it in the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will refactor the logic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this is here because we cannot call it in the constructor, and we want to warn the user if they haven't manually set the _option
themselves.
idleTransaction.registerBeforeFinishCallback(transaction => { | ||
this._metrics.addPerformanceEntires(transaction); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe to unify it with L199 syntax? (not sure about this
context here, would have to verify it. Or we can make L199 accept arrow function instead to make it more "obvious" instead of curried function, your call
idleTransaction.registerBeforeFinishCallback(transaction => { | |
this._metrics.addPerformanceEntires(transaction); | |
}); | |
idleTransaction.registerBeforeFinishCallback(this._metrics.addPerformanceEntires); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this issue here was with the this
context.
I refactored to something like this:
idleTransaction.registerBeforeFinishCallback((transaction, endTimestamp) => {
this._metrics.addPerformanceEntires(transaction);
adjustTransactionDuration(secToMs(maxTransactionDuration), transaction, endTimestamp);
});
|
||
if (global.document) { | ||
// tslint:disable-next-line: prefer-for-of | ||
for (let i = 0; i < document.scripts.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason to ignore prefer-for-of
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied the implementation from what was there before. I assume it is because we want the performance benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remember tbh :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback after first pass. Your turn @HazAT
This will add the `@sentry/ember` package. This will provide an Ember addon that fits into the Ember ecosystem and will in the future offer custom framework specific functionality. Once #2736 and other tracing PR's have landed, we can add Ember specific performance instrumentation.
This will add the `@sentry/ember` package. This will provide an Ember addon that fits into the Ember ecosystem and will in the future offer custom framework specific functionality. Once #2736 and other tracing PR's have landed, we can add Ember specific performance instrumentation.
This will add the `@sentry/ember` package. This will provide an Ember addon that fits into the Ember ecosystem and will in the future offer custom framework specific functionality. Once #2736 and other tracing PR's have landed, we can add Ember specific performance instrumentation.
* feat: Create IdleTransaction class (#2720) * feat: Add BrowserTracing integration (#2723) * feat: Add span creators to @sentry/tracing package (#2736) * ref: Convert React and Vue Tracing to use active transaction (#2741) * build: generate tracing bundles * fix: Remove circular dependency between span and transaction * ref: Add side effects true to tracing * build: Only include @sentry/browser for bundle * fix: Make sure vue and react are backwards compatible with @sentry/apm
Continuing from #2723, This PR adds span creators to
@sentry/tracing
.During the integration setup, we add three new functions to register this functionality.
We also use a separate
MetricsInstrumentation
class (inbrowser/metrics.ts
) to track metrics data. That way we can easily swap it out if we want to expose the option in the future.I didn't do full tests due to the large size of this PR, but I figured it was something we can get back to.