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

feat(node): Add @sentry/node/preload hook #12213

Merged
merged 4 commits into from
May 27, 2024
Merged

feat(node): Add @sentry/node/preload hook #12213

merged 4 commits into from
May 27, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented May 24, 2024

This PR adds a new way to initialize @sentry/node, which allows to use the SDK with performance instrumentation even if you cannot (for whatever reason) call Sentry.init() at the very start of your app.

CJS usage

In CommonJS mode, you can run the SDK like this:

node --require @sentry/node/preload ./app.js
// app.js
const express = require('express');
const Sentry = require('@sentry/node');

const dsn = await getSentryDsn();
Sentry.init({ dsn });

// express is instrumented even though we initialized Sentry late

ESM usage

in ESM mode, you can run the SDK like this:

node --import @sentry/node/preload ./app.mjs
// app.mjs
import express from 'express';
import * as Sentry from '@sentry/node';

const dsn = await getSentryDsn();
Sentry.init({ dsn });

// express is instrumented even though we initialized Sentry late

Configuration options

This script will by default preload all opentelemetry instrumentation. You can choose to instrument only specific packages like this:

SENTRY_PRELOAD_INTEGRATIONS="Http,Express,Graphql" --import @sentry/node/preload ./app.mjs

You can also enable debug logging for the script via SENTRY_DEBUG=true.

Manually preloading

It is also possible to manually call preloadOpenTelemetry() to achieve the same thing. For example, in a CJS app you could do the following thing if you want to initialize late but don't want to use --require:

// preload.js
const Sentry = require('@sentry/node');
Sentry.preloadOpenTelemetry();

// app.js
// call this first, before any other requires!
require('./preload.js');
// Then, other stuff
const express = require('express');
const Sentry = require('@sentry/node');

const dsn = await getSentryDsn();
Sentry.init({ dsn });

@mydea mydea self-assigned this May 24, 2024
@AbhiPrasad
Copy link
Member

What happens if a user manually initializes OpenTelemetry? Can they still delay calling Sentry.init?

@timfish
Copy link
Collaborator

timfish commented May 24, 2024

How does this differ from using --import @sentry/node/import? I can see the code differenced but why are these differences required?

I assumed --import @sentry/node/import would load import-in-the-middle and then import order in the app doesn't matter, it still instruments libraries.

Copy link
Contributor

github-actions bot commented May 24, 2024

size-limit report 📦

Path Size
@sentry/browser 21.74 KB (0%)
@sentry/browser (incl. Tracing) 32.77 KB (0%)
@sentry/browser (incl. Tracing, Replay) 68.24 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.66 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 72.29 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 84.32 KB (0%)
@sentry/browser (incl. Feedback) 37.75 KB (0%)
@sentry/browser (incl. sendFeedback) 26.31 KB (0%)
@sentry/browser (incl. FeedbackAsync) 30.73 KB (0%)
@sentry/react 24.43 KB (0%)
@sentry/react (incl. Tracing) 35.77 KB (0%)
@sentry/vue 25.68 KB (0%)
@sentry/vue (incl. Tracing) 34.58 KB (0%)
@sentry/svelte 21.88 KB (0%)
CDN Bundle 24.28 KB (0%)
CDN Bundle (incl. Tracing) 34.22 KB (0%)
CDN Bundle (incl. Tracing, Replay) 68.03 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 73.02 KB (0%)
CDN Bundle - uncompressed 71.46 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 101.55 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed 211.46 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 223.81 KB (0%)
@sentry/nextjs (client) 35.12 KB (0%)
@sentry/sveltekit (client) 33.36 KB (0%)
@sentry/node 114.6 KB (+0.25% 🔺)
@sentry/aws-serverless 103.29 KB (+0.09% 🔺)

@mydea
Copy link
Member Author

mydea commented May 24, 2024

How does this differ from using --import @sentry/node/import? I can see the code differenced but why are these differences required?

I assumed --import @sentry/node/import would load import-in-the-middle and then import order in the app doesn't matter, it still instruments libraries.

In our tests, import/execution order sadly still mattered. If something was imported before we added the OTEL instrumentation, it did not work :( Actually, the test is not "good" because in this specific test stuff does work. I updated this now, it fails when something is used before Sentry is run. E.g.:

import express from 'express';

const app = express();
// setup app...

// somewhere later
setTimeout(function() {
  Sentry.init(...);
}, 1000);

In this case, express is already uninstrumented before we run init :/

What happens if a user manually initializes OpenTelemetry? Can they still delay calling Sentry.init?

This should all still work, basically this does nothing except setup the instrumentations, it does not actually setup otel itself (e.g. no span processor etc. is created yet).

@andreiborza
Copy link
Member

How does this differ from using --import @sentry/node/import? I can see the code differenced but why are these differences required?

I assumed --import @sentry/node/import would load import-in-the-middle and then import order in the app doesn't matter, it still instruments libraries.

If I understood correctly, some people didn't have a DSN ready by the time we need to import/init everything for instrumentation to work. This would be a workaround.

@mydea
Copy link
Member Author

mydea commented May 24, 2024

How does this differ from using --import @sentry/node/import? I can see the code differenced but why are these differences required?
I assumed --import @sentry/node/import would load import-in-the-middle and then import order in the app doesn't matter, it still instruments libraries.

If I understood correctly, some people didn't have a DSN ready by the time we need to import/init everything for instrumentation to work. This would be a workaround.

You can use @sentry/node/import without DSN, which does setup up import-in-the-middle correctly. but this still requires the otel instrumentation to be loaded before a module is used :/

@timfish
Copy link
Collaborator

timfish commented May 24, 2024

// somewhere later
setTimeout(function() {
  Sentry.init(...);
}, 1000);

In this case, express is already uninstrumented before we run init :/

Ah that does make sense for that case.

Is there any reason not to encourage using an instrument.js instead?

node --require ./instrument.js app.js
// instrument.js
const Sentry = require('@sentry/node');
const dsn = await getSentryDsn();
Sentry.init({ dsn });

// app.js
const express = require('express');

@mydea
Copy link
Member Author

mydea commented May 24, 2024

// somewhere later
setTimeout(function() {
  Sentry.init(...);
}, 1000);

In this case, express is already uninstrumented before we run init :/

Ah that does make sense for that case.

Is there any reason not to encourage using an instrument.js instead?

node --require ./instrument.js app.js
// instrument.js
const Sentry = require('@sentry/node');
const dsn = await getSentryDsn();
Sentry.init({ dsn });

// app.js
const express = require('express');

Yeah, we generally do recommend that, but this is not possible if users cannot init at this point - e.g. if they load their DSN from somewhere else (something that we don't recommend, but there are people out there that have setups like this...)

So this is really just an alternative way to get stuff running for these people, and will def. not be the recommended way to init sentry, but just an escape hatch!

Copy link
Collaborator

@timfish timfish left a comment

Choose a reason for hiding this comment

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

Ah yes understood!

My only minor concern is that we're racking up quite a few different ways to initialise the SDK.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

My only minor concern is that we're racking up quite a few different ways to initialise the SDK.

yeah I also feel this, but I think this is an important thing to expose, so let's release. Maybe we can iterate and combine @sentry/node/preload and @sentry/node/import somehow 🤔.

@mydea
Copy link
Member Author

mydea commented May 27, 2024

My only minor concern is that we're racking up quite a few different ways to initialise the SDK.

yeah I also feel this, but I think this is an important thing to expose, so let's release. Maybe we can iterate and combine @sentry/node/preload and @sentry/node/import somehow 🤔.

We could, eventually, make import be a variant of preload with some env var set 🤔 but let's get this out for now and tweak stuff later as needed!

@mydea mydea merged commit b188e61 into develop May 27, 2024
104 checks passed
@mydea mydea deleted the fn/lazy-init branch May 27, 2024 07:52
@jeengbe
Copy link
Contributor

jeengbe commented May 27, 2024

Out of curiosity, what's a use case for this? I can't imagine any situation in which you don't have control over the entry point of your code. Maybe with frameworks like Next?

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.

5 participants