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

ref(tracing): Set default sampling context data where startTransaction is called #3210

Merged
merged 4 commits into from Jan 29, 2021

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jan 27, 2021

Currently, all default sampling context data, including platform-specific data, is gathered by the tracing package. This requires the use of node-specific and browser-specific stuff in a package meant for both, which has lead to (and continues to lead to) problems.

This PR takes advantage of the ability to pass custom sampling context in order to provide that default data* directly from the source, obviating the need to mix platforms in a problematic way. Specifically, among other things, it removes the use of dynamicRequire, which was crashing in some cases.

* request data in the case of node and location data in the case of browser

Fixes #2971.
Fixes #3060.
Fixes #3171.
Fixes #3172.

@github-actions
Copy link
Contributor

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.21 KB (-0.12% 🔽)
@sentry/browser - Webpack 21.09 KB (-0.02% 🔽)
@sentry/react - Webpack 21.11 KB (-0.03% 🔽)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 26.95 KB (-1.65% 🔽)

Copy link
Contributor

@kamilogorek kamilogorek 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, although this startIdleTransaction signature hurts me inside a little.

const idleTransaction = startIdleTransaction(hub, finalContext, idleTimeout, true);
const { location } = getGlobalObject() as WindowOrWorkerGlobalScope & { location: Location };

const idleTransaction = startIdleTransaction(
Copy link
Contributor

Choose a reason for hiding this comment

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

5 arguments 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

You're not wrong. It's one of my goals for v7 to make the entire IdleTransaction business a lot simpler.

@lobsterkatie lobsterkatie merged commit 230a6bd into master Jan 29, 2021
@lobsterkatie lobsterkatie deleted the kmclb-fix-tracessampler-dynamic-import branch January 29, 2021 22:48
This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants