Skip to content
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

send to sentry if error #19

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

send to sentry if error #19

wants to merge 2 commits into from

Conversation

Dennis-Schroeder
Copy link
Member

is the enough to protect the code from sentry or do we need more?

@Dennis-Schroeder Dennis-Schroeder requested a review from a team August 30, 2023 12:54
lib/logger.js Outdated
@@ -234,6 +244,10 @@ function logToConsole(type, sender, msgShort, _extras) {
if (!checkType(type)) return;
if (!logThis(type)) return;

if (_extras instanceof Error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is a good approach to handle errors and log them in Sentry through some shared package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what problem do you see?
the current thing is that we write the same code over and over. we could hide it in a function within the same repo but then we copy past the same function to x repos and need to maintain them.

from a clean arch perspective this is polluting our code base it has no business value but as a developer you need to know about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configuration: Each project may have different requirements for logging, such as different log levels or different destinations for logs. If the Sentry logger is defined in a shared package, it may not be possible to configure it differently for each project.

Maintenance: If the Sentry logger is defined in a shared package, any changes to the logger will affect all projects that use the package. This can make it difficult to maintain the logger and ensure that it works correctly for each project.

Dependencies: The Sentry logger may have dependencies on other packages or libraries that are not compatible with all projects. If the logger is defined in a shared package, it may be difficult to manage these dependencies and ensure that they work correctly for each project.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configuration: I guess answered in the other comment
maintenance: 4 changes in 3 years... I'm willing to take the risk^^
dependencies: versioning

Comment on lines +20 to +29
sentry = {
dsn: isProd ? process.env.SENTRY_DNS : undefined,
integrations: [new Dedupe()], // dedupe based on fingerprint and stack trace
tracesSampleRate: 1, // 100% of all errors will be reported
environment: isProd ? 'production' : 'testing',
serverName: require('os').hostname()
}
const Sentry = require('@sentry/node')
Sentry.init({ dsn: sentry.dsn })

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, with this code you must be 1000% sure that you use same settings everywhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's just a draft... of course the must be removed. the config belongs the the repos using it.
it's just about protecting the code from sentry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants