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(browser): Export BrowserTracing from @sentry/browser #7472

Merged
merged 7 commits into from
Mar 17, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Mar 15, 2023

Ref #7342

This PR:

  • Adds @sentry-internal/tracing as a dependency of @sentry/browser
  • Exports BrowserTracing from root of @sentry/browser
  • Ensures that BrowserTracing is excluded from main browser bundle
  • Adds a call to addTracingExtensions from BrowserTracing constructor so this doesn't need to be called separately

@AbhiPrasad
Copy link
Member

I wonder why the integration tests are failing 🤔

@timfish
Copy link
Collaborator Author

timfish commented Mar 16, 2023

wonder why the integration tests are failing

Yep, strange. It looks like only the bundle tests are failing. Need to look into it further!

I guess at some point the tracing bundle will to move to @sentry/browser so that we can delete @sentry/tracing.

@timfish
Copy link
Collaborator Author

timfish commented Mar 16, 2023

Ah, it looks like it's completely to do with test setup. I excluded BrowserTracing from the base @sentry/browser bundles and all the tests currently import @sentry/tracing too. Because I removed the @sentry/tracing import, that bundle no longer gets included in the tests.

I think maybe we need to split the tests into those that require tracing and those that do not.

@timfish
Copy link
Collaborator Author

timfish commented Mar 16, 2023

I've rolled back the changes to the integration tests for now.

I think after this PR, the next steps might be:

  • Move generation of bundle.tracing.*.js to @sentry/browser
  • Fix integration tests to use the correct bundle

@timfish timfish marked this pull request as ready for review March 16, 2023 11:55
@mydea
Copy link
Member

mydea commented Mar 16, 2023

Just leaving a note to #7479 here, need to sync this (whichever is merged first) to make sure all is still good 😅

@@ -29,6 +29,10 @@ export { INTEGRATIONS as Integrations };
export { Replay } from '@sentry/replay';
// __ROLLUP_EXCLUDE_REPLAY_FROM_BUNDLES_END__

// __ROLLUP_EXCLUDE_BROWSER_TRACING_FROM_BUNDLES_BEGIN__
export { BrowserTracing } from '@sentry-internal/tracing';
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, should we export this from src/integrations/index.ts instead? As this is where you'd currently get this from e.g. in CDN bundles. Also where I hooked this in for #7479.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might make more sense to put it in src/integrations/index.ts since it IS an integration. However, the Replay integration is exported just above this and I thought it might be better to keep all the __ROLLUP_EXCLUDE wrapped exports together near the comments describing why they shouldn't be deleted.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good question... So I think either way we have to make sure it is available in both places, as that's where we currently have it. So both of these work as of today for CDN bundles:

new Sentry.BrowserTracing();
new Sentry.Integrations.BrowserTracing();

So TLDR I guess it's fine to export it from the root, but we need to make sure that it is also on Sentry.Integrations in CDN bundles.

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 do this in a follow up given the bundle is already being generated from https://github.com/getsentry/sentry-javascript/blob/develop/packages/tracing/src/index.bundle.base.ts - so I think we should be fine.

@timfish
Copy link
Collaborator Author

timfish commented Mar 16, 2023

Just leaving a note to #7479 here

Ah I see. These shims are empty plugins that warn when you're using a bundle without the type?

Will we either include the shim OR the integration? Are there cases where we'd include neither?

@AbhiPrasad
Copy link
Member

Will we either include the shim OR the integration

Each bundle will either have a shim or the actual integration.

@AbhiPrasad
Copy link
Member

Volta seems to be flaking pretty bad https://github.com/getsentry/sentry-javascript/actions/runs/4438668704/jobs/7790044090?pr=7472

There's a couple of user reports here: volta-cli/volta#1252

@timfish
Copy link
Collaborator Author

timfish commented Mar 16, 2023

Volta seems to be flaking pretty bad

I've seen this at a smaller scale for a couple of months on the Electron repository. It may just be that the node.js download URLs are particularly flakey today!

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