Skip to content
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

ref: Rewrite React Profiler #2677

Merged
merged 19 commits into from Jun 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -10,6 +10,8 @@
- [core] fix: Call `bindClient` when creating new `Hub` to make integrations work automatically (#2665)
- [gatsby] feat: Add @sentry/gatsby package (#2652)
- [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls`
- [react] ref: Refactor Profiler to account for update and render (#2677)
- [tracing] feat: Add ability to get span from activity using `getActivitySpan` (#2677)

## 5.17.0

Expand Down
18 changes: 18 additions & 0 deletions packages/apm/src/integrations/tracing.ts
@@ -1,3 +1,4 @@
// tslint:disable: max-file-line-count
import { Hub } from '@sentry/hub';
import { Event, EventProcessor, Integration, Severity, Span, SpanContext, TransactionContext } from '@sentry/types';
import {
Expand Down Expand Up @@ -817,6 +818,9 @@ export class Tracing implements Integration {

/**
* Removes activity and finishes the span in case there is one
* @param id the id of the activity being removed
* @param spanData span data that can be updated
*
*/
public static popActivity(id: number, spanData?: { [key: string]: any }): void {
// The !id is on purpose to also fail with 0
Expand Down Expand Up @@ -866,6 +870,20 @@ export class Tracing implements Integration {
}, timeout);
}
}

/**
* Get span based on activity id
*/
public static getActivitySpan(id: number): Span | undefined {
if (!id) {
return undefined;
}
const activity = Tracing._activities[id];
if (activity) {
return activity.span;
}
return undefined;
}
}

/**
Expand Down
32 changes: 17 additions & 15 deletions packages/react/src/index.ts
@@ -1,22 +1,24 @@
import { addGlobalEventProcessor, SDK_VERSION } from '@sentry/browser';

function createReactEventProcessor(): void {
addGlobalEventProcessor(event => {
event.sdk = {
...event.sdk,
name: 'sentry.javascript.react',
packages: [
...((event.sdk && event.sdk.packages) || []),
{
name: 'npm:@sentry/react',
version: SDK_VERSION,
},
],
version: SDK_VERSION,
};
if (addGlobalEventProcessor) {
Copy link
Member

Choose a reason for hiding this comment

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

Are there cases where we don't have addGlobalEventProcessor? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

If a user doesn't mock it out in tests, don't want to create unnecessary friction.

addGlobalEventProcessor(event => {
event.sdk = {
...event.sdk,
name: 'sentry.javascript.react',
packages: [
...((event.sdk && event.sdk.packages) || []),
{
name: 'npm:@sentry/react',
version: SDK_VERSION,
},
],
version: SDK_VERSION,
};

return event;
});
return event;
});
}
}

export * from '@sentry/browser';
Expand Down
251 changes: 178 additions & 73 deletions packages/react/src/profiler.tsx
@@ -1,6 +1,6 @@
import { getCurrentHub } from '@sentry/browser';
import { Integration, IntegrationClass } from '@sentry/types';
import { logger } from '@sentry/utils';
import { Integration, IntegrationClass, Span } from '@sentry/types';
import { logger, timestampWithMs } from '@sentry/utils';
import * as hoistNonReactStatic from 'hoist-non-react-statics';
import * as React from 'react';

Expand All @@ -10,91 +10,167 @@ const TRACING_GETTER = ({
id: 'Tracing',
} as any) as IntegrationClass<Integration>;

let globalTracingIntegration: Integration | null = null;
const getTracingIntegration = () => {
if (globalTracingIntegration) {
return globalTracingIntegration;
}

globalTracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
return globalTracingIntegration;
};

/**
*
* Based on implementation from Preact:
* https:github.com/preactjs/preact/blob/9a422017fec6dab287c77c3aef63c7b2fef0c7e1/hooks/src/index.js#L301-L313
*
* Schedule a callback to be invoked after the browser has a chance to paint a new frame.
* Do this by combining requestAnimationFrame (rAF) + setTimeout to invoke a callback after
* the next browser frame.
*
* Also, schedule a timeout in parallel to the the rAF to ensure the callback is invoked
* even if RAF doesn't fire (for example if the browser tab is not visible)
*
* This is what we use to tell if a component activity has finished
*
* Warn if tracing integration not configured. Will only warn once.
*/
function afterNextFrame(callback: Function): void {
let timeout: number | undefined;
let raf: number;

const done = () => {
window.clearTimeout(timeout);
window.cancelAnimationFrame(raf);
window.setTimeout(callback);
};

raf = window.requestAnimationFrame(done);
timeout = window.setTimeout(done, 100);
function warnAboutTracing(name: string): void {
if (globalTracingIntegration === null) {
logger.warn(
`Unable to profile component ${name} due to invalid Tracing Integration. Please make sure the Tracing integration is setup properly.`,
);
}
}

/**
* getInitActivity pushes activity based on React component mount
* pushActivity creates an new react activity.
* Is a no-op if Tracing integration is not valid
* @param name displayName of component that started activity
*/
const getInitActivity = (name: string): number | null => {
const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);

if (tracingIntegration !== null) {
// tslint:disable-next-line:no-unsafe-any
return (tracingIntegration as any).constructor.pushActivity(name, {
description: `<${name}>`,
op: 'react',
});
function pushActivity(name: string, op: string): number | null {
if (globalTracingIntegration === null) {
return null;
}

logger.warn(
`Unable to profile component ${name} due to invalid Tracing Integration. Please make sure to setup the Tracing integration.`,
);
return null;
};
// tslint:disable-next-line:no-unsafe-any
return (globalTracingIntegration as any).constructor.pushActivity(name, {
description: `<${name}>`,
op: `react.${op}`,
});
}

/**
* popActivity removes a React activity.
* Is a no-op if Tracing integration is not valid.
* @param activity id of activity that is being popped
*/
function popActivity(activity: number | null): void {
if (activity === null || globalTracingIntegration === null) {
return;
}

// tslint:disable-next-line:no-unsafe-any
(globalTracingIntegration as any).constructor.popActivity(activity);
}

/**
* Obtain a span given an activity id.
* Is a no-op if Tracing integration is not valid.
* @param activity activity id associated with obtained span
*/
function getActivitySpan(activity: number | null): Span | undefined {
if (activity === null || globalTracingIntegration === null) {
return undefined;
}

// tslint:disable-next-line:no-unsafe-any
return (globalTracingIntegration as any).constructor.getActivitySpan(activity) as Span | undefined;
}

export type ProfilerProps = {
// The name of the component being profiled.
name: string;
// If the Profiler is disabled. False by default. This is useful if you want to disable profilers
// in certain environments.
disabled?: boolean;
// If time component is on page should be displayed as spans. True by default.
hasRenderSpan?: boolean;
// If component updates should be displayed as spans. True by default.
hasUpdateSpan?: boolean;
// props given to component being profiled.
updateProps: { [key: string]: any };
};

/**
* The Profiler component leverages Sentry's Tracing integration to generate
* spans based on component lifecycles.
*/
class Profiler extends React.Component<ProfilerProps> {
public activity: number | null;
// The activity representing how long it takes to mount a component.
public mountActivity: number | null = null;
// The span of the mount activity
public mountSpan: Span | undefined = undefined;
// The span of the render
public renderSpan: Span | undefined = undefined;

public static defaultProps: Partial<ProfilerProps> = {
disabled: false,
hasRenderSpan: true,
hasUpdateSpan: true,
};

public constructor(props: ProfilerProps) {
super(props);
const { name, disabled = false } = this.props;

this.activity = getInitActivity(this.props.name);
if (disabled) {
return;
}

if (getTracingIntegration()) {
this.mountActivity = pushActivity(name, 'mount');
} else {
warnAboutTracing(name);
}
}

// If a component mounted, we can finish the mount activity.
public componentDidMount(): void {
afterNextFrame(this.finishProfile);
}

// Sometimes a component will unmount first, so we make
// sure to also finish the mount activity here.
public componentWillUnmount(): void {
afterNextFrame(this.finishProfile);
this.mountSpan = getActivitySpan(this.mountActivity);
popActivity(this.mountActivity);
this.mountActivity = null;
}

public finishProfile = () => {
if (!this.activity) {
return;
public componentDidUpdate({ updateProps, hasUpdateSpan = true }: ProfilerProps): void {
// Only generate an update span if hasUpdateSpan is true, if there is a valid mountSpan,
// and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive.
// We are just trying to give baseline clues for further investigation.
if (hasUpdateSpan && this.mountSpan && updateProps !== this.props.updateProps) {
// See what props haved changed between the previous props, and the current props. This is
// set as data on the span. We just store the prop keys as the values could be potenially very large.
const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]);
if (changedProps.length > 0) {
// The update span is a point in time span with 0 duration, just signifying that the component
// has been updated.
const now = timestampWithMs();
this.mountSpan.startChild({
data: {
changedProps,
},
description: `<${this.props.name}>`,
endTimestamp: now,
op: `react.update`,
startTimestamp: now,
});
}
}
}

const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
if (tracingIntegration !== null) {
// tslint:disable-next-line:no-unsafe-any
(tracingIntegration as any).constructor.popActivity(this.activity);
this.activity = null;
// If a component is unmounted, we can say it is no longer on the screen.
// This means we can finish the span representing the component render.
public componentWillUnmount(): void {
const { name, hasRenderSpan = true } = this.props;

if (this.mountSpan && hasRenderSpan) {
// If we were able to obtain the spanId of the mount activity, we should set the
// next activity as a child to the component mount activity.
this.mountSpan.startChild({
description: `<${name}>`,
endTimestamp: timestampWithMs(),
op: `react.render`,
startTimestamp: this.mountSpan.endTimestamp,
});
}
};
}

public render(): React.ReactNode {
return this.props.children;
Expand All @@ -103,16 +179,22 @@ class Profiler extends React.Component<ProfilerProps> {

/**
* withProfiler is a higher order component that wraps a
* component in a {@link Profiler} component.
* component in a {@link Profiler} component. It is recommended that
* the higher order component be used over the regular {@link Profiler} component.
*
* @param WrappedComponent component that is wrapped by Profiler
* @param name displayName of component being profiled
* @param options the {@link ProfilerProps} you can pass into the Profiler
*/
function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>, name?: string): React.FC<P> {
const componentDisplayName = name || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
function withProfiler<P extends object>(
WrappedComponent: React.ComponentType<P>,
// We do not want to have `updateProps` given in options, it is instead filled through the HOC.
options?: Pick<Partial<ProfilerProps>, Exclude<keyof ProfilerProps, 'updateProps'>>,
): React.FC<P> {
const componentDisplayName =
(options && options.name) || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;

const Wrapped: React.FC<P> = (props: P) => (
<Profiler name={componentDisplayName}>
<Profiler {...options} name={componentDisplayName} updateProps={props}>
<WrappedComponent {...props} />
</Profiler>
);
Expand All @@ -132,17 +214,40 @@ function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>
* Requires React 16.8 or above.
* @param name displayName of component being profiled
*/
function useProfiler(name: string): void {
const [activity] = React.useState(() => getInitActivity(name));
function useProfiler(
name: string,
options: { disabled?: boolean; hasRenderSpan?: boolean } = {
disabled: false,
hasRenderSpan: true,
},
): void {
const [mountActivity] = React.useState(() => {
if (options && options.disabled) {
return null;
}

if (getTracingIntegration()) {
return pushActivity(name, 'mount');
}

warnAboutTracing(name);
return null;
});

React.useEffect(() => {
afterNextFrame(() => {
const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
if (tracingIntegration !== null) {
// tslint:disable-next-line:no-unsafe-any
(tracingIntegration as any).constructor.popActivity(activity);
const mountSpan = getActivitySpan(mountActivity);
popActivity(mountActivity);

return () => {
if (mountSpan && options.hasRenderSpan) {
mountSpan.startChild({
description: `<${name}>`,
endTimestamp: timestampWithMs(),
op: `react.render`,
startTimestamp: mountSpan.endTimestamp,
});
}
});
};
}, []);
}

Expand Down