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

feat(vue): Deprecate configuring Vue tracing options anywhere else other than through the vueIntegration's tracingOptions option #14385

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

lforst
Copy link
Member

@lforst lforst commented Nov 20, 2024

Ref: #14265

Currently there are 4 ways of configuring the tracingOptions:

  • flat in Sentry.init()
  • inside tracingOptions in Sentry.init()
  • flat in the vueIntegration() options
  • inside tracingOptions in the vueIntegration() options

Because that is may to many ways of configuration, we decided that the only way should be inside tracingOptions in the vueIntegration() options.

Copy link
Contributor

github-actions bot commented Nov 21, 2024

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.01 KB - -
@sentry/browser - with treeshaking flags 21.72 KB - -
@sentry/browser (incl. Tracing) 35.62 KB - -
@sentry/browser (incl. Tracing, Replay) 72.41 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.7 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 76.71 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 89.18 KB - -
@sentry/browser (incl. Feedback) 39.75 KB - -
@sentry/browser (incl. sendFeedback) 27.64 KB - -
@sentry/browser (incl. FeedbackAsync) 32.43 KB - -
@sentry/react 25.71 KB - -
@sentry/react (incl. Tracing) 38.49 KB - -
@sentry/vue 27.2 KB -0.05% -12 B 🔽
@sentry/vue (incl. Tracing) 37.43 KB -0.03% -9 B 🔽
@sentry/svelte 23.18 KB - -
CDN Bundle 24.18 KB - -
CDN Bundle (incl. Tracing) 37.19 KB - -
CDN Bundle (incl. Tracing, Replay) 71.96 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 77.31 KB - -
CDN Bundle - uncompressed 71.15 KB - -
CDN Bundle (incl. Tracing) - uncompressed 110.52 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 223.32 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 236.54 KB - -
@sentry/nextjs (client) 38.58 KB - -
@sentry/sveltekit (client) 36.13 KB - -
@sentry/node 134.63 KB - -
@sentry/node - without tracing 96.47 KB - -
@sentry/aws-serverless 106.7 KB - -

View base workflow run

@lforst lforst requested review from chargome and mydea November 21, 2024 10:43
integrations: [
Sentry.vueIntegration({
tracingOptions: {
trackComponents: true,
Copy link
Member

Choose a reason for hiding this comment

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

the only thing we could think about elevating to root-level config IMHO is trackComponents, this seems somewhat important enough to possibly warrant this (and also should not be too confusing). So IMHO we could still allow this specifically to also be passed directly to init(), in addition to allowing to pass it to the integration, but no strong feelings.

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 also admittedly don't have hard feelings towards this. I would err towards not allowing it top-level because it opens the door for things tree-shaking in the future, and less ambiguous docs.

@Lms24 @s1gr1d do you have opinions on whether or not we should still allow trackComponents as a top-level Sentry.init() option?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer keeping it integration only because if Sentry.vueIntegration is filtered out, then trackComponents doesn't do anything.

Copy link
Member

@s1gr1d s1gr1d Nov 21, 2024

Choose a reason for hiding this comment

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

I am with Abhi on that, but in Nuxt the vueIntegration is added by the SDK not the user, so I would have to introduce an option in the Nuxt SDK init() which still allows overriding the vueIntegration options.

Something like vueOptions?

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 opened a PR to update the docs. That will maybe highlight a bit better what the config will look like: getsentry/sentry-docs#11917

Copy link
Member

Choose a reason for hiding this comment

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

Component tracking options are mostly defined outside the Sentry.init options across our SDKs. I think it's fine to only have them at integration level in Vue. For Nuxt, we probably want to figure out a way of still letting users change things but I'd argue this is a long-term goal. AFAIK, we don't have docs on component tracking in meta framework SDKs at all at the moment.

Copy link
Member

@s1gr1d s1gr1d Nov 22, 2024

Choose a reason for hiding this comment

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

Ah I see how you envisioned it for Nuxt - users add the vueIntegration themselves. However, I think that would not work here, as the Nuxt SDK already adds the vueIntegration internally to setup the Vue router. This happens here:

sentryClient.addIntegration(vueIntegration({ app: vueApp, attachErrorHandler: false }));

It happens at this place in the Nuxt SDK as we need to pass the vueApp and this is the earliest place to do so (the user cannot pass it).

So we would still have to add something like a vueOptions key for the Nuxt SDK init as we cannot call the integration twice.

Copy link
Member

Choose a reason for hiding this comment

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

So we would still have to add something like a vueOptions key for the Nuxt SDK init as we cannot call the integration twice.

i think that's fine

@lforst lforst merged commit 807883e into develop Nov 25, 2024
36 checks passed
@lforst lforst deleted the lforst-vue-tracing-options branch November 25, 2024 08:57
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.

5 participants