feat: add vite-plugin sentry #1991

Open
dpschen wants to merge 1 commits from dpschen/frontend:feature/feat-vite-plugin-sentry into main
Collaborator

I started adding this, but realized that I don't know the properties that I need to set this up.

TODO

  • remove dryRun, debug and skipEnvironmentCheck from config
I started adding this, but realized that I don't know the properties that I need to set this up. ## TODO - [ ] remove `dryRun`, `debug` and `skipEnvironmentCheck` from config
dpschen added 1 commit 8 months ago
continuous-integration/drone/pr Build is passing Details
5e11609edd
feat: add vite-plugin sentry
dpschen added the kind/feature help wanted question labels 8 months ago
Owner

What does the plugin do compared to the sentry integration we already have? Bundle configuration and plugins at build time for production builds?

What does the plugin do compared to the sentry integration we already have? Bundle configuration and plugins at build time for production builds?
Collaborator

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!

Beep boop, I'm a bot.

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! > Beep boop, I'm a bot.
Poster
Collaborator

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

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
Owner

Ah I see, makes sense to use it then.

Ah I see, makes sense to use it then.
dpschen force-pushed feature/feat-vite-plugin-sentry from 5e11609edd to 300e336cda 5 months ago
Owner

What's missing here to get this merged?

What's missing here to get this merged?
Poster
Collaborator

Probably needs an update and I was not sure how to provide the configuration via drone

Probably needs an update and I was not sure how to provide the configuration via drone
Owner

I'll have to save the drone secrets as I am the repo admin.

I'll have to save the drone secrets as I am the repo admin.
dpschen force-pushed feature/feat-vite-plugin-sentry from 300e336cda to 3a2b6bf559 3 months ago
dpschen requested review from konrad 3 months ago
konrad was assigned by dpschen 3 months ago
dpschen changed title from WIP: feat: add vite-plugin sentry to feat: add vite-plugin sentry 3 months ago
dpschen reviewed 3 months ago
package.json Outdated
"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/",
Poster
Collaborator

We remove the sourcemaps so they do not bloat the build, but only uploaded to sentry.

We remove the sourcemaps so they do not bloat the build, but only uploaded to sentry.
Owner

Are they still referenced in the code?

Are they still referenced in the code?
Poster
Collaborator

I don't think so. This was the recommended way of doing this.

I don't think so. This was [the recommended way of doing this](https://github.com/ikenfin/vite-plugin-sentry#delete-generated-source-maps-after-upload-1).
dpschen marked this conversation as resolved
dpschen force-pushed feature/feat-vite-plugin-sentry from 3a2b6bf559 to 996bdd1baf 3 months ago
dpschen reviewed 3 months ago
export function useOnline(options?: ConfigurableWindow) {
const fakeOnlineState = !!import.meta.env.VITE_IS_ONLINE
const fakeOnlineState = Boolean(import.meta.env.VITE_IS_ONLINE)
Poster
Collaborator

Using Boolean is more explicit.

Using `Boolean` is more explicit.
dpschen marked this conversation as resolved
dpschen reviewed 3 months ago
import router from './router'
import App from './App.vue'
import {setupSentry} from './sentry'
Poster
Collaborator

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.
Owner

So it will only be loaded synchroniously in dev builds?

So it will only be loaded synchroniously in dev builds?
Poster
Collaborator

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.
Owner

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.
Poster
Collaborator

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.
Owner

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).
Poster
Collaborator

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.
Owner

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.
dpschen reviewed 3 months ago
if (window.SENTRY_ENABLED) {
import('./sentry').then(sentry => sentry.default(app, router))
try{
Poster
Collaborator

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.

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.
Owner

So that means all sentry-related code will be a seperate bundle which will always be loaded but only activated if sentry is enabled?

So that means all sentry-related code will be a seperate bundle which will always be loaded but only activated if sentry is enabled?
dpschen reviewed 3 months ago
Sentry.init({
release: VERSION,
app,
dsn: window.SENTRY_DSN,
Poster
Collaborator

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

Would probably be good to use env for `SENTRY_DSN` as well.
dpschen reviewed 3 months ago
import type { App } from 'vue'
import type { Router } from 'vue-router'
import {VERSION} from './version.json'
import 'virtual:vite-plugin-sentry/sentry-config'
Poster
Collaborator

This allows us to share the config with viteSentry

This allows us to share the config with `viteSentry`
dpschen marked this conversation as resolved
dpschen reviewed 3 months ago
// recommended by browserslist => https://github.com/vitejs/vite/tree/main/packages/plugin-legacy#targets
targets: ['defaults', 'not IE 11'],
})
: legacyFn()
Poster
Collaborator

defaults doesn't include IE11 anymore

`defaults` doesn't include IE11 anymore
dpschen marked this conversation as resolved
dpschen reviewed 3 months ago
function getSentryConfig(env: ImportMetaEnv): ViteSentryPluginOptions {
return {
// dryRun: true, // FIXME: remove when ready with configuring
debug: true, // FIXME: remove when ready with configuring
Poster
Collaborator

dryRun and debug should be removed when our setup works.

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

When is that the case?

When is that the case?
Poster
Collaborator

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.
dpschen reviewed 3 months ago
return {
// dryRun: true, // FIXME: remove when ready with configuring
debug: true, // FIXME: remove when ready with configuring
skipEnvironmentCheck: true,
Poster
Collaborator

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.
dpschen reviewed 3 months ago
debug: true, // FIXME: remove when ready with configuring
skipEnvironmentCheck: true,
url: 'https://sentry.io',
Poster
Collaborator

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?
Owner

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.
Poster
Collaborator

Is this the same as the DSN?

Is this the same as the `DSN`?
Owner

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.
dpschen reviewed 3 months ago
project: env.SENTRY_PROJECT,
release: env.SENTRY_VERSION || VERSION,
deploy: {
env: env.mode === 'production'
Poster
Collaborator

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).
Owner

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

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

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?)
Poster
Collaborator

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 :)
Owner

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.
dpschen reviewed 3 months ago
},
}),
viteSentry(getSentryConfig(env)),
visualizer({
Poster
Collaborator

Unsure, but I think this wasn't working anymore since vite 3.

Unsure, but I think this wasn't working anymore since vite 3.
Poster
Collaborator

Okay, some notes about the current state:

  • We have variable definitions for sentry as env variable (like 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.
  • Should we add the env variables to a .env.local.example ?
Okay, some notes about the current state: - [ ] We have variable definitions for sentry as env variable (like `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. - [ ] Should we add the env variables to a `.env.local.example` ?
konrad reviewed 3 months ago
environment:
PNPM_CACHE_FOLDER: .cache/pnpm
SENTRY_AUTH_TOKEN:
from_secret: sentry_auth_token
Owner

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.
Poster
Collaborator

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?
Owner

I've created it as sentry_auth_token.

I've created it as `sentry_auth_token`.
dpschen marked this conversation as resolved
SENTRY_ORG: vikunja
SENTRY_PROJECT: frontend-oss
SENTRY_RELEASE:
from_secret: sentry_release
Owner

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`?
Poster
Collaborator

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`?
Owner

Yes, I meant that, sorry for the confusion

Yes, I meant that, sorry for the confusion
Poster
Collaborator

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.
Owner

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
Owner

We have variable definitions for sentry as env variable (like 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

We'll need a way to set this at runtime. I think the plugin only does this at build time?

> We have variable definitions for sentry as env variable (like 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 We'll need a way to set this at runtime. I think the plugin only does this at build time?
Owner

Should we add the env variables to a .env.local.example ?

I think that's a good idea.

> Should we add the env variables to a .env.local.example ? I think that's a good idea.
konrad reviewed 3 months ago
app,
dsn: window.SENTRY_DSN,
release: import.meta.env.VITE_PLUGIN_SENTRY_CONFIG.release,
dist: import.meta.env.VITE_PLUGIN_SENTRY_CONFIG.dist,
Owner

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?
Poster
Collaborator

No. As written here: #1991

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

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?
Owner

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.
Poster
Collaborator

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

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

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.
Poster
Collaborator

Posting this here as a reminder to implement somehow similar later:

<body>
<noscript>
	<strong>We're sorry but Vikunja doesn't work properly without JavaScript enabled. Please enable it to continue.</strong>
</noscript>
<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 
	// our sentry instance to notify us of potential problems.
	window.SENTRY_ENABLED = false
	window.SENTRY_DSN = 'https://85694a2d757547cbbc90cd4b55c5a18d@o1047380.ingest.sentry.io/6024480'
</script>

<script type="module">
async function loadScript(url) {
	const el = document.createElement('script');
	const scriptLoaded = new Promise(() => {
		script.onload = resolve
	})
	el.src = url;

	const firstScript = document.getElementsByTagName('script')[0];
	firstScript.parentNode.insertBefore(el, firstScript);
	await scriptLoaded()
}

if (window.SENTRY_ENABLED) {
	const SCRIPT_URL = ''
	// make sure that sentry is loaded _before_ app loading
	await loadScript(SCRIPT_URL)
}
</script>
<div id="app"></div>
<script type="module" src="/src/main.ts"></script>
</body>
Posting this here as a reminder to implement somehow similar later: ```html <body> <noscript> <strong>We're sorry but Vikunja doesn't work properly without JavaScript enabled. Please enable it to continue.</strong> </noscript> <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 // our sentry instance to notify us of potential problems. window.SENTRY_ENABLED = false window.SENTRY_DSN = 'https://85694a2d757547cbbc90cd4b55c5a18d@o1047380.ingest.sentry.io/6024480' </script> <script type="module"> async function loadScript(url) { const el = document.createElement('script'); const scriptLoaded = new Promise(() => { script.onload = resolve }) el.src = url; const firstScript = document.getElementsByTagName('script')[0]; firstScript.parentNode.insertBefore(el, firstScript); await scriptLoaded() } if (window.SENTRY_ENABLED) { const SCRIPT_URL = '' // make sure that sentry is loaded _before_ app loading await loadScript(SCRIPT_URL) } </script> <div id="app"></div> <script type="module" src="/src/main.ts"></script> </body> ```

Reviewers

konrad was requested for review 3 months ago
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details
This pull request has changes conflicting with the target branch.
env.d.ts
package.json
pnpm-lock.yaml
src/composables/useOnline.ts
tsconfig.vite-config.json
vite.config.ts
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1991
Loading…
There is no content yet.