Skip to content

feat(tracing): Move common tracing code to core #7339

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

Merged
merged 17 commits into from
Mar 6, 2023

Conversation

AbhiPrasad
Copy link
Member

Supercedes #7166

Closes #7091

This PR:

  • Moves the common tracing code to core/src/tracing
    • @sentry/core now exports the same common types plus:
      • addTracingExtensions - Required so that @sentry/tracing can still add the extensions via addExtensionMethods
      • TRACING_DEFAULTS - This contains the timing defaults that are used across tracing
  • Leaves addExtensionMethods in @sentry/tracing due to _autoloadDatabaseIntegrations
  • Leaves the tests in @sentry/tracing to ensure there are no breaking changes to public API
    • idletransaction.test.ts imports code from core so IdleTransactionSpanRecorder doesn't need to be made public
  • I had to make some changes to the tests where Jest mocks were getting hoisted

This PR does not:

  • Change any of the downstream SDKs to import Span, Transaction, etc from @sentry/core rather than @sentry/tracing

@AbhiPrasad AbhiPrasad requested review from a team, lforst and Lms24 and removed request for a team March 6, 2023 13:12
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.12 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 62.52 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.76 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 55.51 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.49 KB (-0.02% 🔽)
@sentry/browser - Webpack (minified) 66.98 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.52 KB (+0.01% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.2 KB (+0.18% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.21 KB (+0.44% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.46 KB (+0.5% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 42.87 KB (-0.01% 🔽)
@sentry/replay - Webpack (gzipped + minified) 36.93 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 60.62 KB (+0.22% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 54 KB (-0.01% 🔽)

Comment on lines +14 to +15
import { DEFAULT_ENVIRONMENT } from '../constants';
import type { Hub } from '../hub';
Copy link
Contributor

Choose a reason for hiding this comment

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

l: Do we want to point these to the actual definitions instead of to index?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the imports to use ../hub - does this still apply?

Copy link
Contributor

Choose a reason for hiding this comment

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

nope all good

Comment on lines +3 to +4
import type { Hub } from '../hub';
import { getCurrentHub } from '../hub';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

*/
export function _addTracingExtensions(): void {
export function addTracingExtensions(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

l: Can we mark this as deprecated out of caution. I think we wanna move to dependency injection rather than functions with side-effects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this in a follow up PR since we should make the deprecations the final step after we've moved everything over.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM! (Just saw that Luca also already gave great feedback). Agree on leaving the tests in the tracing package for the time being.

@@ -0,0 +1,72 @@
import { addTracingExtensions, getMainCarrier } from '@sentry/core';
Copy link
Member

Choose a reason for hiding this comment

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

the content of this file will be moved entirely to Node in the future, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it will! For now it should be completely tree-shaken out.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) March 6, 2023 14:16
@AbhiPrasad AbhiPrasad merged commit a306abd into develop Mar 6, 2023
@AbhiPrasad AbhiPrasad deleted the feat/tracing-in-core branch March 6, 2023 14:17
krystofwoldrich added a commit to getsentry/sentry-react-native that referenced this pull request Mar 14, 2023
krystofwoldrich added a commit to getsentry/sentry-react-native that referenced this pull request Mar 14, 2023
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.

Move common @sentry/tracing code to @sentry/core
4 participants