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

fix incorrect sampled type on Transaction #11115

Merged
merged 2 commits into from Mar 18, 2024
Merged

Conversation

quisido
Copy link
Contributor

@quisido quisido commented Mar 15, 2024

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@sentry/types does not align with @sentry/core, and it causes build errors when exactOptionalPropertyTypes is true.

Example error below:

.../Yarn/Berry/cache/@sentry-core-npm-7.107.0-d7aa1d2661-10c0.zip/node_modules/@sentry/core/types/tracing/transaction.d.ts:5:22 - error TS2420: Class 'import(".../Yarn/Berry/cache/@sentry-core-npm-7.107.0-d7aa1d2661-10c0.zip/node_modules/@sentry/core/types/tracing/transaction").Transaction' incorrectly implements interface 'import(".../Yarn/Berry/cache/@sentry-types-npm-7.107.0-a994f84978-10c0.zip/node_modules/@sentry/types/types/transaction").Transaction'.
  Types of property 'sampled' are incompatible.
    Type 'boolean | undefined' is not assignable to type 'boolean'.
      Type 'undefined' is not assignable to type 'boolean'.

5 export declare class Transaction extends SpanClass implements TransactionInterface {
                       ~~~~~~~~~~~


Found 1 error in .../Yarn/Berry/cache/@sentry-core-npm-7.107.0-d7aa1d2661-10c0.zip/node_modules/@sentry/core/types/tracing/transaction.d.ts:5

This change fixes this.

@Lms24
Copy link
Member

Lms24 commented Mar 15, 2024

Hey @quisido thanks for opening this PR! Which version are you experiencing this problem on?

Generally, we're going to remove sampled entirely from the SDK so merging the PR into our v8 (develop) branch won't make much sense. We could merge it into v7 though if this is currently causing issues for you on a v7 version of the SDK.

@quisido
Copy link
Contributor Author

quisido commented Mar 15, 2024

Hey @quisido thanks for opening this PR! Which version are you experiencing this problem on?

Generally, we're going to remove sampled entirely from the SDK so merging the PR into our v8 (develop) branch won't make much sense. We could merge it into v7 though if this is currently causing issues for you on a v7 version of the SDK.

Sorry, yes, v7.

@quisido
Copy link
Contributor Author

quisido commented Mar 16, 2024

@Lms24 I've opened a PR for v7 at #11146, but fwiw this bug would still need to be fixed on v8 via this PR, as upgrading to v8 would re-introduce the bug otherwise.

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 see, thx for pointing out that this is still a problem in v8 (at least at the moment). I missed that Transaction now inherits ultimately also from SpanContext where we defined the explicit undefined.

As I said, medium-term, we'll remove Transaction so this won't be an issue anymore but for now, let's ensure we fix this. Thanks!

@Lms24 Lms24 enabled auto-merge (squash) March 18, 2024 11:58
@Lms24 Lms24 merged commit ffdc8aa into getsentry:develop Mar 18, 2024
66 checks passed
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…11115)

`@sentry/types` does not align with `@sentry/core`, and it causes build
errors when `exactOptionalPropertyTypes` is `true`.

This patch fixes the TypeScript error by explicitly adding `undefined` to the `sampled` flag of `Transaction`.
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

2 participants