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 performance monitoring #2571

Merged
merged 5 commits into from
May 12, 2020
Merged

feat: Vue performance monitoring #2571

merged 5 commits into from
May 12, 2020

Conversation

kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented May 6, 2020

Docs Draft: https://gist.github.com/kamilogorek/1279bd2d39ba07f2ddd69cc684449949

Example usage (minimal):

import * as Sentry from "@sentry/browser";
import { Vue as VueIntegration } from "@sentry/integrations";
import { Integrations } from "@sentry/apm";

import Vue from "vue";

Sentry.init({
  dsn: "https://lol@nope.ingest.sentry.io/1337",
  integrations: [
    new Integrations.Tracing({
      tracingOrigins: [/localhost/]
    }),
    new VueIntegration({ Vue })
  ],
  tracesSampleRate: 1.0
});

Example usage (w. components tracking):

import * as Sentry from "@sentry/browser";
import { Vue as VueIntegration } from "@sentry/integrations";
import { Integrations } from "@sentry/apm";
import Vue from "vue";

Sentry.init({
  dsn: "https://lol@nope.ingest.sentry.io/1337",
  integrations: [
    new Integrations.Tracing({
      tracingOrigins: [/localhost/]
    }),
    new VueIntegration({
      Vue,
      tracingOptions: {
        trackComponents: ["App", "home", "RwvArticleList", "Pagination", "RwvHeader", "RwvFooter"]
      }
    })
  ],
  tracesSampleRate: 1.0
});

https://github.com/getsentry/sentry-vue-perf can be used to verify the implementation locally (just swap the implementation in there, for the one showcased above).

@kamilogorek kamilogorek requested a review from a team May 6, 2020 15:03
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

First pass 👀

* https://github.com/vuejs/vue/blob/c2b1cfe9ccd08835f2d99f6ce60f67b4de55187f/src/core/util/error.js#L38-L48
*/
logErrors: boolean;
tracing: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reasonably detect that tracing is enabled without this?
We've talked about enabling tracing whenever the Tracing integration was on. That would make for one less boilerplate line in Sentry.init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'm already doing that. But what if someone wants to have error handling, xhr tracing, but skip Vue tracing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also cannot tell if Tracing integration is in place during setup (subject to change in v6)

@getsentry-bot
Copy link
Contributor

getsentry-bot commented May 7, 2020

Fails
🚫

TSLint failed: @sentry/integrations

  • ERROR: /home/travis/build/getsentry/sentry-javascript/packages/integrations/src/vue.ts[8, 24]: Type assertion on object literals is forbidden, use a type annotation instead.
  • ERROR: /home/travis/build/getsentry/sentry-javascript/packages/integrations/src/vue.ts[239, 37]: Unsafe use of expression of type 'any'.
  • ERROR: /home/travis/build/getsentry/sentry-javascript/packages/integrations/src/vue.ts[310, 11]: Unsafe use of expression of type 'any'.
Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 17.1162 kB) (ES6: 16.1475 kB)

Generated by 🚫 dangerJS against 4df15af

@@ -16,6 +16,7 @@
"module": "esm/index.js",
"types": "dist/index.d.ts",
"dependencies": {
"@sentry/apm": "5.15.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this is interesting. How does it affect bundle size?
How does it impact users of the integrations that don't use @sentry/apm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't if you are not using Vue. It'll be fetched though.
I was thinking about passing apm's Tracing integration as a parameter, buuut not sure if it's worth it just yet.

interface TracingOptions {
/**
* Decides whether to track components by hooking into its lifecycle methods.
* Can be either set to `boolean` to enable/disable tracking for all of them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we defer the option to "track all"? It seems like a sharp edge for me.

Plus, TracingOptions.trackComponents: false somehow conflicts with IntegrationOptions.tracing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trackComponents will be effectively ignored if you don't set tracing: true.
Regarding tracking everything, I'm not sure why we'd want to postpone it tbh cc @HazAT

@kamilogorek kamilogorek merged commit 5cd146b into master May 12, 2020
@kamilogorek kamilogorek deleted the vue-apm branch May 12, 2020 09:27
@rchl
Copy link
Contributor

rchl commented Jun 2, 2020

Is documentation for this released already? Can't see this mentioned on https://docs.sentry.io/performance/distributed-tracing/ so I guess not?

Also, will this be usable when doing server-side rendering of Vue (for example in Nuxt)?

@HazAT
Copy link
Member

HazAT commented Jun 2, 2020

We are currently working on the docs update for this.
No server-side rendering support (yet).

@rchl
Copy link
Contributor

rchl commented Jun 4, 2020

BTW. I think it's wrong that tracing: true is enabled by default since that triggers an error for existing users of Vue integration that don't use tracing:

Sentry Logger [Error]: Vue integration has tracing enabled, but Tracing integration is not configured

So that's effectively a breaking change (even though it might not be breaking anything apart from introducing an error message).

What's your opinion on that? Should it change?

@HazAT
Copy link
Member

HazAT commented Jun 4, 2020

Good catch, yeah we should remove the error message in case tracing is not enabled.
cc @kamilogorek

@rchl
Copy link
Contributor

rchl commented Jun 4, 2020

Clarification, sorry. This is only logged with debug: true set. So maybe it's not an issue? But still kinda wrong to say it's an error if it's handled gracefully.

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

5 participants