feat: integrate Sentry monitoring - #196
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
+ Coverage 36.71% 37.22% +0.51%
==========================================
Files 13 13
Lines 365 368 +3
Branches 108 108
==========================================
+ Hits 134 137 +3
Misses 219 219
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gitstream-cm[bot]](https://avatars.githubusercontent.com/in/230441?s=60&v=4){kind=small}
There was a problem hiding this comment.
✨ PR Review
LGTM
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
|
@CharlieHelps can we use github secrets for the sentry env setup |
|
Yes, we can absolutely use GitHub Secrets here. The Sentry setup in this PR only reads from For GitHub Actions, you’d:
jobs:
run-mcp:
runs-on: ubuntu-latest
env:
SENTRY_DSN: ${{ secrets.SENTRY_DSN }}
SENTRY_ENVIRONMENT: production
SENTRY_TRACES_SAMPLE_RATE: "0.2"
SENTRY_SEND_DEFAULT_PII: "false"
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: 22
- run: pnpm install
- run: pnpm run startIf you’d like, I can follow up with a small README note or workflow snippet in this repo showing the “use GitHub Secrets for Sentry vars” pattern. |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new Sentry integration is functionally sound but centralizes configuration in module-level state, which reduces testability and may cause surprising behavior when environment variables change after module load. Moving Sentry configuration and initialization behind injectable functions (or parameters to buildServer) would better respect entrypoint boundaries and make the behavior easier to test and reuse. The environment variable parsing and README updates are otherwise clear and consistent with the intended opt-in design. No obvious bugs or performance issues were found in the modified code segments.
Summary of changes
Overview of Changes
- Added optional Sentry monitoring support via
@sentry/nodedependency and its transitive OpenTelemetry stack inpackage.jsonandpnpm-lock.yaml. - Extended
.env.sampleandREADME.mdwith Sentry-related environment variables (SENTRY_DSN,SENTRY_ENVIRONMENT,SENTRY_TRACES_SAMPLE_RATE,SENTRY_SEND_DEFAULT_PII) and documentation of behavior. - Introduced
getSentryConfigFromEnvand Sentry initialization logic insrc/index.ts, including:- Reading and validating Sentry env vars.
- Conditionally calling
Sentry.initand wrapping theMcpServerinstance withSentry.wrapMcpServerWithSentry.
- Updated
buildServerto construct a base MCP server and wrap it with Sentry when configured. - Relaxed the explicit type on the
process.exitmock insrc/index.test.tsto satisfytscwhile preserving behavior. - Fixed a minor markdown formatting issue in the README acknowledgements list.
| function getSentryConfigFromEnv() { | ||
| const dsn = process.env.SENTRY_DSN; | ||
| if (!dsn) { | ||
| return null; | ||
| } | ||
|
|
||
| let tracesSampleRate = 1.0; | ||
| const tracesSampleRateEnv = process.env.SENTRY_TRACES_SAMPLE_RATE; | ||
| if (tracesSampleRateEnv !== undefined) { | ||
| const parsed = Number.parseFloat(tracesSampleRateEnv); | ||
| if (Number.isNaN(parsed) || parsed < 0 || parsed > 1) { | ||
| console.error( | ||
| `Invalid SENTRY_TRACES_SAMPLE_RATE="${tracesSampleRateEnv}", falling back to 1.0. Expected a number between 0 and 1.`, | ||
| ); | ||
| } else { | ||
| tracesSampleRate = parsed; | ||
| } | ||
| } | ||
|
|
||
| const sendDefaultPiiEnv = process.env.SENTRY_SEND_DEFAULT_PII; | ||
| const sendDefaultPii = | ||
| sendDefaultPiiEnv === "true" || sendDefaultPiiEnv === "1"; | ||
|
|
||
| return { | ||
| dsn, | ||
| tracesSampleRate, | ||
| sendDefaultPii, | ||
| environment: process.env.SENTRY_ENVIRONMENT, | ||
| }; | ||
| } | ||
|
|
||
| const sentryConfig = getSentryConfigFromEnv(); |
There was a problem hiding this comment.
This helper currently bakes in the environment-variable reading and is evaluated at module load time via the sentryConfig constant. That makes it awkward to write focused tests around whether Sentry wrapping is applied and also makes it harder to override configuration in alternate entrypoints (e.g., if createServer is reused in a different runtime). Extracting the Sentry configuration and initialization behind an injectable function (or passing sentryConfig as an argument into buildServer) would improve testability and keep the side effects at the true entrypoint boundary rather than in module scope.
Additionally, because Sentry.init is called at import time when SENTRY_DSN is set, any consumer importing configSchema/buildServer in a different process (like tooling) will also incur Sentry initialization, which may not be desired in all contexts.
Suggestion
Consider restructuring so that Sentry configuration and initialization are tied strictly to the CLI/entrypoint path rather than module import, for example:
export function getSentryConfigFromEnv(env: NodeJS.ProcessEnv = process.env) {
const dsn = env.SENTRY_DSN;
if (!dsn) return null;
// ...rest unchanged...
}
function initSentryIfConfigured(env: NodeJS.ProcessEnv = process.env) {
const cfg = getSentryConfigFromEnv(env);
if (!cfg) return null;
Sentry.init(cfg);
return cfg;
}
// In your actual `runServer` / CLI entrypoint:
const sentryConfig = initSentryIfConfigured();
function buildServer(apiKey: string, sentryCfg = sentryConfig) {
const baseServer = new McpServer({ name, version });
const server = sentryCfg ? Sentry.wrapMcpServerWithSentry(baseServer) : baseServer;
// ...
}This keeps side effects at the executable boundary and makes getSentryConfigFromEnv and buildServer directly testable with injected environment/config objects. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor.
|
@CharlieHelps we want that they are backed into the built mcp server so that we have observability from our users please use and don't use .env for sentry, we can have the config in the code |
Expand this to see my work.
|
|
I’ve updated the PR so Sentry is now baked into the built MCP server using your provided config and no Changes
Verification# Install deps (pull in @sentry/node)
$ pnpm install
# Build
$ pnpm run build
# TypeScript typecheck (no emit)
$ npx tsc --noEmit
# Unit + non-API integration tests
$ pnpm vitest run src/index.test.ts src/utils/*.test.ts \
tests/docker.test.ts tests/integration/http-transport.integration.test.ts
# Biome format/lint (auto-fix on)
$ pnpm run check
|
There was a problem hiding this comment.
✨ PR Review
The PR adds Sentry monitoring with hardcoded configuration, but there's a significant disconnect between the PR description and actual implementation regarding opt-in behavior.
2 issues detected:
🔒 Security - Hardcoded telemetry initialization without user consent contradicts the described opt-in behavior
Details: The PR description claims Sentry monitoring is "fully opt-in via environment variables" and mentions a
getSentryConfigFromEnvhelper, but the actual implementation hardcodes Sentry configuration and always initializes it without any opt-out mechanism. This creates a privacy concern as all users will have their usage data sent to the hardcoded DSN without consent.
File:src/index.ts🚀 Performance - 100% trace sampling may cause performance overhead and excessive data collection 🛠️
Details: Setting
tracesSampleRate: 1.0means 100% of all traces will be captured and sent to Sentry, which could impact performance in high-traffic scenarios and generate excessive telemetry data.
File:src/index.ts (12-12)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| const sentryConfig = { | ||
| dsn: "https://ce696d8333b507acbf5203eb877bce0f@o4508975499575296.ingest.de.sentry.io/4509049671647312", | ||
| // Tracing must be enabled for MCP monitoring to work | ||
| tracesSampleRate: 1.0, |
There was a problem hiding this comment.
🚀 Performance - Full Trace Sampling: Consider using a lower sample rate (e.g., 0.1 or 0.01) or making it configurable via environment variables to balance observability with performance.
| tracesSampleRate: 1.0, | |
| tracesSampleRate: process.env.SENTRY_TRACES_SAMPLE_RATE ? parseFloat(process.env.SENTRY_TRACES_SAMPLE_RATE) : 0.1, |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
There was a problem hiding this comment.
The Sentry integration is functionally correct but introduces strong global side effects by initializing Sentry at module load and wrapping McpServer inside buildServer, which reduces testability and composability. The change also shifts from an opt-in, env-driven telemetry model to an always-on, hard-coded DSN with no in-repo opt-out besides forking, which may surprise existing users and makes the core module harder to reuse in different contexts. README updates correctly describe the new behavior, but a small, code-level escape hatch for disabling telemetry would provide better flexibility without undermining observability goals. No obvious logical bugs or performance regressions are apparent in the modified code.
Additional notes (2)
-
Readability |
README.md:98-111
The README correctly documents that Sentry usessendDefaultPii: falseand that disabling telemetry requires forking. However, it does not explicitly state that: -
The DSN is hard-coded in the source, and
-
Initialization happens eagerly when the server entry module is used (rather than being controlled via config/env).
Clarifying these two points would better set expectations for security/privacy reviewers and downstream users inspecting the behavior, and would reduce surprises for anyone trying to reason about when and how telemetry is sent.
- Maintainability |
src/index.test.ts:101-106
The change to theprocess.exitspy removes the explicit type annotation on_code, which resolves the earlier TypeScript compilation issue while keeping the test behavior intact. There is no logical or behavioral regression here; the test still asserts thatprocess.exit(1)is called and throws as expected.
Summary of changes
Summary of Changes
- Added
@sentry/nodeas a runtime dependency and wired in the full Sentry MCP/OpenTelemetry stack viapnpm-lock.yaml. - Baked Sentry monitoring directly into
src/index.ts: Sentry is initialized at module load with a hard-coded DSN,tracesSampleRate: 1.0, andsendDefaultPii: false, and theMcpServeris always wrapped withSentry.wrapMcpServerWithSentry. - Updated the README with a new Sentry monitoring section explaining that telemetry is always on in the published package and can only be disabled via a fork.
- Kept
.env.sampleeffectively unchanged (only whitespace/formatting), and slightly simplified theprocess.exitspy insrc/index.test.ts. - Lockfile changes pull in many Sentry/OpenTelemetry-related dependencies and types as a result of the new
@sentry/nodedependency.
| ### Sentry monitoring | ||
|
|
||
| `hevy-mcp` ships with Sentry monitoring baked into the built MCP server so | ||
| that usage and errors from published builds can be observed. | ||
|
|
||
| The server initializes `@sentry/node` with a fixed DSN and tracing settings | ||
| directly in the code (see `src/index.ts`), and wraps the underlying | ||
| `McpServer` with `Sentry.wrapMcpServerWithSentry` so requests and tool calls | ||
| are captured by Sentry automatically. The configuration uses | ||
| `sendDefaultPii: false` to keep Sentry's default PII collection disabled. | ||
|
|
||
| There is currently no built-in toggle to disable Sentry for the published | ||
| package. If you need a build without Sentry telemetry, you can fork the | ||
| repository and remove the Sentry initialization in `src/index.ts`. |
There was a problem hiding this comment.
The new hard-coded DSN and always-on Sentry initialization mean telemetry is permanently enabled for all users of the published package, and the README states the only way to disable it is to fork and edit src/index.ts. That is a runtime behavior change from the previous opt-in, env-based design and may surprise existing users who pull a new version expecting no network calls unless explicitly configured.
Even if this change is product-approved, it would be safer to at least keep a minimal guard (e.g., behind a HEVY_MCP_TELEMETRY_DISABLED env var or a documented flag) to support privacy-sensitive environments without forcing a fork, and to allow CI/local testing without external calls when desired.
Suggestion
If product constraints allow any configurability, introduce a single opt-out mechanism in code (for example, an environment variable like HEVY_MCP_DISABLE_SENTRY or HEVY_MCP_TELEMETRY=off) and mention it in this README section. The implementation can be as small as:
- Checking the env var before calling
Sentry.init, and - Skipping
wrapMcpServerWithSentrywhen telemetry is disabled.
This keeps telemetry effectively always-on in normal deployments, but gives privacy-sensitive users and CI a documented escape hatch without requiring a fork. Reply with "@CharlieHelps yes please" if you'd like me to add a commit wiring in such a minimal opt-out and updating the docs.
# [1.14.0](v1.13.2...v1.14.0) (2025-12-10) ### Features * integrate Sentry monitoring ([#196](#196)) ([4ea9b31](4ea9b31))
2 participants
Implement optional Sentry monitoring for the MCP server, wiring it up to the
Sentry MCP integration while keeping it fully opt-in via environment
variables.
Changes
@sentry/noderuntime dependency (v9.47.1) to enable Sentry MCPinstrumentation.
src/index.tsvia a newgetSentryConfigFromEnvhelper that reads:
SENTRY_DSN(required to enable Sentry),SENTRY_TRACES_SAMPLE_RATE(validated as a number between 0 and 1,falling back to
1.0with a stderr warning when invalid),SENTRY_ENVIRONMENT, andSENTRY_SEND_DEFAULT_PII(only"true"or"1"enable PII capture).McpServerinstance withSentry.wrapMcpServerWithSentrywhen avalid Sentry config is present so tool calls and errors are captured in
Sentry; when
SENTRY_DSNis unset, behavior is unchanged..env.sampleand the README configuration section with Sentryenvironment variables and explicit notes about PII and sampling defaults.
process.exitmock insrc/index.test.tssotsc --noEmitpasses while preserving existing testbehavior.
Verification
biome.jsonvs CLI version.noExplicitAnywarnings insrc/tools/webhooks.ts(
hevyClient as any), untouched in this PR.tests/integration/hevy-mcp.integration.test.tswas intentionally not runbecause it requires a real
HEVY_API_KEY; this PR does not change anyHevy API surface.
I kept the implementation minimal and did not restructure the entrypoint to
make
sentryConfiginjectable, since the behavior is a straightforwardwrapper and is indirectly covered by the existing server entry tests.
Closes #194
✨ PR Description
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how