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(gatsby): Upload source maps automatically when sentry-cli is configured #4109

Merged
merged 5 commits into from
Nov 8, 2021

Conversation

iker-barriocanal
Copy link
Member

Follow-up to #4064.

When sentry-cli is configured, source maps are automatically uploaded without requiring additional configuration from the user. When it's not, they aren't uploaded (doesn't break builds). The user can also define a custom configuration (in gatsby-node.js), which is higher priority than the one the SDK defines.

@iker-barriocanal iker-barriocanal requested a review from a team November 2, 2021 10:17
@iker-barriocanal iker-barriocanal self-assigned this Nov 2, 2021
@iker-barriocanal iker-barriocanal requested review from sl0thentr0py and smeubank and removed request for a team November 2, 2021 10:17
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 22.45 KB (+0.01% 🔺)
@sentry/browser - Webpack 23.35 KB (0%)
@sentry/react - Webpack 23.38 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 29.89 KB (+0.01% 🔺)

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.

Can we make sure we have a docs PR ready alongside this?

@iker-barriocanal
Copy link
Member Author

@AbhiPrasad I'm working on them. The previous approach should still work the same way it does now, so I don't think docs are a blocker to land this PR.

packages/gatsby/gatsby-node.js Outdated Show resolved Hide resolved
errorHandler(err, invokeErr) {
const { message } = err;
if (message.includes('organization slug is required') || message.includes('project slug is required')) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Does this still log the error if we do an early return?

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't

Copy link
Member

Choose a reason for hiding this comment

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

@iker-barriocanal Feel like we need a way to tell users about misconfigured sentry-cli without it being blocking. Can we take advantage of the debug flag somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean exactly? Telling users when they haven't correctly configured sentry-cli? If this is the case:

  • Enabling debug will generate too much noise, and they can always customize this by setting SENTRY_LOG_LEVEL env var, see configuration values.
  • The code to check whether the file and its values are correct would be too complex and too hard to maintain for the value it provides. Are we planning to check if a sentry-cli config file exists? Or, checking the format is correct? Or, checking if the values are correct (need to ask Sentry whether the org exists)?

Considering sentry-cli has proper docs, failing doesn't have major consequences (not uploading source maps for a misconfiguration, which they can read the docs of, or try to have their own config, can retry as many times as they wish at no cost (not a production issue)...), and we can update Gatsby docs if the process is clear enough, I don't think it's worth it to provide additional checks. I'd rather remove this UX improvement over adding it and potentially provide a worse UX.

Note that I might have misunderstood what you meant.

Copy link
Member

Choose a reason for hiding this comment

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

I just wanted to avoid situations where someone forgets to set an org slug, and doesn't understand why source maps uploading isn't working for gatsby (since the error is being swallowed).

I think we should add to docs how user's can define their own custom config with the webpack plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a good point, but I don't think it's worth covering it in this context. As for the docs, we link to the Sentry Webpack docs here; although these sections could be improved, is there anything specific you have in mind?

@iker-barriocanal iker-barriocanal enabled auto-merge (squash) November 8, 2021 09:40
@iker-barriocanal iker-barriocanal merged commit 6b278f1 into master Nov 8, 2021
@iker-barriocanal iker-barriocanal deleted the iker/ref/gatsby-webpack-plugin branch November 8, 2021 09:44
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.

None yet

3 participants