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

feat(component-annotate): Introduce new plugin to annotate frontend components at build-time #468

Merged
merged 52 commits into from
Jan 31, 2024

Conversation

0Calories
Copy link
Member

@0Calories 0Calories commented Jan 31, 2024

Originally from this PR, splitting it into incremental smaller PRs and also making some adjustments RE: Abhi's suggestions:
#464

This PR in particular adds a new package to the bundler plugins; a Babel plugin that will annotate frontend components at build-time with additional data. A lot of the source code is vendored from FullStory's React annotate plugin, but has been modified and converted to TypeScript. For now, the plugin only works on React, but we can extend it to other frontend frameworks / libraries in the future.

Example of an automatically annotated component in Sentry's frontend:

image

This will allow the browser SDK to pick off component names and send them to Sentry, allowing for some very useful features:

  • React component names in Replays UI
  • React component name search / indexing in Replays
  • React component names in Breadcrumbs
  • React component names in interaction transactions, grouping spans by component name
image

0Calories and others added 30 commits January 29, 2024 14:40
Arrange in calling order, for easier reading
- more explicit `ExcludedComponent` type
- correct `JSXOpeningElement` node type
- extract helper to fetch the component name
- remove unnecessary checks
- remove redundant name finding logic
- add explicit return type
- add types
- add explicit type checking logic
- cleaner iteration pattern
- remove unneeded `as`
…eact-annotate-component-names-plugin-typescript
- fix incorrect logic that was based on the path
- add some clarifying comments
- more robust logic for checking for arrays
- more robust null checks for missing child nodes
- improve old-school iteration patterns
- use correctly type-incorrect property
- add stricter types
Add more robust checking logic
Again, `Array.isArray` doesn't do the right thing here.
Nice try, but the null check needs to be lower
@0Calories 0Calories requested review from lforst and mydea January 31, 2024 16:43
@0Calories 0Calories marked this pull request as ready for review January 31, 2024 17:14
@0Calories
Copy link
Member Author

Still some tweaks I need to do on this PR, but I've opened it for review for the time being

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for splitting it up :)

Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a README with some explanations about how it works?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, writing that right now!

import { transform } from "@babel/core";
import { componentNameAnnotatePlugin as plugin } from "../src/index";

const BananasPizzaAppStandardInput = `import React, { Component } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

can we make these separate JS files we import and assert on?

You should be able to use fs.readFileSync or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to put this in the backlog for now, eventually when we extend this plugin with more functionality, I want to overhaul the test suite. I'm not entirely satisfied with the vendored tests (coverage is good but tests could be cleaner), but I want to focus on shipping this first

packages/component-annotate-plugin/src/index.ts Outdated Show resolved Hide resolved
interface AnnotationOpts {
native?: boolean;
"annotate-fragments"?: boolean;
ignoreComponents?: IgnoredComponent[];
Copy link
Member

Choose a reason for hiding this comment

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

Will we expose any of these to users?

If so we should add doc strings and rename annotate-fragments to annotateFragments

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exposing this for the time being

packages/component-annotate-plugin/src/index.ts Outdated Show resolved Hide resolved
packages/component-annotate-plugin/src/index.ts Outdated Show resolved Hide resolved
[![npm dt](https://img.shields.io/npm/dt/@sentry/component-annotate-plugin.svg)](https://www.npmjs.com/package/@component-annotate-plugin)

A Babel plugin that automatically annotates your output DOM with their respective frontend component names.
This will unlock the capability to search for Replays in Sentry by component name, as well as see component names in breadcrumbs and performance monitoring.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add minimum SDK version needed, and rename this file to be README.md

Also can we add a note that this is in a beta state? See https://github.com/getsentry/sentry-javascript/blob/develop/packages/bun/README.md for an example of a quick beta note.

@@ -0,0 +1,81 @@
{
"name": "@sentry/component-annotate-plugin",
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

This needs to align with the other version of the other packages in this repo.

@0Calories
Copy link
Member Author

Addressed your comments @AbhiPrasad

@0Calories 0Calories merged commit 4d123db into main Jan 31, 2024
16 checks passed
@0Calories 0Calories deleted the feat/react-annotate-component-names-plugin branch January 31, 2024 22:05
0Calories added a commit to getsentry/sentry that referenced this pull request Feb 6, 2024
…#64439)

Replaces the Fullstory Babel annotate plugin with our own version. See
getsentry/sentry-javascript-bundler-plugins#468
for more details

This plugin essentially does the same thing, but will annotate the DOM
with `data-sentry-component` annotations instead, which is what our SDK
looks for when sending component name data to Sentry. This will allow us
to start dogfooding new features like span grouping by component names,
breadcrumbs, and component names in Replays!

Check out the [NPM
Page](https://www.npmjs.com/package/@sentry/component-annotate-plugin)
for more details on the plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants