-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
…try-javascript into feat/tracing-in-core
size-limit report 📦
|
import { DEFAULT_ENVIRONMENT } from '../constants'; | ||
import type { Hub } from '../hub'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope all good
import type { Hub } from '../hub'; | ||
import { getCurrentHub } from '../hub'; |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Supercedes #7166
Closes #7091
This PR:
core/src/tracing
@sentry/core
now exports the same common types plus:addTracingExtensions
- Required so that@sentry/tracing
can still add the extensions viaaddExtensionMethods
TRACING_DEFAULTS
- This contains the timing defaults that are used across tracingaddExtensionMethods
in@sentry/tracing
due to_autoloadDatabaseIntegrations
@sentry/tracing
to ensure there are no breaking changes to public APIidletransaction.test.ts
imports code from core soIdleTransactionSpanRecorder
doesn't need to be made publicThis PR does not:
Span
,Transaction
, etc from@sentry/core
rather than@sentry/tracing