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: Deprecate extractTraceParentData from @sentry/core & downstream packages #9158

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Oct 3, 2023

Instead, users should import this from @sentry/utils.

This was also re-exported from all downstream packages (e.g. @sentry/browser, @sentry/node). I don't think this is something users need to import from there...? If you need this, installing @sentry/utils is probably fine?

Else if we think we want to keep exporting this, we can also remove the comment, but then we should just properly move this from utils to core and instead remove the utils export, I'd say.

@mydea mydea requested review from Lms24 and AbhiPrasad October 3, 2023 07:27
@mydea mydea self-assigned this Oct 3, 2023
*/
export { TRACEPARENT_REGEXP, extractTraceparentData } from '@sentry/utils';
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: TRACEPARENT_REGEXP was not actually used/exported any further.

@mydea mydea force-pushed the fn/deprecate-extractTraceParentData branch from a1f0937 to bd3c35e Compare October 3, 2023 07:34
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 65.42 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 55.63 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 30.98 KB (0%)
@sentry/browser - Webpack (gzipped) 21.3 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 61.95 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 29.1 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped) 21.24 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 195.42 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 88.32 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 63.3 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.81 KB (0%)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 65.77 KB (+0.01% 🔺)
@sentry/react - Webpack (gzipped) 21.34 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 82.49 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 48.13 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 16 KB (0%)

@mydea mydea force-pushed the fn/deprecate-extractTraceParentData branch from bd3c35e to 8258922 Compare October 3, 2023 08:20
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.

I guess this is fine because we also have continueTrace coming

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.

I also think this is fine!

@@ -8,6 +8,10 @@ npx @sentry/migr8@latest

This will let you select which updates to run, and automatically update your code. Make sure to still review all code changes!

## Deprecate `extractTraceParentData` export from `@sentry/core` & downstream packages

Instead, import this directly from `@sentry/utils`.
Copy link
Member

Choose a reason for hiding this comment

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

l: I'd probably add a hint here that continueTrace might be a suitable replacement

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense, I'll wait till we merge that and reference this then!

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the sentence below, WDYT?

@mydea mydea force-pushed the fn/deprecate-extractTraceParentData branch from 8258922 to 9bdaf4d Compare October 5, 2023 09:50
Instead, uses should import this from `@sentry/utils`.
@mydea mydea force-pushed the fn/deprecate-extractTraceParentData branch from 9bdaf4d to a018b53 Compare November 14, 2023 10:23
@mydea mydea merged commit f1ed0e9 into develop Nov 14, 2023
@mydea mydea deleted the fn/deprecate-extractTraceParentData branch November 14, 2023 13:47
@@ -70,6 +69,7 @@ export const getActiveTransaction = getActiveTransactionT;
*
* `extractTraceparentData` can be imported from `@sentry/node`, `@sentry/browser`, or your framework SDK
*/
// eslint-disable-next-line deprecation/deprecation
export const extractTraceparentData = extractTraceparentDataT;
Copy link

Choose a reason for hiding this comment

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

@mydea I'm getting an error upgrading from 7.77.0 to 7.81.1

@parcel/validator-typescript: Cannot find module '@sentry/utils' or its 

corresponding type declarations.
  {PROJECT-}/.yarn/cache/@sentry-tracing-npm-7.81.1-37dcfd79a9-02e639e8aa.zip/node_modules/@sentry/tracing/types/index.d.ts:34:60

    33 |  */
  > 34 | port("@sentry/utils").extractTraceparentData;
  >    |      ^^^^^^^^^^^^^^^^ Cannot find module '@sentry/utils' or its corresponding type declarations.
    35 | /**
    36 |  * @deprecated `@sentry/tracing` has been deprecated and will be moved 

It's just a guess but I think the error might come @sentry/utils being a devDependency of @sentry/tracing.

Copy link

@jbreton jbreton Nov 21, 2023

Choose a reason for hiding this comment

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

I compared how sentry is initialized on my side with the documentation and BrowserTracing is imported through @sentry/react, we were importing it from @sentry/tracing directly.

Changing the initialization fixed the problem. This could be considered a bug since @sentry/tracing requirement was documented to be removed in a future major release

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey,

so this should still work when being imported from @sentry/tracing too - can it be that you had some lingering un-updated versions hanging around? That's usually the reason for these kind of things, and also why we are pushing to having everything imported from a single place (e.g. @sentry/react). Probably you had some child dependency of sentry/utils with an older versions where that didn't exist yet!

Copy link

Choose a reason for hiding this comment

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

I've looked at the lock file and all sentry related modules where at 7.81.1. I made sure to start from a fresh clone of the projet and cleaned all caches.

It may be something with yarn, parcel and the way the import made that doesn't work well.

I stopped looking since using the new recommended init works. I just wanted to let you know there might be something here. I wasn't sure if I should fill it as a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for bringing this up! We'll keep an eye on this, if this comes up more maybe something is wrong, but "hopefully" it's some weird thing with versions. 👍

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

4 participants