Feat: Add Sentry to batch submitter #480
Conversation
🦋 Changeset detectedLatest commit: 882fe12 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
annieke
requested review from
ben-chain,
karlfloersch,
maurelian and
smartcontracts
as code owners
| @@ -16,7 +17,8 @@ import { | |||
| } from '..' | |||
|
|
|||
| /* Logger */ | |||
| const log = new Logger({ name: 'oe:batch-submitter:init' }) | |||
| const destination = createWriteStream({ dsn: process.env.SENTRY_DSN, tracesSampleRate: 1.0 }) | |||
There was a problem hiding this comment.
We should minimize usage of process.env to a single file (i.e., something like config.ts)
There was a problem hiding this comment.
run-batch-submitter is that file for now; the whole package needs some cleanup for i think for now this is the right place
There was a problem hiding this comment.
tracesSampleRate at 1.0 is still quite high
There was a problem hiding this comment.
^updated to also be 0.05
| @@ -1,5 +1,6 @@ | |||
| /* External Imports */ | |||
| import { Logger, injectL2Context } from '@eth-optimism/core-utils' | |||
| import { createWriteStream } from 'pino-sentry' | |||
There was a problem hiding this comment.
We should maybe make this used in core-utils's logger.ts?
There was a problem hiding this comment.
^yeah this might be a good idea! pino-sentry uses an older version of pino that i can't consistently overwrite, so i wanted to test out the differences between bs (uses this older version with logging) and dtl (uses the express plugins without logging) during this initial phase, while figuring out the versioning headache
There was a problem hiding this comment.
seems like this still isn't resolved: yarnpkg/yarn#4874
There was a problem hiding this comment.
since i can't update to latest sentry, thoughts on keeping dtl on regular sentry for now and testing out pino-sentry through batch submitter? i'm down to tweak this as we go, just don't know what features we might be missing out on yet. i can also make a PR or fork pino-sentry to update
There was a problem hiding this comment.
I think the right approach is to make a PR upstream and update their dep, if not a lot of work? Not a blocker for this PR
There was a problem hiding this comment.
done! aandrewww/pino-sentry#25
|
We need to ensure that using sentry is optional because we don't want it running with local development |
So far I've tested that Sentry doesn't report errors without a dsn. We won't have pesky alerts, but I've created an issue here to add environment management so we can do away with running these monitoring services at all during localdev: #491 |
| @@ -16,7 +17,11 @@ import { | |||
| } from '..' | |||
|
|
|||
| /* Logger */ | |||
| const log = new Logger({ name: 'oe:batch-submitter:init' }) | |||
| const destination = createWriteStream({ | |||
| dsn: process.env.SENTRY_DSN, | |||
There was a problem hiding this comment.
What happens if this is not set? We need to make sure this works for local development and CI without sending messages to sentry
There was a problem hiding this comment.
So far I've tested that Sentry doesn't report errors without a dsn. We won't have pesky alerts, but I've created an issue here to add environment management so we can do away with running these monitoring services at all during localdev: #491
Sorry I missed this comment, if its not set and its a noop then its all good
* Add Sentry to batch submitter * Add Sentry to DTL * chore: changeset
Description
First step in adding alerting to batch submitter and other typescript services. This PR adds Sentry to Batch Submitter, which logs our errors and aggregates them. It is linked to pino-logger which should deliver our error logs to Sentry as well.
Additional context
(May also add Sentry to DTL in the same PR)
Metadata
https://github.com/ethereum-optimism/roadmap/issues/843