-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: add sentry logging #13
Conversation
index.js
Outdated
} | ||
return context.done(null, { |
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.
un used code path
@JasonEtco added sentry support |
Bump @JasonEtco any chance we can get this in? |
No need to bump @jakebolam - its only been a few days since you've opened this. I'm not sure that this library should include the Sentry plugin by default - its more of a size concern than anything else. @probot/maintainers feel free to weigh in! In the meantime, you can load the plugin yourself: module.exports = app => {
if (process.env.SENTRY_DSN) {
require('probot/lib/apps/sentry')(app)
}
// Your regular app code...
app.on('*', () => {})
} |
I agree that it should not be included by default for a serverless environment |
Going to close this out, given that there's a workaround above. And it seems like @gr2m and I agree that this doesn't need to be included in the extension. Thanks for starting the conversation @jakebolam ❤️ |
I've been playing around with Lambda's layers and I think that's the route I'd like to support for things like Sentry. There are some existing solutions that have started implementing things this way like IO Pipe1 and Datadog2. I know other FaaS providers are working on similar solutions, so that will probably be the "right" way to do this so the function can remain as small as possible. |
no worries, sounds good to me. |
Fixes #11