feat: add vite-plugin sentry #1991
No reviewers
konrad
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
3 Participants
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#1991
Loading…
Reference in New Issue
No description provided.
Delete Branch "dpschen/frontend:feature/feat-vite-plugin-sentry"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I started adding this, but realized that I don't know the properties that I need to set this up.
TODO
dryRun,debugandskipEnvironmentCheckfrom configWhat does the plugin do compared to the sentry integration we already have? Bundle configuration and plugins at build time for production builds?
Hi dpschen!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://1991-feature-feat-vite-plugin-sentry--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
It provides sourcemap upload.
The sentry plugin only is there for error tracking itself.
There is an offical plugin for webpack, but none for vite.
https://docs.sentry.io/platforms/javascript/sourcemaps/
This is a port of the webpack version
Ah I see, makes sense to use it then.
5e11609eddto300e336cdaWhat's missing here to get this merged?
Probably needs an update and I was not sure how to provide the configuration via drone
I'll have to save the drone secrets as I am the repo admin.
300e336cdato3a2b6bf559WIP: feat: add vite-plugin sentryto feat: add vite-plugin sentry@ -7,3 +7,3 @@"serve:dist-dev": "node scripts/serve-dist.js","serve:dist": "vite preview --port 4173","build": "vite build && workbox copyLibraries dist/","build": "vite build && rimraf dist/**/*.js.map && workbox copyLibraries dist/",We remove the sourcemaps so they do not bloat the build, but only uploaded to sentry.
Are they still referenced in the code?
I don't think so. This was the recommended way of doing this.
3a2b6bf559to996bdd1baf@ -4,3 +4,3 @@export function useOnline(options?: ConfigurableWindow) {const fakeOnlineState = !!import.meta.env.VITE_IS_ONLINEconst fakeOnlineState = Boolean(import.meta.env.VITE_IS_ONLINE)Using
Booleanis more explicit.@ -4,3 +4,3 @@import router from './router'import App from './App.vue'import {setupSentry} from './sentry'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?
Good question. By dev builds you mean while in development, the
build:devcommand 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
eageroption enabled.While in development or built with
build:dev.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:
TBH I think the only real way here are two separate builds where in one it gets conditionally included.
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.
I see. Still, I'd like to avoid the added complexity of releasing two versions for everything.
@ -100,3 +99,3 @@if (window.SENTRY_ENABLED) {import('./sentry').then(sentry => sentry.default(app, router))try{I put sentry tracking libs in a dedicated bundle. I hope by doing so we don't have any problems with unwittingly blocked tracking libraries that are actually part of the application bundle.
So that means all sentry-related code will be a seperate bundle which will always be loaded but only activated if sentry is enabled?
@ -9,4 +9,3 @@Sentry.init({release: VERSION,app,dsn: window.SENTRY_DSN,Would probably be good to use env for
SENTRY_DSNas well.@ -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'This allows us to share the config with
viteSentry@ -23,3 +26,1 @@// recommended by browserslist => https://github.com/vitejs/vite/tree/main/packages/plugin-legacy#targetstargets: ['defaults', 'not IE 11'],}): legacyFn()defaultsdoesn't include IE11 anymore@ -44,0 +36,4 @@function getSentryConfig(env: ImportMetaEnv): ViteSentryPluginOptions {return {// dryRun: true, // FIXME: remove when ready with configuringdebug: true, // FIXME: remove when ready with configuringdryRunanddebugshould be removed when our setup works.When is that the case?
When we are happy with this pull request, the data is correct and we want to enable it.
@ -44,0 +37,4 @@return {// dryRun: true, // FIXME: remove when ready with configuringdebug: true, // FIXME: remove when ready with configuringskipEnvironmentCheck: true,I had to set this as well at some point to debug. Should probably also be removed.
@ -44,0 +39,4 @@debug: true, // FIXME: remove when ready with configuringskipEnvironmentCheck: true,url: 'https://sentry.io',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.
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.
@ -44,0 +45,4 @@project: env.SENTRY_PROJECT,release: env.SENTRY_VERSION || VERSION,deploy: {env: env.mode === 'production'If I got this correctly we should probably improve this to have dedicated Sentry environments for areas like:
We also might want to add other properties of deploy.
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:
env.mode === 'development'or similar)We track currently in
tryright?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
productionenv (because it's a production build) and add try as a deployment name.@ -127,0 +155,4 @@},}),viteSentry(getSentryConfig(env)),visualizer({Unsure, but I think this wasn't working anymore since vite 3.
Okay, some notes about the current state:
SENTRY_AUTH_TOKEN) to be defined in drone as well as some of them in the index.html. The later are overwritten via the run.sh. I'm really unsure if this is the way to go. Seems like we should unify this somehow. Maybe one way could be to use a vite plugin like https://github.com/vbenjs/vite-plugin-html to set the window variables there via the env. This way we still use the envs for build. Really unsure here. One other common thing to do here would be to have some kind of manifest file where we would define this. I'm not a big fan of the latter since it needs an extra request and thus delays the loading..env.local.example?@ -68,2 +68,4 @@environment:PNPM_CACHE_FOLDER: .cache/pnpmSENTRY_AUTH_TOKEN:from_secret: sentry_auth_tokenI've created a new internal integration in sentry and added its token to the drone variable.
Did you add it under
sentry_auth_tokenor with a different name?I've created it as
sentry_auth_token.@ -70,0 +72,4 @@SENTRY_ORG: vikunjaSENTRY_PROJECT: frontend-ossSENTRY_RELEASE:from_secret: sentry_releaseWhy should this be a secret? Shouldn't this be the same as
window.version?window.versiondoesn't exist? Do you mean getting the version number fromversion.json?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.
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
We'll need a way to set this at runtime. I think the plugin only does this at build time?
I think that's a good idea.
@ -11,2 +10,4 @@app,dsn: window.SENTRY_DSN,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?
No. As written here: #1991 (comment)
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.
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.
Posting this here as a reminder to implement somehow similar later:
996bdd1bafto813f7b7608Is this ready to review?
813f7b7608to653415e764I've changed a few things and think this is ready to merge now:
version.jsonfile (generated during CI builds). Local builds which need a different version can still change the version via that file and we avoid another config option.It probably needs a few fixes but we can only figure those out when it's running in production. I'll merge this now so that we can get there quickly.
Thank you for your efforts!