-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chore/sentry #476
Merged
Merged
Chore/sentry #476
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
bba87e2
source map, webpack, stories, babel
EduardZaydler d2a1828
fixed jest tests
EduardZaydler 639dc8f
hot reload
EduardZaydler df66af1
set up
EduardZaydler 4ab0c3f
Merge branch 'chore/source-maps' into chore/sentry
EduardZaydler 13e07d6
getplatform func
EduardZaydler 5916549
configured
EduardZaydler db5eea2
initialized
EduardZaydler ee0e546
Merge remote-tracking branch origin/master into chore/sentry
EduardZaydler f6ac30f
test error dev
EduardZaydler 9d4f2b3
test error dev removed
EduardZaydler bf9c742
webpack plugin removed
EduardZaydler 66cef63
refactored
EduardZaydler 9dec219
after review
EduardZaydler c680fcc
separated error and stacktarce
EduardZaydler 3748e2c
platform on back
EduardZaydler 273bc2e
.
EduardZaydler File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import MoiraApi from "../Api/MoiraApi"; | ||
import { getPlatform, Platform } from "./common"; | ||
import * as Sentry from "@sentry/react"; | ||
|
||
const getDSN = async (api: MoiraApi) => { | ||
try { | ||
const { sentry } = await api.getConfig(); | ||
return sentry?.dsn; | ||
} catch (error) { | ||
return Promise.reject("Error getting DSN"); | ||
} | ||
}; | ||
|
||
const isLocalPlatform = getPlatform() === Platform.LOCAL; | ||
|
||
export const initSentry = async (api: MoiraApi) => { | ||
const key = await getDSN(api); | ||
Sentry.init({ | ||
dsn: key, | ||
debug: isLocalPlatform, | ||
environment: getPlatform(), | ||
enabled: !isLocalPlatform, | ||
tracesSampleRate: 1.0, | ||
}); | ||
}; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hostname.includes()
не очень безопасно.В идеале фронт не должен знать где он запускается, и такие настройки получать либо зашитыми при билде, либо запросом в бэк.
Но эт сложна, так что предлагаю тут себя обезопасить:
includes
использоватьstartsWith
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
тому есть причина: опенсорсность, 1. мы не знаем как у себя будут разворачивать мойру люди извне(с какими хост именами), а .includes дает хоть какой-то универсальности 2. тут просто нельзя добавить список наших хостов)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Так как в сентри вы настроили белый список доменов, и там только контуровские,
то значит от развёрнутых извне мойр вы не хотите получать ошибки.
Поэтому:
Вообще плевать на другие мойры
Не вижу причин почему нельзя.
А ещё определение платформы ты затащил чисто под сентри который мы используем чисто под контуровскую мойру.
Поэтому предлагаю такие изменения:
сменить нейминг
platform
- >sentryKonturPlatform
илиkonturPlatform
Оставить комментарий, что сентри работает только для контуровских доменов и не влияет на самостоятельно развёрнутые мойры
Всё таки в енам запихнуть полные хостнеймы и вместо
includes()
использоватьstartsWith()
или даже===
(можно ещё оставить коммент с ссылкой на настройки мойры, где эти домены прописаны, но это необязательно)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
на правах крокодила и прочитавшего по диагонали: если что можно утащить в конфиги ж
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не соглашусь.
а по поводу окружения, пока предлагаю оставить так и завести задачу на добавить к возвращаемому dsn еще и площадку, что бы можно было и сконфигурировать это дело и не хранить инфу о площадке на фронте
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
так мб сразу?
нам одну строчку у себя добавить как я поняла надо на бэке
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Если данные о платформе утащите на бэк, будет прям огонь
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
будем Даню ждать или кто-нибудь из ребят сделает?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
заведи сначала тасочку с пожелахами и че куда как делать, а там разберемся)