feat: add sentry #879

konrad · 2021-10-19T20:02:38Z

konrad commented

2021-10-19 20:02:38 +00:00

No description provided.

dpschen was assigned by konrad

2021-10-19 20:02:38 +00:00

konrad force-pushed feature/sentry from 5c7efb9cc0 to f23a52e008

2021-10-19 20:26:59 +00:00

Compare

dpschen requested changes 2021-10-19 21:29:10 +00:00

index.html Outdated

						
				@ -33,1 +33,3 @@

				    //

				    // 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

dpschen commented

2021-10-19 21:22:44 +00:00

Since the API_URL is can be changed at runtime and the window.API_URL is basically the "poor mans store" for the url I thought that makes kind of sense for now.

In contrast Sentry shouldn't be something that can be changed at runtime.
Maybe we can use here: https://vitejs.dev/guide/env-and-mode.html#production-replacement

Since the API_URL is can be changed at runtime and the window.API_URL is basically the "poor mans store" for the url I thought that makes kind of sense for now. In contrast Sentry shouldn't be something that can be changed at runtime. Maybe we can use here: https://vitejs.dev/guide/env-and-mode.html#production-replacement

konrad commented

2021-10-21 18:19:51 +00:00

The problem with using vite's env variables is they are not changable without recompiling everything. I'd like self-hosters to be able to enable sentry without recompiling everything, hence the window variable. I think it is fine if you'll need to change it in the index.html file before it has an effect, much like the api url.

dpschen commented

2021-10-26 12:48:27 +00:00

Having a déjà vu(e) again: I thought I ansered here already :D
Makes sense what you write! I guess is a specific need / usecase for Vikunja =)

Having a déjà vu(e) again: I thought I ansered here already :D Makes sense what you write! I guess is a specific need / usecase for Vikunja =)

dpschen marked this conversation as resolved

src/main.ts Outdated

						
				@ -127,6 +129,24 @@ app.config.globalProperties.$message = {

					success,

				}

				import * as Sentry from '@sentry/vue'

dpschen commented

2021-10-19 21:26:07 +00:00

The imports should happen inside the conditional block.
Best would be to split them in an external package aswell.
This makes especially sence since some blockers might else block some main application chunk that includes sentry.

Not sure: can sentry be loaded async? Or is that against what they recommend?

The imports should happen inside the conditional block. Best would be to split them in an external package aswell. This makes especially sence since some blockers might else block some main application chunk that includes sentry. Not sure: can sentry be loaded async? Or is that against what they recommend?

konrad commented

2021-10-21 18:21:27 +00:00

They mention something about loading it async in their docs: https://docs.sentry.io/clients/javascript/install/

So I think it should be fine to extract this in a separate component and load that ansync on startup.

They mention something about loading it async in their docs: https://docs.sentry.io/clients/javascript/install/ So I think it should be fine to extract this in a separate component and load that ansync on startup.

👍 1

konrad commented

2021-10-21 18:45:16 +00:00

Extracted and now loading async.

dpschen commented

2021-10-21 18:55:25 +00:00

Cool :) I think the link is from the legacy version. This seems to be the new: https://docs.sentry.io/platforms/javascript/install/lazy-load-sentry/

👍 1

konrad marked this conversation as resolved

src/main.ts Outdated

						
				@ -130,0 +136,4 @@

					Sentry.init({

						app,

						dsn: 'https://7e684483a06a4225b3e05cc47cae7a11@sentry.kolaente.de/2',

dpschen commented

2021-10-19 21:24:21 +00:00

The dsn should be an env variable aswell.
Thinking here of the Twelve-Factor App methodology.

The `dsn` should be an env variable aswell. Thinking here of the [Twelve-Factor App methodology](https://12factor.net/).

👍 1

konrad commented

2021-10-21 18:44:59 +00:00

Done!

🎉 1

konrad marked this conversation as resolved

dpschen force-pushed feature/sentry from 94681f0280 to d1d8c8274c

2021-10-26 12:25:36 +00:00

Compare

dpschen added 1 commit 2021-10-26 12:40:29 +00:00

continuous-integration/drone/pr Build is passing Details

a7738fa05f

fix: add SENTRY_DSN to window interface

dpschen commented

2021-10-26 12:46:30 +00:00

@konrad: I added SENTRY_DSN to the window interface. The rest looks good 👍

@konrad: I added `SENTRY_DSN` to the window interface. The rest looks good 👍

👍 1

dpschen approved these changes 2021-10-26 12:46:52 +00:00

konrad added 1 commit 2021-10-26 18:45:35 +00:00

continuous-integration/drone/pr Build is passing Details

7909e8a4df

feat: change sentry dsn

konrad commented

2021-10-26 18:46:34 +00:00

I've changed that sentry dsn since we now have sponsoring! 🥳
Probably a lot more reliable than what I'm able to host.

I've changed that sentry dsn since we now have sponsoring! 🥳 Probably a lot more reliable than what I'm able to host.

konrad merged commit 1774fdc604 into main

2021-10-26 18:53:18 +00:00

konrad deleted branch feature/sentry

2021-10-26 18:53:18 +00:00

konrad referenced this issue from a commit

2021-10-26 18:53:19 +00:00

feat: add sentry (#879)

dpschen commented

2021-10-26 20:04:15 +00:00

I've changed that sentry dsn since we now have sponsoring! 🥳
Probably a lot more reliable than what I'm able to host.

Ayyy, coool 🥳

> I've changed that sentry dsn since we now have sponsoring! 🥳 > Probably a lot more reliable than what I'm able to host. Ayyy, coool 🥳

This repo is archived. You cannot comment on pull requests.

No reviewers

No Label

No Milestone

No project

No Assignees

2 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#879