feat: add vite-plugin sentry #1991

Merged
konrad merged 8 commits from dpschen/frontend:feature/feat-vite-plugin-sentry into main 2023-06-18 13:40:23 +00:00
10 changed files with 119 additions and 22 deletions

View File

@ -70,6 +70,10 @@ steps:
pull: always
environment:
dpschen marked this conversation as resolved Outdated

I've created a new internal integration in sentry and added its token to the drone variable.

I've created a new internal integration in sentry and added its token to the drone variable.

Did you add it under sentry_auth_token or with a different name?

Did you add it under `sentry_auth_token` or with a different name?

I've created it as sentry_auth_token.

I've created it as `sentry_auth_token`.
PNPM_CACHE_FOLDER: .cache/pnpm
SENTRY_AUTH_TOKEN:
from_secret: sentry_auth_token
SENTRY_ORG: vikunja

Why should this be a secret? Shouldn't this be the same as window.version?

Why should this be a secret? Shouldn't this be the same as `window.version`?

window.version doesn't exist? Do you mean getting the version number from version.json?

`window.version` doesn't exist? Do you mean getting the version number from `version.json`?

Yes, I meant that, sorry for the confusion

Yes, I meant that, sorry for the confusion

If we work locally that doesn't include the current commit, right?
Still not sure what I should do here now.

If we work locally that doesn't include the current commit, right? Still not sure what I should do here now.

It will only include the last commit. I'd argue building for prod locally is an edge case where we can set the version manually. No need to overcomplicate things

It will only include the last commit. I'd argue building for prod locally is an edge case where we can set the version manually. No need to overcomplicate things
SENTRY_PROJECT: frontend-oss
commands:
- corepack enable && pnpm config set store-dir .cache/pnpm
- pnpm run build
@ -521,6 +525,6 @@ steps:
from_secret: crowdin_key
---
kind: signature
hmac: a41964ffb64789df5553d7f51e05ac60d8243a4d8b7dfdd5be8de851aea5f9d7
hmac: f2524d77d2d7d284319e20650627cc68b81631d166a0dab808d272e7a132054f
...

24
env.d.ts vendored
View File

@ -4,19 +4,31 @@
/// <reference types="@histoire/plugin-vue/components" />
declare module 'postcss-focus-within/browser' {
import focusWithinInit from 'postcss-focus-within/browser'
export default focusWithinInit
import focusWithinInit from 'postcss-focus-within/browser'
export default focusWithinInit
}
declare module 'css-has-pseudo/browser' {
import cssHasPseudo from 'css-has-pseudo/browser'
export default cssHasPseudo
import cssHasPseudo from 'css-has-pseudo/browser'
export default cssHasPseudo
}
interface ImportMetaEnv {
readonly VITE_IS_ONLINE: boolean
readonly VIKUNJA_API_URL?: string
readonly VIKUNJA_HTTP_PORT?: number
readonly VIKUNJA_HTTPS_PORT?: number
readonly VIKUNJA_SENTRY_ENABLED?: boolean
readonly VIKUNJA_SENTRY_DSN?: string
readonly SENTRY_AUTH_TOKEN?: string
readonly SENTRY_ORG?: string
readonly SENTRY_PROJECT?: string
readonly VITE_WORKBOX_DEBUG?: boolean
readonly VITE_IS_ONLINE: boolean
}
interface ImportMeta {
readonly env: ImportMetaEnv
readonly env: ImportMetaEnv
}

View File

@ -18,12 +18,12 @@
<div id="app"></div>
<script type="module" src="/src/main.ts"></script>
<script>
//
//
// This variable points the frontend to the api.
// It has to be the full url, including the last /api/v1 part and port.
// You can change this if your api is not reachable on the same port as the frontend.
window.API_URL = 'http://localhost:3456/api/v1'
// Enable error tracking with sentry. If this is set to true, will send anonymized data to
// Enable error tracking with sentry. If this is set to true, will send anonymized data to
// our sentry instance to notify us of potential problems.
window.SENTRY_ENABLED = false
window.SENTRY_DSN = 'https://85694a2d757547cbbc90cd4b55c5a18d@o1047380.ingest.sentry.io/6024480'

View File

@ -136,6 +136,7 @@
"vite": "4.3.9",
"vite-plugin-inject-preload": "1.3.1",
"vite-plugin-pwa": "0.16.4",
"vite-plugin-sentry": "1.1.6",
"vite-svg-loader": "4.0.0",
"vitest": "0.32.2",
"vue-tsc": "1.8.0",

View File

@ -1,4 +1,4 @@
lockfileVersion: '6.0'
lockfileVersion: '6.1'
settings:
autoInstallPeers: true
@ -279,6 +279,9 @@ devDependencies:
vite-plugin-pwa:
specifier: 0.16.4
version: 0.16.4(vite@4.3.9)(workbox-build@7.0.0)(workbox-window@7.0.0)
vite-plugin-sentry:
specifier: 1.1.6
version: 1.1.6(vite@4.3.9)
vite-svg-loader:
specifier: 4.0.0
version: 4.0.0
@ -3008,6 +3011,22 @@ packages:
tslib: 1.14.1
dev: false
/@sentry/cli@2.19.1:
resolution: {integrity: sha512-7RVwOUwVoOplsG1Jqo8YCke2BEQpGU+AvXAhl1HP6S4qB4mFat6Asr9EhvapPZvufpC85RvLt22h3q0yakRgaA==}
engines: {node: '>= 10'}
hasBin: true
requiresBuild: true
dependencies:
https-proxy-agent: 5.0.1
node-fetch: 2.6.7
progress: 2.0.3
proxy-from-env: 1.1.0
which: 2.0.2
transitivePeerDependencies:
- encoding
- supports-color
dev: true
/@sentry/core@7.55.2:
resolution: {integrity: sha512-clzQirownxqADv9+fopyMJTHzaoWedkN2+mi4ro1LxjLgROdXBFurMCC1nT+7cH/xvQ5gMIRkM/y/4gRtKy2Ew==}
engines: {node: '>=8'}
@ -9228,6 +9247,19 @@ packages:
- supports-color
dev: true
/vite-plugin-sentry@1.1.6(vite@4.3.9):
resolution: {integrity: sha512-IgrIIl+VKwfLEoJ5O/8rplWmnkjdzqlBwmncWJH19mitr/v7SP7YnXlzv+vdAvxBo5eA8QPYx0J2i58NnaQ3fg==}
engines: {node: '>= 12'}
peerDependencies:
vite: ^2.6.0 || ^3.0.0
dependencies:
'@sentry/cli': 2.19.1
vite: 4.3.9(@types/node@18.16.18)(sass@1.63.4)(terser@5.10.0)
transitivePeerDependencies:
- encoding
- supports-color
dev: true
/vite-svg-loader@4.0.0:
resolution: {integrity: sha512-0MMf1yzzSYlV4MGePsLVAOqXsbF5IVxbn4EEzqRnWxTQl8BJg/cfwIzfQNmNQxZp5XXwd4kyRKF1LytuHZTnqA==}
dependencies:

View File

@ -4,7 +4,7 @@ import type {ConfigurableWindow} from '@vueuse/core'
export function useOnline(options?: ConfigurableWindow) {
const isOnline = useNetworkOnline(options)
dpschen marked this conversation as resolved Outdated

Using Boolean is more explicit.

Using `Boolean` is more explicit.
const fakeOnlineState = !!import.meta.env.VITE_IS_ONLINE
const fakeOnlineState = Boolean(import.meta.env.VITE_IS_ONLINE)
if (isOnline.value === false && fakeOnlineState) {
console.log('Setting fake online state', fakeOnlineState)
return ref(true)

View File

@ -4,9 +4,7 @@ import {createApp} from 'vue'
import pinia from './pinia'
import router from './router'
import App from './App.vue'

By loading sentry synchroniously I hope that it tracks errors that happen very early in the app.
This will impact total load time of the app, but as a dev tool I think it's fine.

By loading sentry synchroniously I hope that it tracks errors that happen very early in the app. This will impact total load time of the app, but as a dev tool I think it's fine.

So it will only be loaded synchroniously in dev builds?

So it will only be loaded synchroniously in dev builds?

Good question. By dev builds you mean while in development, the build:dev command or also the ones we have for try?

I guess it might really make sense to really not load it depending on the build. I'm not a fan of bundling tracking scripts, but for good development it's really helpful.

Maybe this could be possible when we load the sentry file with the help of vites glob import with the eager option enabled.

Good question. By dev builds you mean while in development, the `build:dev` command or also the ones we have for try? I guess it might really make sense to really not load it depending on the build. I'm not a fan of bundling tracking scripts, but for good development it's really helpful. Maybe this could be possible when we load the sentry file with the help of [vites glob import](https://vitejs.dev/guide/features.html#glob-import) with the `eager` option enabled.

Good question. By dev builds you mean while in development, the build:dev command or also the ones we have for try?

While in development or built with build:dev.

I guess it might really make sense to really not load it depending on the build. I'm not a fan of bundling tracking scripts, but for good development it's really helpful.

I think it's fine to load this automatically for dev builds but should be disableable (and not loaded in that case) for prod builds.

> Good question. By dev builds you mean while in development, the build:dev command or also the ones we have for try? While in development or built with `build:dev`. > I guess it might really make sense to really not load it depending on the build. I'm not a fan of bundling tracking scripts, but for good development it's really helpful. I think it's fine to load this automatically for dev builds but should be disableable (and not loaded in that case) for prod builds.

Tricky if we want to:

  • have it loaded synchroniously (in order to track errors on initial load)
  • disable it via a window variable

TBH I think the only real way here are two separate builds where in one it gets conditionally included.

Tricky if we want to: - have it loaded synchroniously (in order to track errors on initial load) - disable it via a window variable TBH I think the only real way here are two separate builds where in one it gets conditionally included.

in order to track errors on initial load

Maybe that would be an acceptable trade-off? (To not try to catch these kinds of errors).

> in order to track errors on initial load Maybe that would be an acceptable trade-off? (To not try to catch these kinds of errors).

Unsure. Lots of errors happen on the initial load.
Stuff can behave differently once SPA routing is enabled.

Unsure. Lots of errors happen on the initial load. Stuff can behave differently once SPA routing is enabled.

I see. Still, I'd like to avoid the added complexity of releasing two versions for everything.

I see. Still, I'd like to avoid the added complexity of releasing two versions for everything.
import {error, success} from './message'
import {VERSION} from './version.json'
// Notifications
@ -106,7 +104,11 @@ setLanguage(browserLanguage).then(() => {
}
if (window.SENTRY_ENABLED) {
import('./sentry').then(sentry => sentry.default(app, router))
try {
import('./sentry').then(sentry => sentry.default(app, router))
} catch (e) {
console.error('Could not enable Sentry tracking', e)
}
}
app.use(pinia)

View File

@ -1,15 +1,16 @@
import type { App } from 'vue'
import type { Router } from 'vue-router'
import {VERSION} from './version.json'
import 'virtual:vite-plugin-sentry/sentry-config'
dpschen marked this conversation as resolved Outdated

This allows us to share the config with viteSentry

This allows us to share the config with `viteSentry`
import type {App} from 'vue'
import type {Router} from 'vue-router'
export default async function setupSentry(app: App, router: Router) {
const Sentry = await import('@sentry/vue')
const {Integrations} = await import('@sentry/tracing')
Sentry.init({
release: VERSION,
app,
dsn: window.SENTRY_DSN,

Would probably be good to use env for SENTRY_DSN as well.

Would probably be good to use env for `SENTRY_DSN` as well.
release: import.meta.env.VITE_PLUGIN_SENTRY_CONFIG.release,
dist: import.meta.env.VITE_PLUGIN_SENTRY_CONFIG.dist,

Won't this always load the config from env, bypassing the values set in vite's config file?

Won't this always load the config from env, bypassing the values set in vite's config file?

No. As written here: #1991 (comment)

No. As written here: https://kolaente.dev/vikunja/frontend/pulls/1991#issuecomment-38994

Seems like that link doesn't work, so:
import 'virtual:vite-plugin-sentry/sentry-config' allows us to share the config with the vite config file via its custom env variables. this way they stay in sync. But if that just makes things more complex and we have them in a env variable either way, I guess we could also use them directly?

Seems like that link doesn't work, so: `import 'virtual:vite-plugin-sentry/sentry-config'` allows us to share the config with the vite config file **via** its custom env variables. this way they stay in sync. But if that just makes things more complex and we have them in a env variable either way, I guess we could also use them directly?

If it would be more complex and kind of redundant (because we need env anyway) I'm all for using it directly to not overcomplicate things.

If it would be more complex and kind of redundant (because we need env anyway) I'm all for using it directly to not overcomplicate things.

Yeah I realized this basically while writing :D will change.

Yeah I realized this basically while writing :D will change.

Actually the dist is a dynamic value depending on the dist in the vite config.
Same for release.

Actually the dist is a dynamic value depending on the dist in the vite config. Same for release.
integrations: [
new Integrations.BrowserTracing({
routingInstrumentation: Sentry.vueRouterInstrumentation(router),

View File

@ -14,6 +14,10 @@
"paths": {
"@/*": ["./src/*"]
},
"types": [
// https://github.com/ikenfin/vite-plugin-sentry#typescript
"vite-plugin-sentry/client"
],
"ignoreDeprecations": "5.0"
},
"vueCompilerOptions": {

View File

@ -9,6 +9,8 @@ import VueI18nPlugin from '@intlify/unplugin-vue-i18n/vite'
import {VitePWA} from 'vite-plugin-pwa'
import VitePluginInjectPreload from 'vite-plugin-inject-preload'
import {visualizer} from 'rollup-plugin-visualizer'
import viteSentry, {type ViteSentryPluginOptions} from 'vite-plugin-sentry'
import svgLoader from 'vite-svg-loader'
import postcssPresetEnv from 'postcss-preset-env'
import postcssEasings from 'postcss-easings'
@ -17,6 +19,8 @@ import postcssEasingGradients from 'postcss-easing-gradients'
const pathSrc = fileURLToPath(new URL('./src', import.meta.url))
import {VERSION} from './src/version.json'
// the @use rules have to be the first in the compiled stylesheets
const PREFIXED_SCSS_STYLES = `@use "sass:math";
@import "${pathSrc}/styles/common-imports";`
dpschen marked this conversation as resolved Outdated

defaults doesn't include IE11 anymore

`defaults` doesn't include IE11 anymore
@ -24,16 +28,44 @@ const PREFIXED_SCSS_STYLES = `@use "sass:math";
const isModernBuild = Boolean(process.env.BUILD_MODERN_ONLY)
const legacy = isModernBuild
? undefined
: legacyFn({
// recommended by browserslist => https://github.com/vitejs/vite/tree/main/packages/plugin-legacy#targets
targets: ['defaults', 'not IE 11'],
})
: legacyFn()
console.log(isModernBuild
? 'Building "modern-only" build'
: 'Building "legacy" build with "@vitejs/plugin-legacy"',
)
/*
** Configure sentry plugin

dryRun and debug should be removed when our setup works.

`dryRun` and `debug` should be removed when our setup works.

When is that the case?

When is that the case?

When we are happy with this pull request, the data is correct and we want to enable it.

When we are happy with this pull request, the data is correct and we want to enable it.
*/

I had to set this as well at some point to debug. Should probably also be removed.

I had to set this as well at some point to debug. Should probably also be removed.
function getSentryConfig(env: ImportMetaEnv): ViteSentryPluginOptions {
return {

Unsure if the url also should be an env. Is it even possible to host you own sentry instance?

Unsure if the url also should be an env. Is it even possible to host you own sentry instance?

It is possible, yes. That was the reason behind putting the dsn in a window variable.

It is possible, yes. That was the reason behind putting the dsn in a window variable.

Is this the same as the DSN?

Is this the same as the `DSN`?

I don't think it is. I'm not sure why there is a seperate option to configre this in the plugin.

I don't think it is. I'm not sure why there is a seperate option to configre this in the plugin.
// dryRun: true, // FIXME: remove when ready with configuring
debug: true, // FIXME: remove when ready with configuring
skipEnvironmentCheck: true,
url: 'https://sentry.io',
authToken: env.SENTRY_AUTH_TOKEN,

If I got this correctly we should probably improve this to have dedicated Sentry environments for areas like:

  • dev
  • try
  • production
  • maybe the pull request previews
  • ???

We also might want to add other properties of deploy.

If I got this correctly we should probably improve this to have [dedicated Sentry environments](https://sentry.io/settings/vikunja/projects/frontend-oss/environments/) for areas like: - dev - try - production - maybe the pull request previews - ??? We also might want to add [other properties of deploy](https://github.com/ikenfin/vite-plugin-sentry#deploy-settings).

Can we pass arbitrary strings to sentry to set the environment?

Can we pass arbitrary strings to sentry to set the environment?

I think the environment setting should depend on the bundle type. Things like try should be a name, not an environment. Maybe something like that:

  • production - default, used for try etc
  • dev - used for dev builds (env.mode === 'development' or similar)
  • pr - used for preview envs in PRs
  • ci - (not sure if we're even enabling sentry in ui tests?)
I think the environment setting should depend on the bundle type. Things like try should be a name, not an environment. Maybe something like that: * production - default, used for try etc * dev - used for dev builds (`env.mode === 'development'` or similar) * pr - used for preview envs in PRs * ci - (not sure if we're even enabling sentry in ui tests?)

We track currently in try right?

Asking because if we do remove that I guess we find much less errors with the help of people trying out Vikunja :)

We track currently in `try` right? Asking because if we do remove that I guess we find much less errors with the help of people trying out Vikunja :)

Yes, we currently track in try. I'd keep it enabled but as a production env (because it's a production build) and add try as a deployment name.

Yes, we currently track in try. I'd keep it enabled but as a `production` env (because it's a production build) and add try as a deployment name.
org: env.SENTRY_ORG,
project: env.SENTRY_PROJECT,
release: VERSION,
cleanSourcemapsAfterUpload: true,
deploy: {
env: env.mode === 'production'
? 'production'
: 'development',
},
setCommits: {
auto: true,
},
sourceMaps: {
include: ['./dist/assets'],
ignore: ['node_modules'],
urlPrefix: '~/assets',
},
}
}
/**
* @param fontNames Array of the file names of the fonts without axis and hash suffixes
*/
@ -171,6 +203,7 @@ export default defineConfig(({mode}) => {
],
},
}),
viteSentry(getSentryConfig(env)),
],
resolve: {
alias: [
@ -186,8 +219,16 @@ export default defineConfig(({mode}) => {
port: 4173,
strictPort: true,
},
output: {
manualChunks: {
// by putting tracking related stuff in a separated file we try to prevent unwanted blocking from ad-blockers
sentry: ['./src/sentry.ts', '@sentry/vue', '@sentry/tracing'],
},
},
build: {
target: 'esnext',
// required for sentry debugging: tells vite to create source maps
sourcemap: true,
rollupOptions: {
plugins: [
visualizer({