Conversation
|
There was a problem hiding this comment.
What do you think about making the tracesSampleRate configurable itself instead of coupling it to an environment? Now if we want sentry on a different testnet, then we have to add code here
There was a problem hiding this comment.
Also NODE_ENV is used in the JS ecosystem to be production or development to differentiate between those things. https://expressjs.com/en/advanced/best-practice-performance.html#set-node_env-to-production is an example. We have multiple "production" environments, in particular kovan and mainnet
There was a problem hiding this comment.
i'm open to it! i don't love the extra moving piece, but if trace rate is the only thing we will distinguish between goerli and the others happy to make this an extra config. what else might we want to be different?
There was a problem hiding this comment.
The only other thing that appears to be different is the environment field passed into Metrics.labels. That could also be configured independently.
There was a problem hiding this comment.
made this its own env var!
tynes
left a comment
There was a problem hiding this comment.
I don't love the idea of configuring things based on the name of the network, ie mainnet vs goerli because this results in needing to change code if we want to deploy to a different testnet. I could see us deploying to many ETH2 testnets in the future. I think that it would be better to just configure the actual values themselves instead of making them implicitly based on the network. This way we can change the values with a restart of the process in case we need to change them for some reason
| sentryOptions: { | ||
| release, | ||
| dsn: process.env.SENTRY_DSN, | ||
| tracesSampleRate: parseInt(process.env.SENTRY_TRACE_RATE, 10) || 0.05, |
There was a problem hiding this comment.
Would we ever want to set this to 0?
There was a problem hiding this comment.
@tynes I equate that to not deploying sentry right now
There was a problem hiding this comment.
It would disable tracing, but we primary need sentry for error tracking, so setting it to 0 would still keep error tracking enabled.
* add node_env to batch submitter * rename log to logger for service consistency * address review comments * make ETH_NETWORK_NAME to disambiguate * move tx data to debug
Description
Our services currently run without knowing their environment, which adds extra overhead to how we distinguish errors and metrics between mainnet, kovan, and soon goerli. This PR adds
NODE_ENVandETH_NETWORKto the batch submitter and the related logic for Sentry and prom-client, so we can distinguish between local dev and deployments.Additional context
I plan to make an equivalent PR for the data transport layer, but that requires a bigger refactor related to service-base so want to get this reviewed first.
Metadata