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(core): Add experimental debug ID based source map upload to Rollup and Vite plugins #192

Merged
merged 16 commits into from
Apr 5, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Mar 30, 2023

Ref #191

This PR adds an experimental option to the bundler plugins that will upload source files with debug IDs to Sentry.

In short, the following steps are executed:

  • Inject a code snippet into every chunk/bundle that will put a debug ID for that particular chunk/bundle onto a global object. Additionally, this code snippet contains a string with a certain format that will allow us to extract that debug ID in later steps.
  • Create a temp folder that will be used to upload files with debug IDs to Sentry.
  • Inject a //# debugId=... comment at the bottom of the chunks and write the chunk to the temp folder.
  • Write a debugId and debug_id field to the corresponding source maps if they are found and write them to the temp folder.
  • Upload the temp folder via Sentry CLI with an option that enables artifact uploads.
  • Delete the temp folder.

@lforst lforst force-pushed the lforst-debug-ids branch from 5a920cc to f9d76ce Compare April 4, 2023 12:52
@lforst lforst changed the title feat(core): Add debug id injection feat(core): Add experimental debug ID based source map upload to Rollup and Vite plugins Apr 5, 2023
@lforst lforst force-pushed the lforst-debug-ids branch from 7002e21 to 4907328 Compare April 5, 2023 11:56
@lforst lforst requested a review from AbhiPrasad April 5, 2023 12:42
@lforst lforst marked this pull request as ready for review April 5, 2023 12:42
@lforst lforst requested a review from mydea April 5, 2023 12:48
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.

This needs tests, especially around the use strict logic and determining source map paths.


// TODO: Find a more elaborate process to generate this. (Maybe with type checking and built-in minification)
const DEBUG_ID_INJECTOR_SNIPPET =
';!function(){try{var e="undefined"!=typeof window?window:"undefined"!=typeof global?global:"undefined"!=typeof self?self:{},n=(new Error).stack;n&&(e._sentryDebugIds=e._sentryDebugIds||{},e._sentryDebugIds[n]="__SENTRY_DEBUG_ID__",e._sentryDebugIdIdentifier="sdbid-__SENTRY_DEBUG_ID__")}catch(e){}}();';
Copy link
Member

Choose a reason for hiding this comment

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

We can build this whenever we need to change it - same approach as we use here: getsentry/sentry#46780. I don't think we need to automate this, we can just manually run terser.

So everytime this snippet changes, we rebuild the minified JS. We can have this in follow up PR.

/**
* Deterministically hashes a string and turns the hash into a uuid.
*/
export function stringToUUID(str: string): string {
Copy link
Member

Choose a reason for hiding this comment

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

l: Can we have a test that enforces this will return a uuid - sanity check mostly that it behaves ok across Node versions.

Copy link
Member Author

@lforst lforst Apr 5, 2023

Choose a reason for hiding this comment

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

Added a small test: 4154b4a

*
* Note: Currently only functional for Vite and Rollup.
*/
debugIdUpload?: {
Copy link
Member

Choose a reason for hiding this comment

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

m: Can we add the default values for include/ignore?

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 am not sure if a default for include makes sense, since you either define paths or you don't. Ignore makes sense though, so I added it: a7cf4e3

});

await writeSourceFilePromise;
await writeSourceMapFilePromise;
Copy link
Member

Choose a reason for hiding this comment

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

l: Why double await here? If we want it to run at the same time, can we just do Promise.allSettled?

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 think we want to throw here in case one of the promises fails so I went with Promise.all: 025e4c1

}

/**
* Looks for a particular string pattern (`sdbid-[debug ID]`) in the bundle
Copy link
Member

Choose a reason for hiding this comment

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

Why did we chose sdbid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just wanted to use something short. It can be changed at any time since nothing depends on this. Do you think another pattern fits better?

Copy link
Member

Choose a reason for hiding this comment

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

I would rather it be sentry, but it's no big deal.

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 would rather it be sentry, but it's no big deal.

No! good point! :) befc007

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.

let's ship and try testing it out!


describe("stringToUUID", () => {
test("should return a deterministic UUID", () => {
expect(stringToUUID("Nothing personnel kid")).toBe("52c7a762-5ddf-49a7-6f16-6874a8cb2512");
Copy link
Member

Choose a reason for hiding this comment

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

❤️


// For now we write both fields until we know what will become the standard - if ever.
map["debug_id"] = debugId;
map["debugId"] = debugId;
Copy link
Member

Choose a reason for hiding this comment

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

image

@lforst lforst merged commit 7129dc6 into main Apr 5, 2023
@lforst lforst deleted the lforst-debug-ids branch April 5, 2023 14:33
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.

2 participants