vikunja/frontend
Archived
6
39
Fork 48
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feat: add sentry #879

Merged
konrad merged 6 commits from feature/sentry into main 2021-10-26 18:53:18 +00:00
Conversation 5 Commits 6 Files Changed 6 +118 -12
konrad commented 2021-10-19 20:02:38 +00:00
Owner
Copy Link
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
Member

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

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.

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
Member

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
Member

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

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

Extracted and now loading async.

Extracted and now loading async.
dpschen commented 2021-10-21 18:55:25 +00:00
Member

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/

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
Member

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

Done!

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
fix: add SENTRY_DSN to window interface
All checks were successful
continuous-integration/drone/pr Build is passing
Details
a7738fa05f
dpschen commented 2021-10-26 12:46:30 +00:00
Member
Copy Link

@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
feat: change sentry dsn
All checks were successful
continuous-integration/drone/pr Build is passing
Details
7909e8a4df
konrad commented 2021-10-26 18:46:34 +00:00
Author
Owner
Copy Link

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
Member
Copy Link

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.
Reviewers
No Reviewers
dpschen
Labels
Clear labels
area/internal-code
changes requested
confirmed
dependencies
duplicate
This issue or pull request already exists
good first issue
help wanted
Need some help
hosting
invalid
Something is wrong
kind/bug
Something is not working
kind/feature
New feature
question
More information is needed
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees dpschen
2 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/frontend#879
No description provided.
Delete Branch "feature/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?