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: move tracePropagationTargets to BrowserTracing options #7449

Closed
wants to merge 1 commit into from
Closed

fix: move tracePropagationTargets to BrowserTracing options #7449

wants to merge 1 commit into from

Conversation

ddubrava
Copy link
Contributor

Pre-merge checklist

If you work at Sentry, you're able to merge your own PR without review, but please don't unless there's a good reason.

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs
  • PR was reviewed and approved by a member of the Sentry docs team

Description of changes

'tracePropagationTargets' does not exist in type 'BrowserOptions', so I moved this property to BrowserTracing({...})

Screenshot 2023-07-12 at 15 02 52

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Extra resources

@vercel
Copy link

vercel bot commented Jul 17, 2023

@ddubrava is attempting to deploy a commit to the Sentry Team on Vercel.

A member of the Team first needs to authorize it.

@AbhiPrasad
Copy link
Member

@ddubrava as per 7.57.0 tracePropagationTargets is a top level option. Closing this PR as a result.

See the PR I merged in last week: #7267

Are you seeing this error on the latest SDK version?

@AbhiPrasad AbhiPrasad closed this Jul 17, 2023
@ddubrava
Copy link
Contributor Author

ddubrava commented Jul 17, 2023

@ddubrava as per 7.57.0 tracePropagationTargets is a top level option. Closing this PR as a result.

See the PR I merged in last week: #7267

Are you seeing this error on the latest SDK version?

ah ok, I haven't tried the last version. We are on the same major version, didn't expect breaking changes. By the way, there are a few places in a documentation where tracePropagationTargets are not on the top level. See https://docs.sentry.io/platforms/javascript/guides/capacitor/, https://docs.sentry.io/platforms/javascript/guides/electron/performance/

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jul 17, 2023

ah ok, I haven't tried the last version. We are on the same major version, didn't expect breaking changes

No breaking change! You can still pass in tracePropagationTargets into BrowserTracing and it'll work fine, just marked with a deprecated JSDoc. We just updated the docs to reflect the newest public API.

By the way, there are a few places in a documentation where tracePropagationTargets are not on the top level

These should be electron/capacitor which don't have support for the top level one yet (since they follow different versioning than the other SDKs)

@ddubrava ddubrava deleted the trace-propagation-targets-are-in-wrong-place branch July 17, 2023 19:27
@AbhiPrasad
Copy link
Member

btw thanks for opening a PR, appreciate the contribution and offer to help!!

@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants