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

Upload source maps during prod build #2523

Merged
merged 5 commits into from
Jul 29, 2024

Conversation

RubenSandwich
Copy link
Collaborator

@RubenSandwich RubenSandwich commented Jul 24, 2024

πŸ”— Relevant links

πŸ—’οΈ What

  • Include the creation and upload of sourcemaps in the postbuild step to unminify RUM errors
  • Source maps are created, uploaded to Datadog, then deleted.
  • Added logic checks so that this script does not run in development mode or when VERCEL_ENV is undefined (as it will be for anyone populating local env vars from Vercel)

🀷 Why

  • Unminified errors will allow us to quickly debug user issues.

πŸ› οΈ How

  • Considered adding rewrite to next.config.js as well as middleware; however, the middleware config matcher already targets _next and static.
  • Adding a rewrite to next.config would mean our bundle size increases

πŸ§ͺ Testing

I added an error here. It will allow the build to complete without issue, but will trigger a RUM error so we can test that the error has unminified code.

  1. Visit this page
  2. Open the console. You should see this error in the console
Screenshot 2024-07-26 at 14 51 04 3. View the unminified error [here](https://app.datadoghq.com/rum/[email protected]:[email protected]:user+service:non-prod.developer.hashicorp.com+version:0db18a6c2a43cd79ee85fc472d7808c976311947)

Additional testing to confirm there are no sourcemaps

  1. Visit this page
  2. Add .map to the end of the filename or visit https://dev-portal-git-rn-featbuild-source-maps-during-deploy-hashicorp.vercel.app/_next/static/chunks/3652-2ac327b032073ad3.js.map?dpl=dpl_BRMFdS4zMDHxDYVjSL4QXgtb7dbX
  3. You should see the 404 page

Copy link

vercel bot commented Jul 24, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
dev-portal βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Jul 26, 2024 9:16pm

Copy link

github-actions bot commented Jul 24, 2024

πŸ“¦ Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action πŸ€–

This PR introduced no changes to the javascript bundle πŸ™Œ

Copy link
Contributor

@zchsh zchsh left a comment

Choose a reason for hiding this comment

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

πŸ™Œ This looks great so far!

πŸ€” Main feedback is whether we should use the non-prod.developer.hashicorp.com service in DataDog for non-prod source maps. I think this might be necessary since it's already set up in our config files!


πŸ§ͺ I'm not as familiar with DataDog, so with the empty PR description I had to poke around a bit to try to test things! Given that in the Vercel logs, line 748 we see this version identifier:

CleanShot 2024-07-25 at 11 37 24@2x

I think we would expect to see source maps uploaded on the developer.hashicorp.com service with version 70fb5226724a8cca166384645f88e26d950e3f26... I couldn't find a version 70fb5226724a8cca166384645f88e26d950e3f26 in that service, but I could find it fornon-prod.developer.hashicorp.com, although without source maps (as far as I can tell):

https://app.datadoghq.com/rum/performance-monitoring?query=%40session.type%3Auser%20%40application.id%3A40a2019f-88ad-4076-ab52-ffd074064eed%20version%3A70fb5226724a8cca166384645f88e26d950e3f26

That being said, I might be going about testing in a weird way here, very open to other approaches!

Thinking out loud, but could we (temporarily) intentionally throw an error on a specific page in this PR? I think then we could use that page to test both that we're uploading source maps (which seems to be confirmed by the Vercel logs in this PR), and that we're uploading them in a way where we can trace them back when an error happens.

scripts/upload-source-maps.ts Outdated Show resolved Hide resolved

try {
const status = execSync(
`DATADOG_API_KEY=${DATADOG_API_KEY} npx @datadog/datadog-ci sourcemaps upload .next/static/ --service=${SERVICE} --release-version=${LATEST_SHA} --minified-path-prefix=${PATH_PREFIX}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed this error in the Vercel build logs:

CleanShot 2024-07-25 at 11 11 34@2x

It seems like "running datadog-ci within the git repo" should in theory be happening, but isn't working as expected πŸ€” It seems like we can manually set the relevant info ourselves... but I am curious why the DataDog CLI doesn't seem to be finding any git remotes... maybe a Vercel thing? 🀷 I'm lost 😬

Copy link
Collaborator

@heatlikeheatwave heatlikeheatwave Jul 25, 2024

Choose a reason for hiding this comment

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

I think this is coming up it's trying to get metadata. From the DD docs.

If you run datadog-ci sourcemaps upload within a Git working directory, Datadog collects repository metadata.

More info here: https://github.com/DataDog/datadog-ci/tree/master/src/commands/sourcemaps#link-errors-with-your-source-code

Copy link
Collaborator

Choose a reason for hiding this comment

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

Update: the error went away when I added the flag --disable-git on my test branch

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, glad the error's gone! πŸŽ‰

In a perfect world, I think if we could resolve the issue without the --disable-git flag, then we might be able to link back to the source repo (screenshot from here):

CleanShot 2024-07-26 at 12 02 52@2x

Feels kinda unnecessary though... the setup you've done to get the above link working feels like it's the really valuable thing here πŸ‘ Finding out how to get DataDog's git stuff to work feels like something we can tack on later as a very optional enhancement if we really want to πŸ‘

Comment on lines 37 to 57
// https://github.com/DataDog/datadog-ci/tree/79c0edce658c54001327e8bbb3f1030e8f4ccc93/src/commands/deployment
// https://app.datadoghq.com/source-code/setup/apm?env=preview&service=developer.hashicorp.com&version=
// try {
// const DD_GIT_COMMIT_SHA = process.env.DD_GIT_COMMIT_SHA
// const DD_GIT_REPOSITORY_URL = process.env.DD_GIT_REPOSITORY_URL

// const gitInfo = `--source-control-provider=git --source-control-repository-url=${DD_GIT_REPOSITORY_URL} --source-control-revision=${DD_GIT_COMMIT_SHA}`

// const deployStatus = execSync(
// `DATADOG_API_KEY=${DATADOG_API_KEY} npx @datadog/datadog-ci deployments create ${gitInfo} --service=${SERVICE} --env=${process.env.VERCEL_ENV}`
// )

// deployStatus
// .toString()
// .split('\n')
// .forEach((line) => {
// console.log(line)
// })

// console.log('Deployment created successfully')
// }
Copy link
Contributor

@zchsh zchsh Jul 25, 2024

Choose a reason for hiding this comment

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

🧹 Seems like we can remove this commented code, unless it's here for testing!

EDIT: just looked at commit history, realized this might be related to the deployment existing in the non-prod service rather than prod πŸ’­

scripts/upload-source-maps.ts Outdated Show resolved Hide resolved
Comment on lines 8 to 16
if (process.env.VERCEL_ENV === 'development') {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ§ͺ I feel like it could be good to have a way to confirm that the basics of source maps are working locally... maybe when VERCEL_ENV is development, we could take on the optional --dry-run parameter and leave everything else the same?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what do you mean by working locally? that we can test uploading them to datadog locally?

Copy link
Contributor

Choose a reason for hiding this comment

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

More just to see that datadog-ci picks up on the *.map files, and that they're removed after the script runs! I believe the --dry-run flag will mean source maps aren't uploaded. Definitely feels optional πŸ‘

@RubenSandwich
Copy link
Collaborator Author

RubenSandwich commented Jul 25, 2024

Handing off this PR to @heatlikeheatwave. As the original PR was more of a pair programming proof of concept to get DD source map working in prod. Heat has a few more things they are going to try. But we now have a working POC. πŸŽ‰

@heatlikeheatwave
Copy link
Collaborator

πŸ™Œ This looks great so far!

πŸ€” Main feedback is whether we should use the non-prod.developer.hashicorp.com service in DataDog for non-prod source maps. I think this might be necessary since it's already set up in our config files!

πŸ§ͺ I'm not as familiar with DataDog, so with the empty PR description I had to poke around a bit to try to test things! Given that in the Vercel logs, line 748 we see this version identifier:

CleanShot 2024-07-25 at 11 37 24@2x

I think we would expect to see source maps uploaded on the developer.hashicorp.com service with version 70fb5226724a8cca166384645f88e26d950e3f26... I couldn't find a version 70fb5226724a8cca166384645f88e26d950e3f26 in that service, but I could find it fornon-prod.developer.hashicorp.com, although without source maps (as far as I can tell):

https://app.datadoghq.com/rum/performance-monitoring?query=%40session.type%3Auser%20%40application.id%3A40a2019f-88ad-4076-ab52-ffd074064eed%20version%3A70fb5226724a8cca166384645f88e26d950e3f26

That being said, I might be going about testing in a weird way here, very open to other approaches!

Thinking out loud, but could we (temporarily) intentionally throw an error on a specific page in this PR? I think then we could use that page to test both that we're uploading source maps (which seems to be confirmed by the Vercel logs in this PR), and that we're uploading them in a way where we can trace them back when an error happens.

@zchsh Tested out your idea (to throw intentional error in preview and track in non-prod service). You can see the unminified error here! πŸŽ‰

scripts/upload-source-maps.ts Outdated Show resolved Hide resolved
scripts/upload-source-maps.ts Outdated Show resolved Hide resolved
scripts/upload-source-maps.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@zchsh zchsh left a comment

Choose a reason for hiding this comment

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

βœ… Testing steps working as expected! Looks great πŸ’―

@heatlikeheatwave heatlikeheatwave merged commit f50afe7 into main Jul 29, 2024
9 checks passed
@heatlikeheatwave heatlikeheatwave deleted the rn/feat/build-source-maps-during-deploy branch July 29, 2024 18:10
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